Hotfix/crash invalid vk binding (#875)

* Add diagnostic for vk::binding failure.

* Add test for vk::binding failure.

* Add the expected output for glsl-layout-define.hlsl

* * Copy over initialize expr if available when validating unchecked
* Fix unloop - because now it always has one parameter (when before it could have none)
* Split vk::binding and layout tests with invalid parameters
* Removed the diagnostic for 2 ints expected
* Added vk::binding that doesn't specify set in vk-bindings.slang

* * Fix typo
* Improve comments.

9 files changed, 66 insertions, 6 deletions

diff --git a/source/slang/check.cpp b/source/slang/check.cpp
index 998324612..37c0d5baf 100644
--- a/source/slang/check.cpp
+++ b/source/slang/check.cpp
@@ -2603,10 +2603,24 @@ namespace Slang
                 }
                 else if (auto bindingAttr = as<GLSLBindingAttribute>(attr))
                 {
-                    SLANG_ASSERT(attr->args.Count() == 2);
+                    // This must be vk::binding or gl::binding (as specified in core.meta.slang under vk_binding/gl_binding)
+                    // Must have 2 int parameters. Ideally this would all be checked from the specification
+                    // in core.meta.slang, but that's not completely implemented. So for now we check here.
+                    if (attr->args.Count() != 2)
+                    {
+                        return false;
+                    }
+
+                    // TODO(JS): Prior validation currently doesn't ensure both args are ints (as specified in core.meta.slang), so check here
+                    // to make sure they both are
                     auto binding = checkConstantIntVal(attr->args[0]);
                     auto set = checkConstantIntVal(attr->args[1]);
 
+                    if (binding == nullptr || set == nullptr)
+                    {
+                        return false;
+                    }
+                    
                     bindingAttr->binding = int32_t(binding->value);
                     bindingAttr->set = int32_t(set->value);
                 }
@@ -2703,6 +2717,13 @@ namespace Slang
                         return false;
                     }
                 }
