improve diagnostics for root elements

Author: inbelic

clang/include/clang/Basic/DiagnosticParseKinds.td

@@ -1857,6 +1857,7 @@ def err_hlsl_virtual_inheritance
     : Error<"virtual inheritance is unsupported in HLSL">;
 
 // HLSL Root Signature Parser Diagnostics
+def err_hlsl_rootsig_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/lib/Parse/ParseHLSLRootSignature.cpp

@@ -25,54 +25,60 @@ RootSignatureParser::RootSignatureParser(
       Lexer(Signature->getString()), PP(PP), CurToken(0) {}
 
 bool RootSignatureParser::parse() {
-  // Iterate as many RootSignatureElements as possible
-  do {
+  // Iterate as many RootSignatureElements as possible, until we hit the
+  // end of the stream
+  while (!peekExpectedToken(TokenKind::end_of_stream)) {
     std::optional<RootSignatureElement> Element = std::nullopt;
     if (tryConsumeExpectedToken(TokenKind::kw_RootFlags)) {
+      // RootFlags
       SourceLocation ElementLoc = getTokenLocation(CurToken);
       auto Flags = parseRootFlags();
       if (!Flags.has_value())
         return true;
       Element = RootSignatureElement(ElementLoc, *Flags);
-    }
-
-    if (tryConsumeExpectedToken(TokenKind::kw_RootConstants)) {
+    } else if (tryConsumeExpectedToken(TokenKind::kw_RootConstants)) {
+      // RootConstants
       SourceLocation ElementLoc = getTokenLocation(CurToken);
       auto Constants = parseRootConstants();
       if (!Constants.has_value())
         return true;
       Element = RootSignatureElement(ElementLoc, *Constants);
-    }
-
-    if (tryConsumeExpectedToken(TokenKind::kw_DescriptorTable)) {
+    } else if (tryConsumeExpectedToken(TokenKind::kw_DescriptorTable)) {
+      // DescriptorTable
       SourceLocation ElementLoc = getTokenLocation(CurToken);
       auto Table = parseDescriptorTable();
       if (!Table.has_value())
         return true;
       Element = RootSignatureElement(ElementLoc, *Table);
-    }
-
-    if (tryConsumeExpectedToken(
-            {TokenKind::kw_CBV, TokenKind::kw_SRV, TokenKind::kw_UAV})) {
+    } else if (tryConsumeExpectedToken(
+                   {TokenKind::kw_CBV, TokenKind::kw_SRV, TokenKind::kw_UAV})) {
+      // RootDescriptor - CBV, SRV, UAV
       SourceLocation ElementLoc = getTokenLocation(CurToken);
       auto Descriptor = parseRootDescriptor();
       if (!Descriptor.has_value())
         return true;
       Element = RootSignatureElement(ElementLoc, *Descriptor);
-    }
-
-    if (tryConsumeExpectedToken(TokenKind::kw_StaticSampler)) {
+    } else if (tryConsumeExpectedToken(TokenKind::kw_StaticSampler)) {
+      // StaticSampler
       SourceLocation ElementLoc = getTokenLocation(CurToken);
       auto Sampler = parseStaticSampler();
       if (!Sampler.has_value())
         return true;
       Element = RootSignatureElement(ElementLoc, *Sampler);
+    } else {
+      consumeNextToken(); // position to start of invalid token
+      reportDiag(diag::err_hlsl_rootsig_invalid_param)
+          << /*param of=*/TokenKind::kw_RootSignature;
+      return true;
     }
 
     if (Element.has_value())
       Elements.push_back(*Element);
 
-  } while (tryConsumeExpectedToken(TokenKind::pu_comma));
+    // ',' denotes another element, otherwise, expected to be at end of stream
+    if (!tryConsumeExpectedToken(TokenKind::pu_comma))
+      break;
+  }
 
   return consumeExpectedToken(TokenKind::end_of_stream);
 }
@@ -251,9 +257,7 @@ std::optional<DescriptorTable> RootSignatureParser::parseDescriptorTable() {
         return std::nullopt;
       Elements.push_back(RootSignatureElement(ElementLoc, *Clause));
       Table.NumClauses++;
-    }
-
-    if (tryConsumeExpectedToken(TokenKind::kw_visibility)) {
+    } else if (tryConsumeExpectedToken(TokenKind::kw_visibility)) {
       if (Visibility.has_value()) {
         reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind;
         return std::nullopt;

clang/test/SemaHLSL/RootSignature-err.hlsl

@@ -17,11 +17,11 @@ 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}}
+[RootSignature("DescriptorTable(), invalid")] // expected-error {{invalid parameter of RootSignature}}
 void bad_root_signature_4() {}
 
-// expected-error@+1 {{expected ')'}}
-[RootSignature("RootConstants(b0, num32BitConstants = 1, invalid)")]
+// expected-error@+1 {{expected end of stream}}
+[RootSignature("RootFlags() RootConstants(b0, num32BitConstants = 1))")]
 void bad_root_signature_5() {}
 
 #define MultiLineRootSignature \

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_rootsig_invalid_param);
   ASSERT_TRUE(Parser.parse());
 
   ASSERT_TRUE(Consumer->isSatisfied());
@@ -1239,4 +1239,29 @@ TEST_F(ParseHLSLRootSignatureTest, InvalidNonZeroFlagsTest) {
   ASSERT_TRUE(Consumer->isSatisfied());
 }
 
+TEST_F(ParseHLSLRootSignatureTest, InvalidRootElementMissingCommaTest) {
+  // This test will check that an error is produced when there is a missing
+  // comma between parameters
+  const llvm::StringLiteral Source = R"cc(
+    RootFlags()
+    RootConstants(num32BitConstants = 1, b0)
+  )cc";
+
+  auto Ctx = createMinimalASTContext();
+  StringLiteral *Signature = wrapSource(Ctx, Source);
+
+  TrivialModuleLoader ModLoader;
+  auto PP = createPP(Source, ModLoader);
+
+  SmallVector<RootSignatureElement> Elements;
+  hlsl::RootSignatureParser Parser(RootSignatureVersion::V1_1, Elements,
+                                   Signature, *PP);
+
+  // Test correct diagnostic produced
+  Consumer->setExpected(diag::err_expected);
+  ASSERT_TRUE(Parser.parse());
+
+  ASSERT_TRUE(Consumer->isSatisfied());
+}
+
 } // anonymous namespace