[clang] d60da27 - [HLSL][RootSignature] Implement diagnostic for missed comma (#147350)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 10 10:52:23 PDT 2025
Author: Finn Plummer
Date: 2025-07-10T10:52:20-07:00
New Revision: d60da27400cd96855542cd992d326c10a34dd0f7
URL: https://github.com/llvm/llvm-project/commit/d60da27400cd96855542cd992d326c10a34dd0f7
DIFF: https://github.com/llvm/llvm-project/commit/d60da27400cd96855542cd992d326c10a34dd0f7.diff
LOG: [HLSL][RootSignature] Implement diagnostic for missed comma (#147350)
This pr fixes a bug that allows parameters to be specified without an
intermediate comma.
After this pr, we will correctly produce a diagnostic for (eg):
```
RootFlags(0) CBV(b0)
```
This pr updates the problematic code pattern containing a chain of 'if'
statements to a chain of 'else if' statements, to prevent parsing of an
element before checking for a comma.
This pr also does 2 small updates, while in the region:
1. Simplify the `do` loop that these `if` statements are contained in.
This helps code readability and makes it easier to improve the
diagnostics further
2. Moves the `consumeExpectedToken` function calls to be right after the
`parse.*Params` invocation. This will ensure that the comma or invalid
token error is presented before a "missed mandatory param" diagnostic.
- Updates all occurrences of the if chains with an else-if chain
- Simplifies the surrounding `do` loop to be an easier to understand
`while` loop
- Moves the `consumeExpectedToken` diagnostic right after the loop so
that the missing comma diagnostic is produce before checking for any
missed mandatory arguments
- Adds unit tests for this scenario
- Small fix to the diagnostic of `RootDescriptors` to use their
respective `Token` instead of `RootConstants`
Resolves: https://github.com/llvm/llvm-project/issues/147337
Added:
clang/test/SemaHLSL/RootSignature.hlsl
Modified:
clang/lib/Parse/ParseHLSLRootSignature.cpp
clang/test/SemaHLSL/RootSignature-err.hlsl
clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
Removed:
################################################################################
diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index cf86c62f3b671..dc5f6faefbab4 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,
@@ -139,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)
@@ -162,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;
}
@@ -206,6 +203,11 @@ std::optional<RootDescriptor> RootSignatureParser::parseRootDescriptor() {
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;
@@ -224,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;
}
@@ -243,18 +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++;
- }
-
- if (tryConsumeExpectedToken(TokenKind::kw_visibility)) {
+ } 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;
@@ -267,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;
}
@@ -323,6 +324,11 @@ RootSignatureParser::parseDescriptorTableClause() {
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;
@@ -344,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;
}
@@ -366,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;
@@ -411,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;
}
@@ -428,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;
@@ -443,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;
@@ -455,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;
@@ -471,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;
@@ -488,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;
}
@@ -499,9 +498,9 @@ RootSignatureParser::parseRootDescriptorParams(TokenKind RegType) {
"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;
@@ -510,10 +509,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;
@@ -526,10 +523,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;
@@ -542,10 +537,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;
@@ -559,7 +552,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;
}
@@ -570,9 +567,9 @@ RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) {
"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;
@@ -581,10 +578,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;
@@ -603,10 +598,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;
@@ -619,10 +612,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;
@@ -641,10 +632,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;
@@ -659,7 +648,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;
}
@@ -670,9 +662,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;
@@ -681,10 +673,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;
@@ -697,10 +687,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;
@@ -713,10 +701,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;
@@ -729,10 +715,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;
@@ -745,10 +729,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;
@@ -761,10 +743,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;
@@ -777,10 +757,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;
@@ -793,10 +771,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;
@@ -809,10 +785,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;
@@ -825,10 +799,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;
@@ -841,10 +813,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;
@@ -857,10 +827,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;
@@ -874,7 +842,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/test/SemaHLSL/RootSignature-err.hlsl b/clang/test/SemaHLSL/RootSignature-err.hlsl
index 118fc38daf3f2..04013974d28b9 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 at +1 {{expected ')' to denote end of parameters, or, another valid parameter of RootConstants}}
[RootSignature(MultiLineRootSignature)]
void bad_root_signature_6() {}
+
+// expected-error at +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/test/SemaHLSL/RootSignature.hlsl b/clang/test/SemaHLSL/RootSignature.hlsl
new file mode 100644
index 0000000000000..810f81479caab
--- /dev/null
+++ b/clang/test/SemaHLSL/RootSignature.hlsl
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -fsyntax-only %s -verify
+
+// expected-no-diagnostics
+
+// Test that we have consistent behaviour for comma parsing. Namely:
+// - a single trailing comma is allowed after any parameter
+// - a trailing comma is not required
+
+[RootSignature("CBV(b0, flags = DATA_VOLATILE,), DescriptorTable(Sampler(s0,),),")]
+void maximum_commas() {}
+
+[RootSignature("CBV(b0, flags = DATA_VOLATILE), DescriptorTable(Sampler(s0))")]
+void minimal_commas() {}
diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
index ff1697f1bbb9a..e82dcadebba3f 100644
--- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
+++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
@@ -1238,4 +1238,166 @@ 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());
+}
+
+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());
+}
+
+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());
+}
+
+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());
+}
+
+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());
+}
+
+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
More information about the cfe-commits
mailing list