Skip to content

Commit 7ccdc59

Browse files
inbelicinbelic
authored
[HLSL][RootSignature] Add basic parameter validations of Root Elements (#145795)
In this pr we go through and enforce the various bounded parameter values for non-flag values and the valid flag combinations for `RootDescriptor` and `DescriptorRange` flags. For reference of the required checks, please see here: https://github.com/llvm/wg-hlsl/blob/main/proposals/0002-root-signature-in-clang.md#validations-in-sema. - Updates `SemaHLSL` to iterate through `RootElement`s and verify that all non-flag parameters are within their bounds - Updates `SemaHLSL` to iterate through `RootElement`s and verify that all flag parameters are a valid combination - Extends `RootSignature-err.hlsl` with testcases for all invalid specifications - Adds `RootSignature-flags-err.hlsl` with testcase for invalid flag specifications Resolves: #129940
1 parent b6313b3 commit 7ccdc59

File tree

6 files changed

+202
-14
lines changed

6 files changed

+202
-14
lines changed

‎clang/include/clang/Basic/DiagnosticSemaKinds.td

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13094,6 +13094,9 @@ def err_invalid_hlsl_resource_type: Error<
1309413094
def err_hlsl_spirv_only: Error<"%0 is only available for the SPIR-V target">;
1309513095
def err_hlsl_vk_literal_must_contain_constant: Error<"the argument to vk::Literal must be a vk::integral_constant">;
1309613096

13097+
def err_hlsl_invalid_rootsig_value : Error<"value must be in the range [%0, %1]">;
13098+
def err_hlsl_invalid_rootsig_flag : Error< "invalid flags for version 1.%0">;
13099+
1309713100
def subst_hlsl_format_ranges: TextSubstitution<
1309813101
"%select{t|u|b|s}0[%1;%select{%3]|unbounded)}2">;
1309913102

‎clang/lib/Sema/SemaHLSL.cpp

Lines changed: 92 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,9 @@
4343
#include "llvm/Support/Casting.h"
4444
#include "llvm/Support/DXILABI.h"
4545
#include "llvm/Support/ErrorHandling.h"
46+
#include "llvm/Support/FormatVariadic.h"
4647
#include "llvm/TargetParser/Triple.h"
48+
#include <cmath>
4749
#include <cstddef>
4850
#include <iterator>
4951
#include <utility>
@@ -1083,6 +1085,92 @@ void SemaHLSL::ActOnFinishRootSignatureDecl(
10831085

10841086
bool SemaHLSL::handleRootSignatureElements(
10851087
ArrayRef<hlsl::RootSignatureElement> Elements) {
1088+
// Define some common error handling functions
1089+
bool HadError = false;
1090+
auto ReportError = [this, &HadError](SourceLocation Loc, uint32_t LowerBound,
1091+
uint32_t UpperBound) {
1092+
HadError = true;
1093+
this->Diag(Loc, diag::err_hlsl_invalid_rootsig_value)
1094+
<< LowerBound << UpperBound;
1095+
};
1096+
1097+
auto ReportFloatError = [this, &HadError](SourceLocation Loc,
1098+
float LowerBound,
1099+
float UpperBound) {
1100+
HadError = true;
1101+
this->Diag(Loc, diag::err_hlsl_invalid_rootsig_value)
1102+
<< llvm::formatv("{0:f}", LowerBound).sstr<6>()
1103+
<< llvm::formatv("{0:f}", UpperBound).sstr<6>();
1104+
};
1105+
1106+
auto VerifyRegister = [ReportError](SourceLocation Loc, uint32_t Register) {
1107+
if (!llvm::hlsl::rootsig::verifyRegisterValue(Register))
1108+
ReportError(Loc, 0, 0xfffffffe);
1109+
};
1110+
1111+
auto VerifySpace = [ReportError](SourceLocation Loc, uint32_t Space) {
1112+
if (!llvm::hlsl::rootsig::verifyRegisterSpace(Space))
1113+
ReportError(Loc, 0, 0xffffffef);
1114+
};
1115+
1116+
const uint32_t Version =
1117+
llvm::to_underlying(SemaRef.getLangOpts().HLSLRootSigVer);
1118+
const uint32_t VersionEnum = Version - 1;
1119+
auto ReportFlagError = [this, &HadError, VersionEnum](SourceLocation Loc) {
1120+
HadError = true;
1121+
this->Diag(Loc, diag::err_hlsl_invalid_rootsig_flag)
1122+
<< /*version minor*/ VersionEnum;
1123+
};
1124+
1125+
// Iterate through the elements and do basic validations
1126+
for (const hlsl::RootSignatureElement &RootSigElem : Elements) {
1127+
SourceLocation Loc = RootSigElem.getLocation();
1128+
const llvm::hlsl::rootsig::RootElement &Elem = RootSigElem.getElement();
1129+
if (const auto *Descriptor =
1130+
std::get_if<llvm::hlsl::rootsig::RootDescriptor>(&Elem)) {
1131+
VerifyRegister(Loc, Descriptor->Reg.Number);
1132+
VerifySpace(Loc, Descriptor->Space);
1133+
1134+
if (!llvm::hlsl::rootsig::verifyRootDescriptorFlag(
1135+
Version, llvm::to_underlying(Descriptor->Flags)))
1136+
ReportFlagError(Loc);
1137+
} else if (const auto *Constants =
1138+
std::get_if<llvm::hlsl::rootsig::RootConstants>(&Elem)) {
1139+
VerifyRegister(Loc, Constants->Reg.Number);
1140+
VerifySpace(Loc, Constants->Space);
1141+
} else if (const auto *Sampler =
1142+
std::get_if<llvm::hlsl::rootsig::StaticSampler>(&Elem)) {
1143+
VerifyRegister(Loc, Sampler->Reg.Number);
1144+
VerifySpace(Loc, Sampler->Space);
1145+
1146+
assert(!std::isnan(Sampler->MaxLOD) && !std::isnan(Sampler->MinLOD) &&
1147+
"By construction, parseFloatParam can't produce a NaN from a "
1148+
"float_literal token");
1149+
1150+
if (!llvm::hlsl::rootsig::verifyMaxAnisotropy(Sampler->MaxAnisotropy))
1151+
ReportError(Loc, 0, 16);
1152+
if (!llvm::hlsl::rootsig::verifyMipLODBias(Sampler->MipLODBias))
1153+
ReportFloatError(Loc, -16.f, 15.99);
1154+
} else if (const auto *Clause =
1155+
std::get_if<llvm::hlsl::rootsig::DescriptorTableClause>(
1156+
&Elem)) {
1157+
VerifyRegister(Loc, Clause->Reg.Number);
1158+
VerifySpace(Loc, Clause->Space);
1159+
1160+
if (!llvm::hlsl::rootsig::verifyNumDescriptors(Clause->NumDescriptors)) {
1161+
// NumDescriptor could techincally be ~0u but that is reserved for
1162+
// unbounded, so the diagnostic will not report that as a valid int
1163+
// value
1164+
ReportError(Loc, 1, 0xfffffffe);
1165+
}
1166+
1167+
if (!llvm::hlsl::rootsig::verifyDescriptorRangeFlag(
1168+
Version, llvm::to_underlying(Clause->Type),
1169+
llvm::to_underlying(Clause->Flags)))
1170+
ReportFlagError(Loc);
1171+
}
1172+
}
1173+
10861174
using RangeInfo = llvm::hlsl::rootsig::RangeInfo;
10871175
using OverlappingRanges = llvm::hlsl::rootsig::OverlappingRanges;
10881176
using InfoPairT = std::pair<RangeInfo, const hlsl::RootSignatureElement *>;
@@ -1130,7 +1218,10 @@ bool SemaHLSL::handleRootSignatureElements(
11301218
&Elem)) {
11311219
RangeInfo Info;
11321220
Info.LowerBound = Clause->Reg.Number;
1133-
assert(0 < Clause->NumDescriptors && "Verified as part of TODO(#129940)");
1221+
// Relevant error will have already been reported above and needs to be
1222+
// fixed before we can conduct range analysis, so shortcut error return
1223+
if (Clause->NumDescriptors == 0)
1224+
return true;
11341225
Info.UpperBound = Clause->NumDescriptors == RangeInfo::Unbounded
11351226
? RangeInfo::Unbounded
11361227
: Info.LowerBound + Clause->NumDescriptors -

‎clang/test/AST/HLSL/RootSignatures-AST.hlsl

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,14 @@
1313

1414
#define SampleRS "RootFlags( ALLOW_INPUT_ASSEMBLER_INPUT_LAYOUT | " \
1515
"DENY_VERTEX_SHADER_ROOT_ACCESS), " \
16-
"CBV(b0, space = 1, flags = DATA_STATIC), " \
16+
"CBV(b0, space = 1, flags = DATA_VOLATILE), " \
1717
"SRV(t0), " \
1818
"UAV(u0), " \
1919
"DescriptorTable( CBV(b1), " \
2020
"SRV(t1, numDescriptors = 8, " \
21-
" flags = DESCRIPTORS_VOLATILE), " \
21+
" flags = DATA_VOLATILE | DESCRIPTORS_VOLATILE), " \
2222
"UAV(u1, numDescriptors = unbounded, " \
23-
" flags = DESCRIPTORS_VOLATILE)), " \
23+
" flags = DATA_VOLATILE | DESCRIPTORS_VOLATILE)), " \
2424
"DescriptorTable(Sampler(s0, space=1, numDescriptors = 4)), " \
2525
"RootConstants(num32BitConstants=3, b10), " \
2626
"StaticSampler(s1)," \
@@ -34,7 +34,7 @@
3434
// CHECK-SAME: RootElements{
3535
// CHECK-SAME: RootFlags(AllowInputAssemblerInputLayout | DenyVertexShaderRootAccess),
3636
// CHECK-SAME: RootCBV(b0,
37-
// CHECK-SAME: space = 1, visibility = All, flags = DataStatic
37+
// CHECK-SAME: space = 1, visibility = All, flags = DataVolatile
3838
// CHECK-SAME: ),
3939
// CHECK-SAME: RootSRV(t0,
4040
// CHECK-SAME: space = 0, visibility = All,
@@ -50,10 +50,10 @@
5050
// CHECK-V1_1-SAME: flags = DataStaticWhileSetAtExecute
5151
// CHECK-SAME: ),
5252
// CHECK-SAME: SRV(
53-
// CHECK-SAME: t1, numDescriptors = 8, space = 0, offset = DescriptorTableOffsetAppend, flags = DescriptorsVolatile
53+
// CHECK-SAME: t1, numDescriptors = 8, space = 0, offset = DescriptorTableOffsetAppend, flags = DescriptorsVolatile | DataVolatile
5454
// CHECK-SAME: ),
5555
// CHECK-SAME: UAV(
56-
// CHECK-SAME: u1, numDescriptors = unbounded, space = 0, offset = DescriptorTableOffsetAppend, flags = DescriptorsVolatile
56+
// CHECK-SAME: u1, numDescriptors = unbounded, space = 0, offset = DescriptorTableOffsetAppend, flags = DescriptorsVolatile | DataVolatile
5757
// CHECK-SAME: ),
5858
// CHECK-SAME: DescriptorTable(
5959
// CHECK-SAME: numClauses = 3, visibility = All
@@ -92,14 +92,14 @@ void same_rs_main() {}
9292
#define SampleSameRS \
9393
"RootFlags( ALLOW_INPUT_ASSEMBLER_INPUT_LAYOUT | " \
9494
"DENY_VERTEX_SHADER_ROOT_ACCESS), " \
95-
"CBV(b0, space = 1, flags = DATA_STATIC), " \
95+
"CBV(b0, space = 1, flags = DATA_VOLATILE), " \
9696
"SRV(t0), " \
9797
"UAV(u0), " \
9898
"DescriptorTable( CBV(b1), " \
99-
"SRV(t1, numDescriptors = 8, " \
100-
" flags = DESCRIPTORS_VOLATILE), " \
101-
"UAV(u1, numDescriptors = unbounded, " \
102-
" flags = DESCRIPTORS_VOLATILE)), " \
99+
" SRV(t1, numDescriptors = 8, " \
100+
" flags = DATA_VOLATILE | DESCRIPTORS_VOLATILE), " \
101+
" UAV(u1, numDescriptors = unbounded, " \
102+
" flags = DATA_VOLATILE | DESCRIPTORS_VOLATILE)), " \
103103
"DescriptorTable(Sampler(s0, space=1, numDescriptors = 4)), " \
104104
"RootConstants(num32BitConstants=3, b10), " \
105105
"StaticSampler(s1)," \

‎clang/test/CodeGenHLSL/RootSignature.hlsl

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,8 @@ void RootDescriptorsEntry() {}
7676
// CHECK-SAME: i32 2, i32 3, i32 5,
7777

7878
// checking mipLODBias, maxAnisotropy, comparisonFunc, borderColor
79-
// CHECK-SAME: float 0x40403999A0000000, i32 9, i32 3, i32 2,
79+
// note: the hex value is the float bit representation of 12.45
80+
// CHECK-SAME: float 0x4028E66660000000, i32 9, i32 3, i32 2,
8081

8182
// checking minLOD, maxLOD
8283
// CHECK-SAME: float -1.280000e+02, float 1.280000e+02,
@@ -90,7 +91,7 @@ void RootDescriptorsEntry() {}
9091
" addressU = TEXTURE_ADDRESS_MIRROR, " \
9192
" addressV = TEXTURE_ADDRESS_CLAMP, " \
9293
" addressW = TEXTURE_ADDRESS_MIRRORONCE, " \
93-
" mipLODBias = 32.45f, maxAnisotropy = 9, " \
94+
" mipLODBias = 12.45f, maxAnisotropy = 9, " \
9495
" comparisonFunc = COMPARISON_EQUAL, " \
9596
" borderColor = STATIC_BORDER_COLOR_OPAQUE_WHITE, " \
9697
" minLOD = -128.f, maxLOD = 128.f, " \

‎clang/test/SemaHLSL/RootSignature-err.hlsl

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,3 +103,39 @@ void bad_root_signature_22() {}
103103
// expected-error@+1 {{invalid value of RootFlags}}
104104
[RootSignature("RootFlags(local_root_signature | root_flag_typo)")]
105105
void bad_root_signature_23() {}
106+
107+
// Basic validation of register value and space
108+
109+
// expected-error@+2 {{value must be in the range [0, 4294967294]}}
110+
// expected-error@+1 {{value must be in the range [0, 4294967279]}}
111+
[RootSignature("CBV(b4294967295, space = 4294967280)")]
112+
void basic_validation_0() {}
113+
114+
// expected-error@+2 {{value must be in the range [0, 4294967294]}}
115+
// expected-error@+1 {{value must be in the range [0, 4294967279]}}
116+
[RootSignature("RootConstants(b4294967295, space = 4294967280, num32BitConstants = 1)")]
117+
void basic_validation_1() {}
118+
119+
// expected-error@+2 {{value must be in the range [0, 4294967294]}}
120+
// expected-error@+1 {{value must be in the range [0, 4294967279]}}
121+
[RootSignature("StaticSampler(s4294967295, space = 4294967280)")]
122+
void basic_validation_2() {}
123+
124+
// expected-error@+2 {{value must be in the range [0, 4294967294]}}
125+
// expected-error@+1 {{value must be in the range [0, 4294967279]}}
126+
[RootSignature("DescriptorTable(SRV(t4294967295, space = 4294967280))")]
127+
void basic_validation_3() {}
128+
129+
// expected-error@+2 {{value must be in the range [1, 4294967294]}}
130+
// expected-error@+1 {{value must be in the range [1, 4294967294]}}
131+
[RootSignature("DescriptorTable(UAV(u0, numDescriptors = 0), Sampler(s0, numDescriptors = 0))")]
132+
void basic_validation_4() {}
133+
134+
// expected-error@+2 {{value must be in the range [0, 16]}}
135+
// expected-error@+1 {{value must be in the range [-16.00, 15.99]}}
136+
[RootSignature("StaticSampler(s0, maxAnisotropy = 17, mipLODBias = -16.000001)")]
137+
void basic_validation_5() {}
138+
139+
// expected-error@+1 {{value must be in the range [-16.00, 15.99]}}
140+
[RootSignature("StaticSampler(s0, mipLODBias = 15.990001)")]
141+
void basic_validation_6() {}

‎clang/test/SemaHLSL/RootSignature-flags-err.hlsl

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -fsyntax-only \
2+
// RUN: -fdx-rootsignature-version=rootsig_1_0 %s -verify=v10
3+
// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -fsyntax-only \
4+
// RUN: -fdx-rootsignature-version=rootsig_1_1 %s -verify=v11
5+
6+
// Root Descriptor Flags:
7+
8+
// v10-error@+1 {{invalid flags for version 1.0}}
9+
[RootSignature("CBV(b0, flags = DATA_STATIC)")]
10+
void bad_root_descriptor_flags_0() {}
11+
12+
// v10-error@+1 {{invalid flags for version 1.0}}
13+
[RootSignature("CBV(b0, flags = DATA_STATIC_WHILE_SET_AT_EXECUTE)")]
14+
void bad_root_descriptor_flags_1() {}
15+
16+
// v10-error@+2 {{invalid flags for version 1.0}}
17+
// v11-error@+1 {{invalid flags for version 1.1}}
18+
[RootSignature("CBV(b0, flags = DATA_STATIC | DATA_VOLATILE)")]
19+
void bad_root_descriptor_flags_2() {}
20+
21+
// Descriptor Range Flags:
22+
23+
// v10-error@+1 {{invalid flags for version 1.0}}
24+
[RootSignature("DescriptorTable(CBV(b0, flags = DATA_VOLATILE))")]
25+
void bad_descriptor_range_flags_0() {}
26+
27+
// v10-error@+1 {{invalid flags for version 1.0}}
28+
[RootSignature("DescriptorTable(CBV(b0, flags = DATA_STATIC))")]
29+
void bad_descriptor_range_flags_1() {}
30+
31+
// v10-error@+1 {{invalid flags for version 1.0}}
32+
[RootSignature("DescriptorTable(CBV(b0, flags = DATA_STATIC_WHILE_SET_AT_EXECUTE | DESCRIPTORS_VOLATILE))")]
33+
void bad_descriptor_range_flags_2() {}
34+
35+
// v10-error@+1 {{invalid flags for version 1.0}}
36+
[RootSignature("DescriptorTable(CBV(b0, flags = DESCRIPTORS_VOLATILE))")]
37+
void bad_descriptor_range_flags_3() {}
38+
39+
// v10-error@+1 {{invalid flags for version 1.0}}
40+
[RootSignature("DescriptorTable(CBV(b0, flags = DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS))")]
41+
void bad_descriptor_range_flags_4() {}
42+
43+
// v10-error@+2 {{invalid flags for version 1.0}}
44+
// v11-error@+1 {{invalid flags for version 1.1}}
45+
[RootSignature("DescriptorTable(CBV(b0, flags = DATA_STATIC | DATA_STATIC_WHILE_SET_AT_EXECUTE))")]
46+
void bad_descriptor_range_flags_5() {}
47+
48+
// v10-error@+2 {{invalid flags for version 1.0}}
49+
// v11-error@+1 {{invalid flags for version 1.1}}
50+
[RootSignature("DescriptorTable(CBV(b0, flags = DESCRIPTORS_VOLATILE | DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS))")]
51+
void bad_descriptor_range_flags_6() {}
52+
53+
// v10-error@+2 {{invalid flags for version 1.0}}
54+
// v11-error@+1 {{invalid flags for version 1.1}}
55+
[RootSignature("DescriptorTable(CBV(b0, flags = DESCRIPTORS_VOLATILE | DATA_STATIC))")]
56+
void bad_descriptor_range_flags_7() {}
57+

0 commit comments

Comments
 (0)