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@+7 {{expected integer literal after '='}} +// 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() {} + +#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@+4 {{invalid parameter of CBV}} +// expected-error@+3 {{invalid parameter of UAV}} +// expected-error@+2 {{invalid parameter of StaticSampler}} +// expected-error@+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@+2 {{invalid parameter of UAV}} +// expected-error@+1 {{invalid parameter of CBV}} +[RootSignature(TestTableScope)] +void recover_scope_errors() {} + // Basic validation of register value and space // expected-error@+2 {{value must be in the range [0, 4294967294]}} @@ -138,4 +189,4 @@ void basic_validation_5() {} // expected-error@+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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
