[HLSL][RootSignature] Allow for multiple parsing errors in RootSignatureParser

[inbelic]

This pr implements returning multiple parsing errors at the granularity of a RootElement

This is achieved by adding a new interface onto RootSignatureParser, namely, skipUntilExpectedToken. This will be used to consume all the intermediate tokens between when an error has occurred and when the next RootElement begins.

At this granularity, the implementation is somewhat straight forward, as we can just implement this skip function when we return from a parse[RootElement] method and continue in the main parse loop. With the exception that the parseDescriptorTable will also have to skip ahead to the next expected closing ')'.

If we want to provide any finer granularity, then the skip logic becomes significantly more complicated. Skipping to the next root element will provide a good ratio of user experience benefit to complexity of implementation.

For more context see linked issue.

• Updates HLSLRootSignatureParser with a skipUntilExpectedToken interface
• Updates the parse loops to use the skip interface when an error is found on parsing a root element
• Updates parseDescriptorTable to skip ahead to the next ')' if it was inside a clause
• Adds test-case to demonstrate multiple error being reported

Resolves: #145818

[llvmbot]

@llvm/pr-subscribers-hlsl

@llvm/pr-subscribers-clang

Author: Finn Plummer (inbelic)

Changes

This pr implements returning multiple parsing errors at the granularity of a RootElement

This is achieved by adding a new interface onto RootSignatureParser, namely, skipUntilExpectedToken. This will be used to consume all the intermediate tokens between when an error has occurred and when the next RootElement begins.

At this granularity, the implementation is quite straight forward, as we can just implement this skip function when we return from a parse[RootElement] method and continue in the main parse loop. Then continue the loop.

If we want to provide any finer granularity, then the skip logic becomes significantly more complicated. Skipping to the next root element will provide a good ratio of user experience benefit to complexity of implementation.

For more context see linked issue.

• Updates HLSLRootSignatureParser with a skipUntilExpectedToken interface
• Updates the parse loops to use the skip interface when an error is found on parsing a root element
• Adds test-case to demonstrate multiple error being reported

Resolves: #145818

---

Full diff: https://github.com/llvm/llvm-project/pull/147832.diff

3 Files Affected:

• (modified) clang/include/clang/Parse/ParseHLSLRootSignature.h (+7)
• (modified) clang/lib/Parse/ParseHLSLRootSignature.cpp (+52-19)
• (modified) clang/test/SemaHLSL/RootSignature-err.hlsl (+22)

diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index 404bccea10c99..09602602224e9 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/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
diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index 87de627a98fb7..8d0847fb770e3 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/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());
diff --git a/clang/test/SemaHLSL/RootSignature-err.hlsl b/clang/test/SemaHLSL/RootSignature-err.hlsl
index b69807132e795..439dc265645f8 100644
--- a/clang/test/SemaHLSL/RootSignature-err.hlsl
+++ b/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() {}