[clang] a6494a3 - [HLSL][RootSignature] Allow for multiple parsing errors in `RootSignatureParser` (#147832)
via cfe-commits
cfe-commits at lists.llvm.org
Sun Jul 13 22:22:49 PDT 2025
Author: Finn Plummer
Date: 2025-07-13T22:22:46-07:00
New Revision: a6494a3bbf0620ec472d44db4e79c4c508618a61
URL: https://github.com/llvm/llvm-project/commit/a6494a3bbf0620ec472d44db4e79c4c508618a61
DIFF: https://github.com/llvm/llvm-project/commit/a6494a3bbf0620ec472d44db4e79c4c508618a61.diff
LOG: [HLSL][RootSignature] Allow for multiple parsing errors in `RootSignatureParser` (#147832)
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` and `skipUntilClosedParen`
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: https://github.com/llvm/llvm-project/issues/145818
Added:
Modified:
clang/include/clang/Parse/ParseHLSLRootSignature.h
clang/lib/Parse/ParseHLSLRootSignature.cpp
clang/test/SemaHLSL/RootSignature-err.hlsl
Removed:
################################################################################
diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index 404bccea10c99..ad66a26798847 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -198,6 +198,21 @@ 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(RootSignatureToken::Kind Expected);
+ bool skipUntilExpectedToken(ArrayRef<RootSignatureToken::Kind> Expected);
+
+ /// Consume tokens until we reach a closing right paren, ')', or, until we
+ /// have reached the end of the stream. This will place the current token
+ /// to be the end of stream or the right paren.
+ ///
+ /// Returns true if it is closed before the end of stream.
+ bool skipUntilClosedParens(uint32_t NumParens = 1);
+
/// 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 870cb88f40963..db9ed8373d01d 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,76 @@ 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)) {
if (tryConsumeExpectedToken(TokenKind::kw_RootFlags)) {
SourceLocation ElementLoc = getTokenLocation(CurToken);
auto Flags = parseRootFlags();
- if (!Flags.has_value())
- return true;
+ if (!Flags.has_value()) {
+ HadError = true;
+ skipUntilExpectedToken(RootElementKeywords);
+ continue;
+ }
+
Elements.emplace_back(ElementLoc, *Flags);
} else if (tryConsumeExpectedToken(TokenKind::kw_RootConstants)) {
SourceLocation ElementLoc = getTokenLocation(CurToken);
auto Constants = parseRootConstants();
- if (!Constants.has_value())
- return true;
+ if (!Constants.has_value()) {
+ HadError = true;
+ skipUntilExpectedToken(RootElementKeywords);
+ continue;
+ }
Elements.emplace_back(ElementLoc, *Constants);
} else if (tryConsumeExpectedToken(TokenKind::kw_DescriptorTable)) {
SourceLocation ElementLoc = getTokenLocation(CurToken);
auto Table = parseDescriptorTable();
- if (!Table.has_value())
- return true;
+ if (!Table.has_value()) {
+ HadError = true;
+ // We are within a DescriptorTable, we will do our best to recover
+ // by skipping until we encounter the expected closing ')'.
+ skipUntilClosedParens();
+ consumeNextToken();
+ skipUntilExpectedToken(RootElementKeywords);
+ continue;
+ }
Elements.emplace_back(ElementLoc, *Table);
} 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;
+ if (!Descriptor.has_value()) {
+ HadError = true;
+ skipUntilExpectedToken(RootElementKeywords);
+ continue;
+ }
Elements.emplace_back(ElementLoc, *Descriptor);
} else if (tryConsumeExpectedToken(TokenKind::kw_StaticSampler)) {
SourceLocation ElementLoc = getTokenLocation(CurToken);
auto Sampler = parseStaticSampler();
- if (!Sampler.has_value())
- return true;
+ if (!Sampler.has_value()) {
+ HadError = true;
+ skipUntilExpectedToken(RootElementKeywords);
+ continue;
+ }
Elements.emplace_back(ElementLoc, *Sampler);
} else {
+ HadError = 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;
+ skipUntilExpectedToken(RootElementKeywords);
+ continue;
}
- // ',' denotes another element, otherwise, expected to be at end of stream
- if (!tryConsumeExpectedToken(TokenKind::pu_comma))
+ 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);
}
@@ -262,8 +296,13 @@ std::optional<DescriptorTable> RootSignatureParser::parseDescriptorTable() {
// DescriptorTableClause - CBV, SRV, UAV, or Sampler
SourceLocation ElementLoc = getTokenLocation(CurToken);
auto Clause = parseDescriptorTableClause();
- if (!Clause.has_value())
+ if (!Clause.has_value()) {
+ // We are within a DescriptorTableClause, we will do our best to recover
+ // by skipping until we encounter the expected closing ')'
+ skipUntilExpectedToken(TokenKind::pu_r_paren);
+ consumeNextToken();
return std::nullopt;
+ }
Elements.emplace_back(ElementLoc, *Clause);
Table.NumClauses++;
} else if (tryConsumeExpectedToken(TokenKind::kw_visibility)) {
@@ -1371,6 +1410,40 @@ bool RootSignatureParser::tryConsumeExpectedToken(
return true;
}
+bool RootSignatureParser::skipUntilExpectedToken(TokenKind Expected) {
+ return skipUntilExpectedToken(ArrayRef{Expected});
+}
+
+bool RootSignatureParser::skipUntilExpectedToken(
+ ArrayRef<TokenKind> AnyExpected) {
+
+ while (!peekExpectedToken(AnyExpected)) {
+ if (peekExpectedToken(TokenKind::end_of_stream))
+ return false;
+ consumeNextToken();
+ }
+
+ return true;
+}
+
+bool RootSignatureParser::skipUntilClosedParens(uint32_t NumParens) {
+ TokenKind ParenKinds[] = {
+ TokenKind::pu_l_paren,
+ TokenKind::pu_r_paren,
+ };
+ while (skipUntilExpectedToken(ParenKinds)) {
+ consumeNextToken();
+ if (CurToken.TokKind == TokenKind::pu_r_paren)
+ NumParens--;
+ else
+ NumParens++;
+ if (NumParens == 0)
+ return true;
+ }
+
+ return false;
+}
+
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 e517274d937b0..ccfa093baeb87 100644
--- a/clang/test/SemaHLSL/RootSignature-err.hlsl
+++ b/clang/test/SemaHLSL/RootSignature-err.hlsl
@@ -104,6 +104,57 @@ void bad_root_signature_22() {}
[RootSignature("RootFlags(local_root_signature | root_flag_typo)")]
void bad_root_signature_23() {}
+#define DemoMultipleErrorsRootSignature \
+ "CBV(b0, space = 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 at +7 {{expected integer literal after '='}}
+// expected-error at +6 {{did not specify mandatory parameter 's register'}}
+// expected-error at +5 {{specified the same parameter 'visibility' multiple times}}
+// expected-error at +4 {{integer literal is too large to be represented as a 32-bit signed integer type}}
+// expected-error at +3 {{flag value is neither a literal 0 nor a named value}}
+// expected-error at +2 {{expected ')' or ','}}
+// expected-error at +1 {{invalid parameter of RootSignature}}
+[RootSignature(DemoMultipleErrorsRootSignature)]
+void multiple_errors() {}
+
+#define DemoGranularityRootSignature \
+ "CBV(b0, reported_diag, flags = skipped_diag)," \
+ "DescriptorTable( " \
+ " UAV(u0, reported_diag), " \
+ " SRV(t0, skipped_diag), " \
+ ")," \
+ "StaticSampler(s0, reported_diag, SRV(t0, reported_diag)" \
+ ""
+
+// expected-error at +4 {{invalid parameter of CBV}}
+// expected-error at +3 {{invalid parameter of UAV}}
+// expected-error at +2 {{invalid parameter of StaticSampler}}
+// expected-error at +1 {{invalid parameter of SRV}}
+[RootSignature(DemoGranularityRootSignature)]
+void granularity_errors() {}
+
+#define TestTableScope \
+ "DescriptorTable( " \
+ " UAV(u0, reported_diag), " \
+ " SRV(t0, skipped_diag), " \
+ " Sampler(s0, skipped_diag), " \
+ ")," \
+ "CBV(s0, reported_diag)"
+
+// expected-error at +2 {{invalid parameter of UAV}}
+// expected-error at +1 {{invalid parameter of CBV}}
+[RootSignature(TestTableScope)]
+void recover_scope_errors() {}
+
// Basic validation of register value and space
// expected-error at +2 {{value must be in the range [0, 4294967294]}}
@@ -138,4 +189,4 @@ void basic_validation_5() {}
// expected-error at +1 {{value must be in the range [-16.00, 15.99]}}
[RootSignature("StaticSampler(s0, mipLODBias = 15.990001)")]
-void basic_validation_6() {}
+void basic_validation_6() {}
\ No newline at end of file
More information about the cfe-commits
mailing list