[HLSL][RootSignature] Implement multiple diagnostics in RootSignatureParser

Author: inbelic

clang/include/clang/Parse/ParseHLSLRootSignature.h

@@ -198,6 +198,13 @@ class RootSignatureParser {
   bool tryConsumeExpectedToken(RootSignatureToken::Kind Expected);
   bool tryConsumeExpectedToken(ArrayRef<RootSignatureToken::Kind> Expected);
 
+  /// Consume tokens until the expected token has been peeked to be next
+  /// or we have reached the end of the stream. Note that this means the
+  /// expected token will be the next token not CurToken.
+  ///
+  /// Returns true if it found a token of the given type.
+  bool skipUntilExpectedToken(ArrayRef<RootSignatureToken::Kind> Expected);
+
   /// Convert the token's offset in the signature string to its SourceLocation
   ///
   /// This allows to currently retrieve the location for multi-token

clang/lib/Parse/ParseHLSLRootSignature.cpp

@@ -17,6 +17,15 @@ namespace hlsl {
 
 using TokenKind = RootSignatureToken::Kind;
 
+static const TokenKind RootElementKeywords[] = {
+    TokenKind::kw_RootFlags,
+    TokenKind::kw_CBV,
+    TokenKind::kw_UAV,
+    TokenKind::kw_SRV,
+    TokenKind::kw_DescriptorTable,
+    TokenKind::kw_StaticSampler,
+};
+
 RootSignatureParser::RootSignatureParser(
     llvm::dxbc::RootSignatureVersion Version,
     SmallVector<RootSignatureElement> &Elements, StringLiteral *Signature,
@@ -27,51 +36,63 @@ RootSignatureParser::RootSignatureParser(
 bool RootSignatureParser::parse() {
   // Iterate as many RootSignatureElements as possible, until we hit the
   // end of the stream
+  bool HadError = false;
   while (!peekExpectedToken(TokenKind::end_of_stream)) {
+    bool HadLocalError = false;
     if (tryConsumeExpectedToken(TokenKind::kw_RootFlags)) {
       SourceLocation ElementLoc = getTokenLocation(CurToken);
       auto Flags = parseRootFlags();
-      if (!Flags.has_value())
-        return true;
-      Elements.emplace_back(RootSignatureElement(ElementLoc, *Flags));
+      if (Flags.has_value())
+        Elements.emplace_back(RootSignatureElement(ElementLoc, *Flags));
+      else
+        HadLocalError = true;
     } else if (tryConsumeExpectedToken(TokenKind::kw_RootConstants)) {
       SourceLocation ElementLoc = getTokenLocation(CurToken);
       auto Constants = parseRootConstants();
-      if (!Constants.has_value())
-        return true;
-      Elements.emplace_back(RootSignatureElement(ElementLoc, *Constants));
+      if (Constants.has_value())
+        Elements.emplace_back(RootSignatureElement(ElementLoc, *Constants));
+      else
+        HadLocalError = true;
     } else if (tryConsumeExpectedToken(TokenKind::kw_DescriptorTable)) {
       SourceLocation ElementLoc = getTokenLocation(CurToken);
       auto Table = parseDescriptorTable();
-      if (!Table.has_value())
-        return true;
-      Elements.emplace_back(RootSignatureElement(ElementLoc, *Table));
+      if (Table.has_value())
+        Elements.emplace_back(RootSignatureElement(ElementLoc, *Table));
+      else
+        HadLocalError = true;
     } else if (tryConsumeExpectedToken(
                    {TokenKind::kw_CBV, TokenKind::kw_SRV, TokenKind::kw_UAV})) {
       SourceLocation ElementLoc = getTokenLocation(CurToken);
       auto Descriptor = parseRootDescriptor();
-      if (!Descriptor.has_value())
-        return true;
-      Elements.emplace_back(RootSignatureElement(ElementLoc, *Descriptor));
+      if (Descriptor.has_value())
+        Elements.emplace_back(RootSignatureElement(ElementLoc, *Descriptor));
+      else
+        HadLocalError = true;
     } else if (tryConsumeExpectedToken(TokenKind::kw_StaticSampler)) {
       SourceLocation ElementLoc = getTokenLocation(CurToken);
       auto Sampler = parseStaticSampler();
-      if (!Sampler.has_value())
-        return true;
-      Elements.emplace_back(RootSignatureElement(ElementLoc, *Sampler));
+      if (Sampler.has_value())
+        Elements.emplace_back(RootSignatureElement(ElementLoc, *Sampler));
+      else
+        HadLocalError = true;
     } else {
+      HadLocalError = true;
       consumeNextToken(); // let diagnostic be at the start of invalid token
       reportDiag(diag::err_hlsl_invalid_token)
           << /*parameter=*/0 << /*param of*/ TokenKind::kw_RootSignature;
-      return true;
     }
 
-    // ',' denotes another element, otherwise, expected to be at end of stream
-    if (!tryConsumeExpectedToken(TokenKind::pu_comma))
+    if (HadLocalError) {
+      HadError = true;
+      skipUntilExpectedToken(RootElementKeywords);
+    } else if (!tryConsumeExpectedToken(TokenKind::pu_comma)) {
+      // ',' denotes another element, otherwise, expected to be at end of stream
       break;
+    }
   }
 
-  return consumeExpectedToken(TokenKind::end_of_stream,
+  return HadError ||
+         consumeExpectedToken(TokenKind::end_of_stream,
                               diag::err_expected_either, TokenKind::pu_comma);
 }
 
@@ -1371,6 +1392,18 @@ bool RootSignatureParser::tryConsumeExpectedToken(
   return true;
 }
 
+bool RootSignatureParser::skipUntilExpectedToken(
+    ArrayRef<TokenKind> AnyExpected) {
+
+  while (!peekExpectedToken(AnyExpected)) {
+    if (peekExpectedToken(TokenKind::end_of_stream))
+      return false;
+    consumeNextToken();
+  }
+
+  return true;
+}
+
 SourceLocation RootSignatureParser::getTokenLocation(RootSignatureToken Tok) {
   return Signature->getLocationOfByte(Tok.LocOffset, PP.getSourceManager(),
                                       PP.getLangOpts(), PP.getTargetInfo());

clang/test/SemaHLSL/RootSignature-err.hlsl

@@ -103,3 +103,25 @@ void bad_root_signature_22() {}
 // expected-error@+1 {{invalid value of RootFlags}}
 [RootSignature("RootFlags(local_root_signature | root_flag_typo)")]
 void bad_root_signature_23() {}
+
+#define DemoMultipleErrorsRootSignature \
+  "CBV(b0, invalid)," \
+  "StaticSampler()" \
+  "DescriptorTable(" \
+  "  visibility = SHADER_VISIBILITY_ALL," \
+  "  visibility = SHADER_VISIBILITY_DOMAIN," \
+  ")," \
+  "SRV(t0, space = 28947298374912374098172)" \
+  "UAV(u0, flags = 3)" \
+  "DescriptorTable(Sampler(s0 flags = DATA_VOLATILE))," \
+  "CBV(b0),,"
+
+// expected-error@+7 {{invalid parameter of CBV}}
+// expected-error@+6 {{did not specify mandatory parameter 's register'}}
+// expected-error@+5 {{specified the same parameter 'visibility' multiple times}}
+// expected-error@+4 {{integer literal is too large to be represented as a 32-bit signed integer type}}
+// expected-error@+3 {{flag value is neither a literal 0 nor a named value}}
+// expected-error@+2 {{expected ')' or ','}}
+// expected-error@+1 {{invalid parameter of RootSignature}}
+[RootSignature(DemoMultipleErrorsRootSignature)]
+void multiple_errors() {}