add invalid_param diagnostic to differentiate between missing comma vs invalid token

Author: inbelic

clang/include/clang/Basic/DiagnosticParseKinds.td

@@ -1860,6 +1860,7 @@ def err_hlsl_virtual_inheritance
     : Error<"virtual inheritance is unsupported in HLSL">;
 
 // HLSL Root Signature Parser Diagnostics
+def err_hlsl_invalid_param : Error<"invalid parameter of %0">;
 def err_hlsl_rootsig_repeat_param : Error<"specified the same parameter '%0' multiple times">;
 def err_hlsl_rootsig_missing_param : Error<"did not specify mandatory parameter '%0'">;
 def err_hlsl_number_literal_overflow : Error<

clang/include/clang/Parse/ParseHLSLRootSignature.h

@@ -100,7 +100,8 @@ class RootSignatureParser {
     std::optional<llvm::dxbc::RootDescriptorFlags> Flags;
   };
   std::optional<ParsedRootDescriptorParams>
-  parseRootDescriptorParams(RootSignatureToken::Kind RegType);
+  parseRootDescriptorParams(RootSignatureToken::Kind DescKind,
+                            RootSignatureToken::Kind RegType);
 
   struct ParsedClauseParams {
     std::optional<llvm::hlsl::rootsig::Register> Reg;
@@ -110,7 +111,8 @@ class RootSignatureParser {
     std::optional<llvm::dxbc::DescriptorRangeFlags> Flags;
   };
   std::optional<ParsedClauseParams>
-  parseDescriptorTableClauseParams(RootSignatureToken::Kind RegType);
+  parseDescriptorTableClauseParams(RootSignatureToken::Kind ClauseKind,
+                                   RootSignatureToken::Kind RegType);
 
   struct ParsedStaticSamplerParams {
     std::optional<llvm::hlsl::rootsig::Register> Reg;

clang/lib/Parse/ParseHLSLRootSignature.cpp

@@ -59,6 +59,11 @@ bool RootSignatureParser::parse() {
       if (!Sampler.has_value())
         return true;
       Elements.emplace_back(RootSignatureElement(ElementLoc, *Sampler));
+    } else {
+      consumeNextToken(); // let diagnostic be at the start of invalid token
+      reportDiag(diag::err_hlsl_invalid_param)
+          << /*param of*/ TokenKind::kw_RootSignature;
+      return true;
     }
 
     // ',' denotes another element, otherwise, expected to be at end of stream
@@ -198,7 +203,7 @@ std::optional<RootDescriptor> RootSignatureParser::parseRootDescriptor() {
   }
   Descriptor.setDefaultFlags(Version);
 
-  auto Params = parseRootDescriptorParams(ExpectedReg);
+  auto Params = parseRootDescriptorParams(DescriptorKind, ExpectedReg);
   if (!Params.has_value())
     return std::nullopt;
 
@@ -261,6 +266,11 @@ std::optional<DescriptorTable> RootSignatureParser::parseDescriptorTable() {
       Visibility = parseShaderVisibility();
       if (!Visibility.has_value())
         return std::nullopt;
+    } else {
+      consumeNextToken(); // let diagnostic be at the start of invalid token
+      reportDiag(diag::err_hlsl_invalid_param)
+          << /*param of*/ TokenKind::kw_DescriptorTable;
+      return std::nullopt;
     }
 
     // ',' denotes another element, otherwise, expected to be at ')'
@@ -316,7 +326,7 @@ RootSignatureParser::parseDescriptorTableClause() {
   }
   Clause.setDefaultFlags(Version);
 
-  auto Params = parseDescriptorTableClauseParams(ExpectedReg);
+  auto Params = parseDescriptorTableClauseParams(ParamKind, ExpectedReg);
   if (!Params.has_value())
     return std::nullopt;
 
@@ -474,6 +484,11 @@ RootSignatureParser::parseRootConstantParams() {
       if (!Visibility.has_value())
         return std::nullopt;
       Params.Visibility = Visibility;
+    } else {
+      consumeNextToken(); // let diagnostic be at the start of invalid token
+      reportDiag(diag::err_hlsl_invalid_param)
+          << /*param of*/ TokenKind::kw_RootConstants;
+      return std::nullopt;
     }
 
     // ',' denotes another element, otherwise, expected to be at ')'
@@ -485,7 +500,8 @@ RootSignatureParser::parseRootConstantParams() {
 }
 
 std::optional<RootSignatureParser::ParsedRootDescriptorParams>
-RootSignatureParser::parseRootDescriptorParams(TokenKind RegType) {
+RootSignatureParser::parseRootDescriptorParams(TokenKind DescKind,
+                                               TokenKind RegType) {
   assert(CurToken.TokKind == TokenKind::pu_l_paren &&
          "Expects to only be invoked starting at given token");
 
@@ -543,6 +559,10 @@ RootSignatureParser::parseRootDescriptorParams(TokenKind RegType) {
       if (!Flags.has_value())
         return std::nullopt;
       Params.Flags = Flags;
+    } else {
+      consumeNextToken(); // let diagnostic be at the start of invalid token
+      reportDiag(diag::err_hlsl_invalid_param) << /*param of*/ DescKind;
+      return std::nullopt;
     }
 
     // ',' denotes another element, otherwise, expected to be at ')'
@@ -554,7 +574,8 @@ RootSignatureParser::parseRootDescriptorParams(TokenKind RegType) {
 }
 
 std::optional<RootSignatureParser::ParsedClauseParams>
-RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) {
+RootSignatureParser::parseDescriptorTableClauseParams(TokenKind ClauseKind,
+                                                      TokenKind RegType) {
   assert(CurToken.TokKind == TokenKind::pu_l_paren &&
          "Expects to only be invoked starting at given token");
 
@@ -638,6 +659,10 @@ RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) {
       if (!Flags.has_value())
         return std::nullopt;
       Params.Flags = Flags;
+    } else {
+      consumeNextToken(); // let diagnostic be at the start of invalid token
+      reportDiag(diag::err_hlsl_invalid_param) << /*param of*/ ClauseKind;
+      return std::nullopt;
     }
 
     // ',' denotes another element, otherwise, expected to be at ')'
@@ -833,6 +858,11 @@ RootSignatureParser::parseStaticSamplerParams() {
       if (!Visibility.has_value())
         return std::nullopt;
       Params.Visibility = Visibility;
+    } else {
+      consumeNextToken(); // let diagnostic be at the start of invalid token
+      reportDiag(diag::err_hlsl_invalid_param)
+          << /*param of*/ TokenKind::kw_StaticSampler;
+      return std::nullopt;
     }
 
     // ',' denotes another element, otherwise, expected to be at ')'

clang/test/SemaHLSL/RootSignature-err.hlsl

@@ -17,24 +17,41 @@ void bad_root_signature_2() {}
 [RootSignature(""), RootSignature("")] // expected-warning {{attribute 'RootSignature' is already applied}}
 void bad_root_signature_3() {}
 
-[RootSignature("DescriptorTable(), invalid")] // expected-error {{expected end of stream}}
+// expected-error@+1 {{invalid parameter of RootSignature}}
+[RootSignature("DescriptorTable(), invalid")]
 void bad_root_signature_4() {}
 
-// expected-error@+1 {{expected ')'}}
+// expected-error@+1 {{invalid parameter of RootConstants}}
 [RootSignature("RootConstants(b0, num32BitConstants = 1, invalid)")]
 void bad_root_signature_5() {}
 
 #define MultiLineRootSignature \
  "CBV(b0)," \
  "RootConstants(num32BitConstants = 3, b0, invalid)"
 
-// CHECK: [[@LINE-2]]:42: note: expanded from macro 'MultiLineRootSignature'
+// CHECK: [[@LINE-2]]:44: note: expanded from macro 'MultiLineRootSignature'
 // CHECK-NEXT: [[@LINE-3]] | "RootConstants(num32BitConstants = 3, b0, invalid)"
-// CHECK-NEXT:             |                                         ^
-// expected-error@+1 {{expected ')'}}
+// CHECK-NEXT:             |                                           ^
+// expected-error@+1 {{invalid parameter of RootConstants}}
 [RootSignature(MultiLineRootSignature)]
 void bad_root_signature_6() {}
 
 // expected-error@+1 {{expected end of stream}}
 [RootSignature("RootFlags() RootConstants(b0, num32BitConstants = 1))")]
 void bad_root_signature_7() {}
+
+// expected-error@+1 {{invalid parameter of RootConstants}}
+[RootSignature("RootConstants(b0, num32BitConstantsTypo = 1))")]
+void bad_root_signature_8() {}
+
+// expected-error@+1 {{invalid parameter of UAV}}
+[RootSignature("UAV(b3")]
+void bad_root_signature_9() {}
+
+// expected-error@+1 {{invalid parameter of SRV}}
+[RootSignature("DescriptorTable(SRV(s1, invalid))")]
+void bad_root_signature_10() {}
+
+// expected-error@+1 {{invalid parameter of DescriptorTable}}
+[RootSignature("DescriptorTable(invalid))")]
+void bad_root_signature_11() {}

clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp

@@ -865,7 +865,7 @@ TEST_F(ParseHLSLRootSignatureTest, InvalidParseInvalidTokenTest) {
                                    Signature, *PP);
 
   // Test correct diagnostic produced - invalid token
-  Consumer->setExpected(diag::err_expected);
+  Consumer->setExpected(diag::err_hlsl_invalid_param);
   ASSERT_TRUE(Parser.parse());
 
   ASSERT_TRUE(Consumer->isSatisfied());
@@ -886,7 +886,7 @@ TEST_F(ParseHLSLRootSignatureTest, InvalidParseUnexpectedEndOfStreamTest) {
   hlsl::RootSignatureParser Parser(RootSignatureVersion::V1_1, Elements,
                                    Signature, *PP);
 
-  // Test correct diagnostic produced - end of stream
+  // Test correct diagnostic produced - expected '(' after DescriptorTable
   Consumer->setExpected(diag::err_expected_after);
 
   ASSERT_TRUE(Parser.parse());