+                else if (auto unrollAttr = as<UnrollAttribute>(attr))
+                {
+                    // Check has an argument. We need this because default behavior is to give an error
+                    // if an attribute has arguments, but not handled explicitly (and the default param will come through
+                    // as 1 arg if nothing is specified)
+                    SLANG_ASSERT(attr->args.Count() == 1);
+                }
                 else if (auto userDefAttr = as<UserDefinedAttribute>(attr))
                 {
                     // check arguments against attribute parameters defined in attribClassDecl
@@ -2813,7 +2834,6 @@ namespace Slang
                 {
                     // TODO: support checking the argument against the declared
                     // type for the parameter.
-                    
                 }
                 else
                 {
@@ -2826,6 +2846,9 @@ namespace Slang
                         //
                         // TODO: we need to figure out how to hook up
                         // default arguments as needed.
+                        // For now just copy the expression over.
+
+                        attr->args.Add(paramDecl->initExpr);
                     }
                     else
                     {
diff --git a/source/slang/diagnostic-defs.h b/source/slang/diagnostic-defs.h
index a00c69dba..c773cc4ed 100644
--- a/source/slang/diagnostic-defs.h
+++ b/source/slang/diagnostic-defs.h
@@ -259,7 +259,6 @@ DIAGNOSTIC(31005, Error, expectedSingleStringArg, "attribute '$0' expects a sing
 
 DIAGNOSTIC(31006, Error, attributeFunctionNotFound, "Could not find function '$0' for attribute'$1'")
 
-
 DIAGNOSTIC(31100, Error, unknownStageName, "unknown stage name '$0'")
 DIAGNOSTIC(31120, Error, invalidAttributeTarget, "invalid syntax target for user defined attribute")
 
diff --git a/source/slang/parser.cpp b/source/slang/parser.cpp
index 72e682a63..a7ccb7b8a 100644
--- a/source/slang/parser.cpp
+++ b/source/slang/parser.cpp
@@ -91,7 +91,7 @@ namespace Slang
         RefPtr<Scope> currentScope;
 
         TokenReader tokenReader;
-        DiagnosticSink * sink;
+        DiagnosticSink* sink;
         int genericDepth = 0;
 
         // Have we seen any `import` declarations? If so, we need
@@ -819,8 +819,12 @@ namespace Slang
         auto keywordToken = advanceToken(parser);
 
         RefPtr<RefObject> parsedObject = syntaxDecl->parseCallback(parser, syntaxDecl->parseUserData);
-        auto syntax = as<T>(parsedObject);
+        if (!parsedObject)
+        {
+            return false;
+        }
 
+        auto syntax = as<T>(parsedObject);
         if (syntax)
         {
             if (!syntax->loc.isValid())
@@ -4440,7 +4444,14 @@ namespace Slang
 
                 parser->ReadToken(TokenType::OpAssign);
 
+                // If the token asked for is not returned found will put in recovering state, and return token found
                 Token valToken = parser->ReadToken(TokenType::IntegerLiteral);
+                // If wasn't the desired IntegerLiteral return that couldn't parse
+                if (valToken.type != TokenType::IntegerLiteral)
+                {
+                    return nullptr;
+                }
+
                 // Work out the value
                 auto value = getIntegerLiteralValue(valToken);
 
diff --git a/tests/bugs/glsl-layout-define.hlsl b/tests/bugs/glsl-layout-define.hlsl
new file mode 100644
index 000000000..8f09fd592
--- /dev/null
+++ b/tests/bugs/glsl-layout-define.hlsl
@@ -0,0 +1,5 @@
+//TEST:SIMPLE: -profile vs_5_0
+
+layout(binding = UNDEFINED_VK_BINDING, set = UNDEFINED_VK_SET) 
+Texture2DArray<float4> Float4Texture2DArrays[] : register(t0, space100);
+
diff --git a/tests/bugs/glsl-layout-define.hlsl.expected b/tests/bugs/glsl-layout-define.hlsl.expected
new file mode 100644
index 000000000..9c00916c3
--- /dev/null
+++ b/tests/bugs/glsl-layout-define.hlsl.expected
@@ -0,0 +1,7 @@
+result code = -1
+standard error = {
+tests/bugs/glsl-layout-define.hlsl(3): error 20001: unexpected identifier, expected integer literal
+tests/bugs/glsl-layout-define.hlsl(3): error 20001: unexpected ')', expected ';'
+}
+standard output = {
+}
diff --git a/tests/bugs/glsl-vk-binding-define.hlsl b/tests/bugs/glsl-vk-binding-define.hlsl
new file mode 100644
index 000000000..4e68c0f75
--- /dev/null
+++ b/tests/bugs/glsl-vk-binding-define.hlsl
@@ -0,0 +1,5 @@
+//TEST:SIMPLE: -profile vs_5_0
+
+[[vk::binding(UNDEFINED_VK_BINDING, UNDEFINED_VK_SET)]]
+Texture2DArray<float4> Float4Texture2DArrays[] : register(t0, space100);
+
diff --git a/tests/bugs/glsl-vk-binding-define.hlsl.expected b/tests/bugs/glsl-vk-binding-define.hlsl.expected
new file mode 100644
index 000000000..aaac7f0c6
--- /dev/null
+++ b/tests/bugs/glsl-vk-binding-define.hlsl.expected
@@ -0,0 +1,7 @@
+result code = -1
+standard error = {
+tests/bugs/glsl-vk-binding-define.hlsl(3): error 30015: undefined identifier 'UNDEFINED_VK_BINDING'.
+tests/bugs/glsl-vk-binding-define.hlsl(3): error 30015: undefined identifier 'UNDEFINED_VK_SET'.
+}
+standard output = {
+}
diff --git a/tests/diagnostics/vk-bindings.slang b/tests/diagnostics/vk-bindings.slang
index f5a8bdeba..a329fcdde 100644
--- a/tests/diagnostics/vk-bindings.slang
+++ b/tests/diagnostics/vk-bindings.slang
@@ -5,6 +5,9 @@
 // D3D `register` without VK binding
 Texture2D t : register(t0);
 
+[[vk::binding(3)]]
+Texture2D t1 : register(t3);
+
 struct S { float4 a; };
 
 // Parameter block with non-zero binding:
diff --git a/tests/diagnostics/vk-bindings.slang.expected b/tests/diagnostics/vk-bindings.slang.expected
index 8e085b55f..8744787ed 100644
--- a/tests/diagnostics/vk-bindings.slang.expected
+++ b/tests/diagnostics/vk-bindings.slang.expected
@@ -1,7 +1,7 @@
 result code = -1
 standard error = {
 tests/diagnostics/vk-bindings.slang(6): warning 39013: shader parameter 't' has a 'register' specified for D3D, but no '[[vk::binding(...)]]` specified for Vulkan
-tests/diagnostics/vk-bindings.slang(11): error 39015: shader parameter 'b' consumes whole descriptor sets, so the binding must be in the form '[[vk::binding(0, ...)]]'; the non-zero binding '2' is not allowed
+tests/diagnostics/vk-bindings.slang(14): error 39015: shader parameter 'b' consumes whole descriptor sets, so the binding must be in the form '[[vk::binding(0, ...)]]'; the non-zero binding '2' is not allowed
 }
 standard output = {
 }