https://github.com/inbelic updated https://github.com/llvm/llvm-project/pull/147350
>From b8644c7d9a44f9480cdbe0b3c46f0899cdcffc28 Mon Sep 17 00:00:00 2001 From: Finn Plummer <canadienf...@gmail.com> Date: Sat, 5 Jul 2025 01:41:26 +0000 Subject: [PATCH 1/7] fix for root elements --- clang/lib/Parse/ParseHLSLRootSignature.cpp | 33 ++++++++----------- clang/test/SemaHLSL/RootSignature-err.hlsl | 4 +++ .../Parse/ParseHLSLRootSignatureTest.cpp | 25 ++++++++++++++ 3 files changed, 43 insertions(+), 19 deletions(-) diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index cf86c62f3b671..7956f1f7fdbfa 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -25,44 +25,41 @@ RootSignatureParser::RootSignatureParser( Lexer(Signature->getString()), PP(PP), CurToken(0) {} bool RootSignatureParser::parse() { - // Iterate as many RootElements as possible - do { + // Iterate as many RootSignatureElements as possible, until we hit the + // end of the stream + while (!peekExpectedToken(TokenKind::end_of_stream)) { if (tryConsumeExpectedToken(TokenKind::kw_RootFlags)) { auto Flags = parseRootFlags(); if (!Flags.has_value()) return true; Elements.push_back(*Flags); - } - - if (tryConsumeExpectedToken(TokenKind::kw_RootConstants)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_RootConstants)) { auto Constants = parseRootConstants(); if (!Constants.has_value()) return true; Elements.push_back(*Constants); - } - - if (tryConsumeExpectedToken(TokenKind::kw_DescriptorTable)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_DescriptorTable)) { auto Table = parseDescriptorTable(); if (!Table.has_value()) return true; Elements.push_back(*Table); - } - - if (tryConsumeExpectedToken( - {TokenKind::kw_CBV, TokenKind::kw_SRV, TokenKind::kw_UAV})) { + } else if (tryConsumeExpectedToken( + {TokenKind::kw_CBV, TokenKind::kw_SRV, TokenKind::kw_UAV})) { auto Descriptor = parseRootDescriptor(); if (!Descriptor.has_value()) return true; Elements.push_back(*Descriptor); - } - - if (tryConsumeExpectedToken(TokenKind::kw_StaticSampler)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_StaticSampler)) { auto Sampler = parseStaticSampler(); if (!Sampler.has_value()) return true; Elements.push_back(*Sampler); } - } 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, diag::err_hlsl_unexpected_end_of_params, @@ -252,9 +249,7 @@ std::optional<DescriptorTable> RootSignatureParser::parseDescriptorTable() { return std::nullopt; Elements.push_back(*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; diff --git a/clang/test/SemaHLSL/RootSignature-err.hlsl b/clang/test/SemaHLSL/RootSignature-err.hlsl index 118fc38daf3f2..50807d611e91a 100644 --- a/clang/test/SemaHLSL/RootSignature-err.hlsl +++ b/clang/test/SemaHLSL/RootSignature-err.hlsl @@ -34,3 +34,7 @@ void bad_root_signature_5() {} // expected-error@+1 {{expected ')' to denote end of parameters, or, another valid parameter of RootConstants}} [RootSignature(MultiLineRootSignature)] void bad_root_signature_6() {} + +// expected-error@+1 {{expected end of stream to denote end of parameters, or, another valid parameter of RootSignature}} +[RootSignature("RootFlags() RootConstants(b0, num32BitConstants = 1))")] +void bad_root_signature_7() {} diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp index ff1697f1bbb9a..fd452b29ebf00 100644 --- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp +++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp @@ -1238,4 +1238,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<RootElement> Elements; + hlsl::RootSignatureParser Parser(RootSignatureVersion::V1_1, Elements, + Signature, *PP); + + // Test correct diagnostic produced + Consumer->setExpected(diag::err_hlsl_unexpected_end_of_params); + ASSERT_TRUE(Parser.parse()); + + ASSERT_TRUE(Consumer->isSatisfied()); +} + } // anonymous namespace >From 24ed530a19fa49bb6363ec0cf2325a85400030e6 Mon Sep 17 00:00:00 2001 From: Finn Plummer <canadienf...@gmail.com> Date: Sat, 5 Jul 2025 01:52:21 +0000 Subject: [PATCH 2/7] fix for descriptor table --- clang/lib/Parse/ParseHLSLRootSignature.cpp | 18 ++++++++----- .../Parse/ParseHLSLRootSignatureTest.cpp | 27 +++++++++++++++++++ 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index 7956f1f7fdbfa..23355567e871b 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -240,16 +240,18 @@ std::optional<DescriptorTable> RootSignatureParser::parseDescriptorTable() { DescriptorTable Table; std::optional<llvm::dxbc::ShaderVisibility> Visibility; - // Iterate as many Clauses as possible - do { + // Iterate as many Clauses as possible, until we hit ')' + while (!peekExpectedToken(TokenKind::pu_r_paren)) { if (tryConsumeExpectedToken({TokenKind::kw_CBV, TokenKind::kw_SRV, TokenKind::kw_UAV, TokenKind::kw_Sampler})) { + // DescriptorTableClause - CBV, SRV, UAV, or Sampler auto Clause = parseDescriptorTableClause(); if (!Clause.has_value()) return std::nullopt; Elements.push_back(*Clause); Table.NumClauses++; } else if (tryConsumeExpectedToken(TokenKind::kw_visibility)) { + // visibility = SHADER_VISIBILITY if (Visibility.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -262,17 +264,21 @@ std::optional<DescriptorTable> RootSignatureParser::parseDescriptorTable() { if (!Visibility.has_value()) return std::nullopt; } - } while (tryConsumeExpectedToken(TokenKind::pu_comma)); - // Fill in optional visibility - if (Visibility.has_value()) - Table.Visibility = Visibility.value(); + // ',' denotes another element, otherwise, expected to be at ')' + if (!tryConsumeExpectedToken(TokenKind::pu_comma)) + break; + } if (consumeExpectedToken(TokenKind::pu_r_paren, diag::err_hlsl_unexpected_end_of_params, /*param of=*/TokenKind::kw_DescriptorTable)) return std::nullopt; + // Fill in optional visibility + if (Visibility.has_value()) + Table.Visibility = Visibility.value(); + return Table; } diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp index fd452b29ebf00..5da2aea0a63c0 100644 --- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp +++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp @@ -1263,4 +1263,31 @@ TEST_F(ParseHLSLRootSignatureTest, InvalidRootElementMissingCommaTest) { ASSERT_TRUE(Consumer->isSatisfied()); } +TEST_F(ParseHLSLRootSignatureTest, InvalidDescriptorTableMissingCommaTest) { + // This test will check that an error is produced when there is a missing + // comma between parameters + const llvm::StringLiteral Source = R"cc( + DescriptorTable( + CBV(b0) + visibility = SHADER_VISIBILITY_ALL + ) + )cc"; + + auto Ctx = createMinimalASTContext(); + StringLiteral *Signature = wrapSource(Ctx, Source); + + TrivialModuleLoader ModLoader; + auto PP = createPP(Source, ModLoader); + + SmallVector<RootElement> Elements; + hlsl::RootSignatureParser Parser(RootSignatureVersion::V1_1, Elements, + Signature, *PP); + + // Test correct diagnostic produced + Consumer->setExpected(diag::err_hlsl_unexpected_end_of_params); + ASSERT_TRUE(Parser.parse()); + + ASSERT_TRUE(Consumer->isSatisfied()); +} + } // anonymous namespace >From 4f210fac40caf9531320bfb5dc3d952a3ed541c6 Mon Sep 17 00:00:00 2001 From: Finn Plummer <canadienf...@gmail.com> Date: Mon, 7 Jul 2025 16:30:59 +0000 Subject: [PATCH 3/7] fix for root constants --- clang/lib/Parse/ParseHLSLRootSignature.cpp | 38 +++++++++---------- .../Parse/ParseHLSLRootSignatureTest.cpp | 27 +++++++++++++ 2 files changed, 45 insertions(+), 20 deletions(-) diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index 23355567e871b..c73025613e819 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -136,6 +136,11 @@ std::optional<RootConstants> RootSignatureParser::parseRootConstants() { if (!Params.has_value()) return std::nullopt; + if (consumeExpectedToken(TokenKind::pu_r_paren, + diag::err_hlsl_unexpected_end_of_params, + /*param of=*/TokenKind::kw_RootConstants)) + return std::nullopt; + // Check mandatory parameters where provided if (!Params->Num32BitConstants.has_value()) { reportDiag(diag::err_hlsl_rootsig_missing_param) @@ -159,11 +164,6 @@ std::optional<RootConstants> RootSignatureParser::parseRootConstants() { if (Params->Space.has_value()) Constants.Space = Params->Space.value(); - if (consumeExpectedToken(TokenKind::pu_r_paren, - diag::err_hlsl_unexpected_end_of_params, - /*param of=*/TokenKind::kw_RootConstants)) - return std::nullopt; - return Constants; } @@ -429,9 +429,9 @@ RootSignatureParser::parseRootConstantParams() { "Expects to only be invoked starting at given token"); ParsedConstantParams Params; - do { - // `num32BitConstants` `=` POS_INT + while (!peekExpectedToken(TokenKind::pu_r_paren)) { if (tryConsumeExpectedToken(TokenKind::kw_num32BitConstants)) { + // `num32BitConstants` `=` POS_INT if (Params.Num32BitConstants.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -444,10 +444,8 @@ RootSignatureParser::parseRootConstantParams() { if (!Num32BitConstants.has_value()) return std::nullopt; Params.Num32BitConstants = Num32BitConstants; - } - - // `b` POS_INT - if (tryConsumeExpectedToken(TokenKind::bReg)) { + } else if (tryConsumeExpectedToken(TokenKind::bReg)) { + // `b` POS_INT if (Params.Reg.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -456,10 +454,8 @@ RootSignatureParser::parseRootConstantParams() { if (!Reg.has_value()) return std::nullopt; Params.Reg = Reg; - } - - // `space` `=` POS_INT - if (tryConsumeExpectedToken(TokenKind::kw_space)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_space)) { + // `space` `=` POS_INT if (Params.Space.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -472,10 +468,8 @@ RootSignatureParser::parseRootConstantParams() { if (!Space.has_value()) return std::nullopt; Params.Space = Space; - } - - // `visibility` `=` SHADER_VISIBILITY - if (tryConsumeExpectedToken(TokenKind::kw_visibility)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_visibility)) { + // `visibility` `=` SHADER_VISIBILITY if (Params.Visibility.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -489,7 +483,11 @@ RootSignatureParser::parseRootConstantParams() { return std::nullopt; Params.Visibility = Visibility; } - } while (tryConsumeExpectedToken(TokenKind::pu_comma)); + + // ',' denotes another element, otherwise, expected to be at ')' + if (!tryConsumeExpectedToken(TokenKind::pu_comma)) + break; + } return Params; } diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp index 5da2aea0a63c0..8d9e8a120b6fc 100644 --- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp +++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp @@ -1290,4 +1290,31 @@ TEST_F(ParseHLSLRootSignatureTest, InvalidDescriptorTableMissingCommaTest) { ASSERT_TRUE(Consumer->isSatisfied()); } +TEST_F(ParseHLSLRootSignatureTest, InvalidRootConstantParamsCommaTest) { + // This test will check that an error is produced when there is a missing + // comma between parameters + const llvm::StringLiteral Source = R"cc( + RootConstants( + num32BitConstants = 1 + b0 + ) + )cc"; + + auto Ctx = createMinimalASTContext(); + StringLiteral *Signature = wrapSource(Ctx, Source); + + TrivialModuleLoader ModLoader; + auto PP = createPP(Source, ModLoader); + + SmallVector<RootElement> Elements; + hlsl::RootSignatureParser Parser(RootSignatureVersion::V1_1, Elements, + Signature, *PP); + + // Test correct diagnostic produced + Consumer->setExpected(diag::err_hlsl_unexpected_end_of_params); + ASSERT_TRUE(Parser.parse()); + + ASSERT_TRUE(Consumer->isSatisfied()); +} + } // anonymous namespace >From 15844909e7f88d4fce582ebaa97eb418ea55f6cf Mon Sep 17 00:00:00 2001 From: Finn Plummer <canadienf...@gmail.com> Date: Mon, 7 Jul 2025 16:54:53 +0000 Subject: [PATCH 4/7] fix for root descriptors --- .../clang/Parse/ParseHLSLRootSignature.h | 3 +- clang/lib/Parse/ParseHLSLRootSignature.cpp | 43 +++++++++---------- .../Parse/ParseHLSLRootSignatureTest.cpp | 27 ++++++++++++ 3 files changed, 50 insertions(+), 23 deletions(-) diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h index 007c3f7ba1e9c..0c6594e44d51b 100644 --- a/clang/include/clang/Parse/ParseHLSLRootSignature.h +++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h @@ -99,7 +99,8 @@ class RootSignatureParser { std::optional<llvm::dxbc::RootDescriptorFlags> Flags; }; std::optional<ParsedRootDescriptorParams> - parseRootDescriptorParams(RootSignatureToken::Kind RegType); + parseRootDescriptorParams(RootSignatureToken::Kind DescType, + RootSignatureToken::Kind RegType); struct ParsedClauseParams { std::optional<llvm::hlsl::rootsig::Register> Reg; diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index c73025613e819..15e01eed78f7c 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -199,10 +199,15 @@ std::optional<RootDescriptor> RootSignatureParser::parseRootDescriptor() { } Descriptor.setDefaultFlags(Version); - auto Params = parseRootDescriptorParams(ExpectedReg); + auto Params = parseRootDescriptorParams(DescriptorKind, ExpectedReg); if (!Params.has_value()) return std::nullopt; + if (consumeExpectedToken(TokenKind::pu_r_paren, + diag::err_hlsl_unexpected_end_of_params, + /*param of=*/DescriptorKind)) + return std::nullopt; + // Check mandatory parameters were provided if (!Params->Reg.has_value()) { reportDiag(diag::err_hlsl_rootsig_missing_param) << ExpectedReg; @@ -221,11 +226,6 @@ std::optional<RootDescriptor> RootSignatureParser::parseRootDescriptor() { if (Params->Flags.has_value()) Descriptor.Flags = Params->Flags.value(); - if (consumeExpectedToken(TokenKind::pu_r_paren, - diag::err_hlsl_unexpected_end_of_params, - /*param of=*/TokenKind::kw_RootConstants)) - return std::nullopt; - return Descriptor; } @@ -493,14 +493,15 @@ RootSignatureParser::parseRootConstantParams() { } std::optional<RootSignatureParser::ParsedRootDescriptorParams> -RootSignatureParser::parseRootDescriptorParams(TokenKind RegType) { +RootSignatureParser::parseRootDescriptorParams(TokenKind DescType, + TokenKind RegType) { assert(CurToken.TokKind == TokenKind::pu_l_paren && "Expects to only be invoked starting at given token"); ParsedRootDescriptorParams Params; - do { - // ( `b` | `t` | `u`) POS_INT + while (!peekExpectedToken(TokenKind::pu_r_paren)) { if (tryConsumeExpectedToken(RegType)) { + // ( `b` | `t` | `u`) POS_INT if (Params.Reg.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -509,10 +510,8 @@ RootSignatureParser::parseRootDescriptorParams(TokenKind RegType) { if (!Reg.has_value()) return std::nullopt; Params.Reg = Reg; - } - - // `space` `=` POS_INT - if (tryConsumeExpectedToken(TokenKind::kw_space)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_space)) { + // `space` `=` POS_INT if (Params.Space.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -525,10 +524,8 @@ RootSignatureParser::parseRootDescriptorParams(TokenKind RegType) { if (!Space.has_value()) return std::nullopt; Params.Space = Space; - } - - // `visibility` `=` SHADER_VISIBILITY - if (tryConsumeExpectedToken(TokenKind::kw_visibility)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_visibility)) { + // `visibility` `=` SHADER_VISIBILITY if (Params.Visibility.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -541,10 +538,8 @@ RootSignatureParser::parseRootDescriptorParams(TokenKind RegType) { if (!Visibility.has_value()) return std::nullopt; Params.Visibility = Visibility; - } - - // `flags` `=` ROOT_DESCRIPTOR_FLAGS - if (tryConsumeExpectedToken(TokenKind::kw_flags)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_flags)) { + // `flags` `=` ROOT_DESCRIPTOR_FLAGS if (Params.Flags.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -558,7 +553,11 @@ RootSignatureParser::parseRootDescriptorParams(TokenKind RegType) { return std::nullopt; Params.Flags = Flags; } - } while (tryConsumeExpectedToken(TokenKind::pu_comma)); + + // ',' denotes another element, otherwise, expected to be at ')' + if (!tryConsumeExpectedToken(TokenKind::pu_comma)) + break; + } return Params; } diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp index 8d9e8a120b6fc..c0c345b6ad58d 100644 --- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp +++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp @@ -1317,4 +1317,31 @@ TEST_F(ParseHLSLRootSignatureTest, InvalidRootConstantParamsCommaTest) { ASSERT_TRUE(Consumer->isSatisfied()); } +TEST_F(ParseHLSLRootSignatureTest, InvalidRootDescriptorParamsCommaTest) { + // This test will check that an error is produced when there is a missing + // comma between parameters + const llvm::StringLiteral Source = R"cc( + CBV( + b0 + flags = 0 + ) + )cc"; + + auto Ctx = createMinimalASTContext(); + StringLiteral *Signature = wrapSource(Ctx, Source); + + TrivialModuleLoader ModLoader; + auto PP = createPP(Source, ModLoader); + + SmallVector<RootElement> Elements; + hlsl::RootSignatureParser Parser(RootSignatureVersion::V1_1, Elements, + Signature, *PP); + + // Test correct diagnostic produced + Consumer->setExpected(diag::err_hlsl_unexpected_end_of_params); + ASSERT_TRUE(Parser.parse()); + + ASSERT_TRUE(Consumer->isSatisfied()); +} + } // anonymous namespace >From 8dae33e6b70ac79cc2b6be266c2324d49443b91c Mon Sep 17 00:00:00 2001 From: Finn Plummer <canadienf...@gmail.com> Date: Mon, 7 Jul 2025 17:12:31 +0000 Subject: [PATCH 5/7] fix for descriptor table clauses --- .../clang/Parse/ParseHLSLRootSignature.h | 3 +- clang/lib/Parse/ParseHLSLRootSignature.cpp | 48 +++++++++---------- .../Parse/ParseHLSLRootSignatureTest.cpp | 29 +++++++++++ 3 files changed, 53 insertions(+), 27 deletions(-) diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h index 0c6594e44d51b..fc71ad5e3722c 100644 --- a/clang/include/clang/Parse/ParseHLSLRootSignature.h +++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h @@ -110,7 +110,8 @@ class RootSignatureParser { std::optional<llvm::dxbc::DescriptorRangeFlags> Flags; }; std::optional<ParsedClauseParams> - parseDescriptorTableClauseParams(RootSignatureToken::Kind RegType); + parseDescriptorTableClauseParams(RootSignatureToken::Kind DescType, + RootSignatureToken::Kind RegType); struct ParsedStaticSamplerParams { std::optional<llvm::hlsl::rootsig::Register> Reg; diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index 15e01eed78f7c..9088da892f8dc 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -320,10 +320,15 @@ RootSignatureParser::parseDescriptorTableClause() { } Clause.setDefaultFlags(Version); - auto Params = parseDescriptorTableClauseParams(ExpectedReg); + auto Params = parseDescriptorTableClauseParams(ParamKind, ExpectedReg); if (!Params.has_value()) return std::nullopt; + if (consumeExpectedToken(TokenKind::pu_r_paren, + diag::err_hlsl_unexpected_end_of_params, + /*param of=*/ParamKind)) + return std::nullopt; + // Check mandatory parameters were provided if (!Params->Reg.has_value()) { reportDiag(diag::err_hlsl_rootsig_missing_param) << ExpectedReg; @@ -345,11 +350,6 @@ RootSignatureParser::parseDescriptorTableClause() { if (Params->Flags.has_value()) Clause.Flags = Params->Flags.value(); - if (consumeExpectedToken(TokenKind::pu_r_paren, - diag::err_hlsl_unexpected_end_of_params, - /*param of=*/ParamKind)) - return std::nullopt; - return Clause; } @@ -563,14 +563,15 @@ RootSignatureParser::parseRootDescriptorParams(TokenKind DescType, } std::optional<RootSignatureParser::ParsedClauseParams> -RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) { +RootSignatureParser::parseDescriptorTableClauseParams(TokenKind DescType, + TokenKind RegType) { assert(CurToken.TokKind == TokenKind::pu_l_paren && "Expects to only be invoked starting at given token"); ParsedClauseParams Params; - do { - // ( `b` | `t` | `u` | `s`) POS_INT + while (!peekExpectedToken(TokenKind::pu_r_paren)) { if (tryConsumeExpectedToken(RegType)) { + // ( `b` | `t` | `u` | `s`) POS_INT if (Params.Reg.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -579,10 +580,8 @@ RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) { if (!Reg.has_value()) return std::nullopt; Params.Reg = Reg; - } - - // `numDescriptors` `=` POS_INT | unbounded - if (tryConsumeExpectedToken(TokenKind::kw_numDescriptors)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_numDescriptors)) { + // `numDescriptors` `=` POS_INT | unbounded if (Params.NumDescriptors.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -601,10 +600,8 @@ RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) { } Params.NumDescriptors = NumDescriptors; - } - - // `space` `=` POS_INT - if (tryConsumeExpectedToken(TokenKind::kw_space)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_space)) { + // `space` `=` POS_INT if (Params.Space.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -617,10 +614,8 @@ RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) { if (!Space.has_value()) return std::nullopt; Params.Space = Space; - } - - // `offset` `=` POS_INT | DESCRIPTOR_RANGE_OFFSET_APPEND - if (tryConsumeExpectedToken(TokenKind::kw_offset)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_offset)) { + // `offset` `=` POS_INT | DESCRIPTOR_RANGE_OFFSET_APPEND if (Params.Offset.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -639,10 +634,8 @@ RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) { } Params.Offset = Offset; - } - - // `flags` `=` DESCRIPTOR_RANGE_FLAGS - if (tryConsumeExpectedToken(TokenKind::kw_flags)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_flags)) { + // `flags` `=` DESCRIPTOR_RANGE_FLAGS if (Params.Flags.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -657,7 +650,10 @@ RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) { Params.Flags = Flags; } - } while (tryConsumeExpectedToken(TokenKind::pu_comma)); + // ',' denotes another element, otherwise, expected to be at ')' + if (!tryConsumeExpectedToken(TokenKind::pu_comma)) + break; + } return Params; } diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp index c0c345b6ad58d..601ee74388edd 100644 --- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp +++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp @@ -1344,4 +1344,33 @@ TEST_F(ParseHLSLRootSignatureTest, InvalidRootDescriptorParamsCommaTest) { ASSERT_TRUE(Consumer->isSatisfied()); } +TEST_F(ParseHLSLRootSignatureTest, InvalidDescriptorClauseParamsCommaTest) { + // This test will check that an error is produced when there is a missing + // comma between parameters + const llvm::StringLiteral Source = R"cc( + DescriptorTable( + UAV( + u0 + flags = 0 + ) + ) + )cc"; + + auto Ctx = createMinimalASTContext(); + StringLiteral *Signature = wrapSource(Ctx, Source); + + TrivialModuleLoader ModLoader; + auto PP = createPP(Source, ModLoader); + + SmallVector<RootElement> Elements; + hlsl::RootSignatureParser Parser(RootSignatureVersion::V1_1, Elements, + Signature, *PP); + + // Test correct diagnostic produced + Consumer->setExpected(diag::err_hlsl_unexpected_end_of_params); + ASSERT_TRUE(Parser.parse()); + + ASSERT_TRUE(Consumer->isSatisfied()); +} + } // anonymous namespace >From 1643d251d01563fee60fc3ffc011de84ae408660 Mon Sep 17 00:00:00 2001 From: Finn Plummer <canadienf...@gmail.com> Date: Mon, 7 Jul 2025 17:18:09 +0000 Subject: [PATCH 6/7] fix for static samplers --- clang/lib/Parse/ParseHLSLRootSignature.cpp | 92 ++++++++----------- .../Parse/ParseHLSLRootSignatureTest.cpp | 27 ++++++ 2 files changed, 63 insertions(+), 56 deletions(-) diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index 9088da892f8dc..5cc1dc598334e 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -367,6 +367,11 @@ std::optional<StaticSampler> RootSignatureParser::parseStaticSampler() { if (!Params.has_value()) return std::nullopt; + if (consumeExpectedToken(TokenKind::pu_r_paren, + diag::err_hlsl_unexpected_end_of_params, + /*param of=*/TokenKind::kw_StaticSampler)) + return std::nullopt; + // Check mandatory parameters were provided if (!Params->Reg.has_value()) { reportDiag(diag::err_hlsl_rootsig_missing_param) << TokenKind::sReg; @@ -412,11 +417,6 @@ std::optional<StaticSampler> RootSignatureParser::parseStaticSampler() { if (Params->Visibility.has_value()) Sampler.Visibility = Params->Visibility.value(); - if (consumeExpectedToken(TokenKind::pu_r_paren, - diag::err_hlsl_unexpected_end_of_params, - /*param of=*/TokenKind::kw_StaticSampler)) - return std::nullopt; - return Sampler; } @@ -664,9 +664,9 @@ RootSignatureParser::parseStaticSamplerParams() { "Expects to only be invoked starting at given token"); ParsedStaticSamplerParams Params; - do { - // `s` POS_INT + while (!peekExpectedToken(TokenKind::pu_r_paren)) { if (tryConsumeExpectedToken(TokenKind::sReg)) { + // `s` POS_INT if (Params.Reg.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -675,10 +675,8 @@ RootSignatureParser::parseStaticSamplerParams() { if (!Reg.has_value()) return std::nullopt; Params.Reg = Reg; - } - - // `filter` `=` FILTER - if (tryConsumeExpectedToken(TokenKind::kw_filter)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_filter)) { + // `filter` `=` FILTER if (Params.Filter.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -691,10 +689,8 @@ RootSignatureParser::parseStaticSamplerParams() { if (!Filter.has_value()) return std::nullopt; Params.Filter = Filter; - } - - // `addressU` `=` TEXTURE_ADDRESS - if (tryConsumeExpectedToken(TokenKind::kw_addressU)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_addressU)) { + // `addressU` `=` TEXTURE_ADDRESS if (Params.AddressU.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -707,10 +703,8 @@ RootSignatureParser::parseStaticSamplerParams() { if (!AddressU.has_value()) return std::nullopt; Params.AddressU = AddressU; - } - - // `addressV` `=` TEXTURE_ADDRESS - if (tryConsumeExpectedToken(TokenKind::kw_addressV)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_addressV)) { + // `addressV` `=` TEXTURE_ADDRESS if (Params.AddressV.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -723,10 +717,8 @@ RootSignatureParser::parseStaticSamplerParams() { if (!AddressV.has_value()) return std::nullopt; Params.AddressV = AddressV; - } - - // `addressW` `=` TEXTURE_ADDRESS - if (tryConsumeExpectedToken(TokenKind::kw_addressW)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_addressW)) { + // `addressW` `=` TEXTURE_ADDRESS if (Params.AddressW.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -739,10 +731,8 @@ RootSignatureParser::parseStaticSamplerParams() { if (!AddressW.has_value()) return std::nullopt; Params.AddressW = AddressW; - } - - // `mipLODBias` `=` NUMBER - if (tryConsumeExpectedToken(TokenKind::kw_mipLODBias)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_mipLODBias)) { + // `mipLODBias` `=` NUMBER if (Params.MipLODBias.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -755,10 +745,8 @@ RootSignatureParser::parseStaticSamplerParams() { if (!MipLODBias.has_value()) return std::nullopt; Params.MipLODBias = MipLODBias; - } - - // `maxAnisotropy` `=` POS_INT - if (tryConsumeExpectedToken(TokenKind::kw_maxAnisotropy)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_maxAnisotropy)) { + // `maxAnisotropy` `=` POS_INT if (Params.MaxAnisotropy.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -771,10 +759,8 @@ RootSignatureParser::parseStaticSamplerParams() { if (!MaxAnisotropy.has_value()) return std::nullopt; Params.MaxAnisotropy = MaxAnisotropy; - } - - // `comparisonFunc` `=` COMPARISON_FUNC - if (tryConsumeExpectedToken(TokenKind::kw_comparisonFunc)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_comparisonFunc)) { + // `comparisonFunc` `=` COMPARISON_FUNC if (Params.CompFunc.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -787,10 +773,8 @@ RootSignatureParser::parseStaticSamplerParams() { if (!CompFunc.has_value()) return std::nullopt; Params.CompFunc = CompFunc; - } - - // `borderColor` `=` STATIC_BORDER_COLOR - if (tryConsumeExpectedToken(TokenKind::kw_borderColor)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_borderColor)) { + // `borderColor` `=` STATIC_BORDER_COLOR if (Params.BorderColor.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -803,10 +787,8 @@ RootSignatureParser::parseStaticSamplerParams() { if (!BorderColor.has_value()) return std::nullopt; Params.BorderColor = BorderColor; - } - - // `minLOD` `=` NUMBER - if (tryConsumeExpectedToken(TokenKind::kw_minLOD)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_minLOD)) { + // `minLOD` `=` NUMBER if (Params.MinLOD.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -819,10 +801,8 @@ RootSignatureParser::parseStaticSamplerParams() { if (!MinLOD.has_value()) return std::nullopt; Params.MinLOD = MinLOD; - } - - // `maxLOD` `=` NUMBER - if (tryConsumeExpectedToken(TokenKind::kw_maxLOD)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_maxLOD)) { + // `maxLOD` `=` NUMBER if (Params.MaxLOD.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -835,10 +815,8 @@ RootSignatureParser::parseStaticSamplerParams() { if (!MaxLOD.has_value()) return std::nullopt; Params.MaxLOD = MaxLOD; - } - - // `space` `=` POS_INT - if (tryConsumeExpectedToken(TokenKind::kw_space)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_space)) { + // `space` `=` POS_INT if (Params.Space.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -851,10 +829,8 @@ RootSignatureParser::parseStaticSamplerParams() { if (!Space.has_value()) return std::nullopt; Params.Space = Space; - } - - // `visibility` `=` SHADER_VISIBILITY - if (tryConsumeExpectedToken(TokenKind::kw_visibility)) { + } else if (tryConsumeExpectedToken(TokenKind::kw_visibility)) { + // `visibility` `=` SHADER_VISIBILITY if (Params.Visibility.has_value()) { reportDiag(diag::err_hlsl_rootsig_repeat_param) << CurToken.TokKind; return std::nullopt; @@ -868,7 +844,11 @@ RootSignatureParser::parseStaticSamplerParams() { return std::nullopt; Params.Visibility = Visibility; } - } while (tryConsumeExpectedToken(TokenKind::pu_comma)); + + // ',' denotes another element, otherwise, expected to be at ')' + if (!tryConsumeExpectedToken(TokenKind::pu_comma)) + break; + } return Params; } diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp index 601ee74388edd..e82dcadebba3f 100644 --- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp +++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp @@ -1373,4 +1373,31 @@ TEST_F(ParseHLSLRootSignatureTest, InvalidDescriptorClauseParamsCommaTest) { ASSERT_TRUE(Consumer->isSatisfied()); } +TEST_F(ParseHLSLRootSignatureTest, InvalidStaticSamplerCommaTest) { + // This test will check that an error is produced when there is a missing + // comma between parameters + const llvm::StringLiteral Source = R"cc( + StaticSampler( + s0 + maxLOD = 3 + ) + )cc"; + + auto Ctx = createMinimalASTContext(); + StringLiteral *Signature = wrapSource(Ctx, Source); + + TrivialModuleLoader ModLoader; + auto PP = createPP(Source, ModLoader); + + SmallVector<RootElement> Elements; + hlsl::RootSignatureParser Parser(RootSignatureVersion::V1_1, Elements, + Signature, *PP); + + // Test correct diagnostic produced + Consumer->setExpected(diag::err_hlsl_unexpected_end_of_params); + ASSERT_TRUE(Parser.parse()); + + ASSERT_TRUE(Consumer->isSatisfied()); +} + } // anonymous namespace >From b9cf61471f5b00ae684071d1b7b7cff4e665fc67 Mon Sep 17 00:00:00 2001 From: Finn Plummer <canadienf...@gmail.com> Date: Tue, 8 Jul 2025 18:28:36 +0000 Subject: [PATCH 7/7] rebase-fix: remove unneeded params - these are lingering from the improve diag related changes --- clang/include/clang/Parse/ParseHLSLRootSignature.h | 6 ++---- clang/lib/Parse/ParseHLSLRootSignature.cpp | 10 ++++------ 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h index fc71ad5e3722c..007c3f7ba1e9c 100644 --- a/clang/include/clang/Parse/ParseHLSLRootSignature.h +++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h @@ -99,8 +99,7 @@ class RootSignatureParser { std::optional<llvm::dxbc::RootDescriptorFlags> Flags; }; std::optional<ParsedRootDescriptorParams> - parseRootDescriptorParams(RootSignatureToken::Kind DescType, - RootSignatureToken::Kind RegType); + parseRootDescriptorParams(RootSignatureToken::Kind RegType); struct ParsedClauseParams { std::optional<llvm::hlsl::rootsig::Register> Reg; @@ -110,8 +109,7 @@ class RootSignatureParser { std::optional<llvm::dxbc::DescriptorRangeFlags> Flags; }; std::optional<ParsedClauseParams> - parseDescriptorTableClauseParams(RootSignatureToken::Kind DescType, - RootSignatureToken::Kind RegType); + parseDescriptorTableClauseParams(RootSignatureToken::Kind RegType); struct ParsedStaticSamplerParams { std::optional<llvm::hlsl::rootsig::Register> Reg; diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index 5cc1dc598334e..dc5f6faefbab4 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -199,7 +199,7 @@ std::optional<RootDescriptor> RootSignatureParser::parseRootDescriptor() { } Descriptor.setDefaultFlags(Version); - auto Params = parseRootDescriptorParams(DescriptorKind, ExpectedReg); + auto Params = parseRootDescriptorParams(ExpectedReg); if (!Params.has_value()) return std::nullopt; @@ -320,7 +320,7 @@ RootSignatureParser::parseDescriptorTableClause() { } Clause.setDefaultFlags(Version); - auto Params = parseDescriptorTableClauseParams(ParamKind, ExpectedReg); + auto Params = parseDescriptorTableClauseParams(ExpectedReg); if (!Params.has_value()) return std::nullopt; @@ -493,8 +493,7 @@ RootSignatureParser::parseRootConstantParams() { } std::optional<RootSignatureParser::ParsedRootDescriptorParams> -RootSignatureParser::parseRootDescriptorParams(TokenKind DescType, - TokenKind RegType) { +RootSignatureParser::parseRootDescriptorParams(TokenKind RegType) { assert(CurToken.TokKind == TokenKind::pu_l_paren && "Expects to only be invoked starting at given token"); @@ -563,8 +562,7 @@ RootSignatureParser::parseRootDescriptorParams(TokenKind DescType, } std::optional<RootSignatureParser::ParsedClauseParams> -RootSignatureParser::parseDescriptorTableClauseParams(TokenKind DescType, - TokenKind RegType) { +RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) { assert(CurToken.TokKind == TokenKind::pu_l_paren && "Expects to only be invoked starting at given token"); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits