[HLSL][RootSignature] Add basic parameter validations of Root Elements #145795

inbelic · 2025-06-25T21:59:07Z

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 RootElements and verify that all non-flag parameters are within their bounds
Updates SemaHLSL to iterate through RootElements 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

llvmbot · 2025-06-25T21:59:36Z

@llvm/pr-subscribers-hlsl

@llvm/pr-subscribers-clang

Author: Finn Plummer (inbelic)

Changes

In this pr we go through and enforce the various bounded parameter values for non-flag values.

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.

This pr should implement all checks with the exception of flag values. Flag values are omitted so that this pr is not blocked by the move of all d3d12 enums to DXConatiner.h.

Update SemaHLSL to iterate through RootElements and verify that all non-flag parameters are within their bounds
Extend RootSignature-err.hlsl with testcases for all invalid specifications

Part 1 of #129940.

Full diff: https://github.com/llvm/llvm-project/pull/145795.diff

3 Files Affected:

(modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+2)
(modified) clang/lib/Sema/SemaHLSL.cpp (+71-2)
(modified) clang/test/SemaHLSL/RootSignature-err.hlsl (+41)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 6eba0619883d3..294e33418fc33 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -13059,6 +13059,8 @@ def err_invalid_hlsl_resource_type: Error<
 def err_hlsl_spirv_only: Error<"%0 is only available for the SPIR-V target">;
 def err_hlsl_vk_literal_must_contain_constant: Error<"the argument to vk::Literal must be a vk::integral_constant">;
 
+def err_hlsl_invalid_rootsig_parameter : Error<"parameter value must be in the range [%0, %1]">;
+
 def subst_hlsl_format_ranges: TextSubstitution<
   "%select{t|u|b|s}0[%1;%select{%3]|unbounded)}2">;
 
diff --git a/clang/lib/Sema/SemaHLSL.cpp b/clang/lib/Sema/SemaHLSL.cpp
index 0974ccbf9267c..1132c0c28daa1 100644
--- a/clang/lib/Sema/SemaHLSL.cpp
+++ b/clang/lib/Sema/SemaHLSL.cpp
@@ -44,6 +44,7 @@
 #include "llvm/Support/DXILABI.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/TargetParser/Triple.h"
+#include <cmath>
 #include <cstddef>
 #include <iterator>
 #include <utility>
@@ -1078,6 +1079,71 @@ void SemaHLSL::ActOnFinishRootSignatureDecl(
 
 bool SemaHLSL::handleRootSignatureDecl(HLSLRootSignatureDecl *D,
                                        SourceLocation Loc) {
+  // Define some common error handling functions
+  bool HadError = false;
+  auto ReportError = [this, Loc, &HadError](uint32_t LowerBound,
+                                            uint32_t UpperBound) {
+    HadError = true;
+    this->Diag(Loc, diag::err_hlsl_invalid_rootsig_parameter)
+        << LowerBound << UpperBound;
+  };
+
+  auto ReportFloatError = [this, Loc, &HadError](float LowerBound,
+                                                 float UpperBound) {
+    HadError = true;
+    this->Diag(Loc, diag::err_hlsl_invalid_rootsig_parameter)
+        << std::to_string(LowerBound) << std::to_string(UpperBound);
+  };
+
+  auto VerifyRegister = [ReportError](uint32_t Register) {
+    if (Register == ~0u)
+      ReportError(0, 0xfffffffe);
+  };
+
+  auto VerifySpace = [ReportError](uint32_t Space) {
+    // [0xfffffff0, 0xffffffff] is reserved system namespace
+    if (0xfffffff0 <= Space)
+      ReportError(0, 0xffffffef);
+  };
+
+  // Iterate through the elements and do basic validations
+  for (const llvm::hlsl::rootsig::RootElement &Elem : D->getRootElements()) {
+    if (const auto *Descriptor =
+            std::get_if<llvm::hlsl::rootsig::RootDescriptor>(&Elem)) {
+      VerifyRegister(Descriptor->Reg.Number);
+      VerifySpace(Descriptor->Space);
+    } else if (const auto *Constants =
+                   std::get_if<llvm::hlsl::rootsig::RootConstants>(&Elem)) {
+      VerifyRegister(Constants->Reg.Number);
+      VerifySpace(Constants->Space);
+    } else if (const auto *Sampler =
+                   std::get_if<llvm::hlsl::rootsig::StaticSampler>(&Elem)) {
+      VerifyRegister(Sampler->Reg.Number);
+      VerifySpace(Sampler->Space);
+
+      assert(!std::isnan(Sampler->MaxLOD) && !std::isnan(Sampler->MinLOD) &&
+             "By construction, parseFloatParam can't produce a NaN from a "
+             "float_literal token");
+
+      if (16 < Sampler->MaxAnisotropy)
+        ReportError(0, 16);
+      if (Sampler->MipLODBias < -16.f || 15.99 < Sampler->MipLODBias)
+        ReportFloatError(-16.f, 15.99);
+    } else if (const auto *Clause =
+                   std::get_if<llvm::hlsl::rootsig::DescriptorTableClause>(
+                       &Elem)) {
+      VerifyRegister(Clause->Reg.Number);
+      VerifySpace(Clause->Space);
+
+      if (Clause->NumDescriptors == 0) {
+        // NumDescriptor could techincally be ~0u but that is reserved for
+        // unbounded, so the diagnostic will not report that as a valid int
+        // value
+        ReportError(1, 0xfffffffe);
+      }
+    }
+  }
+
   // The following conducts analysis on resource ranges to detect and report
   // any overlaps in resource ranges.
   //
@@ -1141,7 +1207,10 @@ bool SemaHLSL::handleRootSignatureDecl(HLSLRootSignatureDecl *D,
                        &Elem)) {
       RangeInfo Info;
       Info.LowerBound = Clause->Reg.Number;
-      assert(0 < Clause->NumDescriptors && "Verified as part of TODO(#129940)");
+      // Relevant error will have already been reported above and needs to be
+      // fixed before we can conduct range analysis, so shortcut error return
+      if (Clause->NumDescriptors == 0)
+        return true;
       Info.UpperBound = Clause->NumDescriptors == RangeInfo::Unbounded
                             ? RangeInfo::Unbounded
                             : Info.LowerBound + Clause->NumDescriptors -
@@ -1247,7 +1316,7 @@ bool SemaHLSL::handleRootSignatureDecl(HLSLRootSignatureDecl *D,
         ReportOverlap(&Info, Overlapping.value());
   }
 
-  return HadOverlap;
+  return HadError | HadOverlap;
 }
 
 void SemaHLSL::handleRootSignatureAttr(Decl *D, const ParsedAttr &AL) {
diff --git a/clang/test/SemaHLSL/RootSignature-err.hlsl b/clang/test/SemaHLSL/RootSignature-err.hlsl
index f544247f4db2a..61db61f6cceb4 100644
--- a/clang/test/SemaHLSL/RootSignature-err.hlsl
+++ b/clang/test/SemaHLSL/RootSignature-err.hlsl
@@ -18,3 +18,44 @@ void bad_root_signature_3() {}
 
 [RootSignature("DescriptorTable(), invalid")] // expected-error {{expected end of stream to denote end of parameters, or, another valid parameter of RootSignature}}
 void bad_root_signature_4() {}
+
+// Basic validation of register value and space
+
+// expected-error@+2 {{parameter value must be in the range [0, 4294967294]}}
+// expected-error@+1 {{parameter value must be in the range [0, 4294967279]}}
+[RootSignature("CBV(b4294967295, space = 4294967280)")]
+void bad_root_signature_5() {}
+
+// expected-error@+2 {{parameter value must be in the range [0, 4294967294]}}
+// expected-error@+1 {{parameter value must be in the range [0, 4294967279]}}
+[RootSignature("RootConstants(b4294967295, space = 4294967280, num32BitConstants = 1)")]
+void bad_root_signature_6() {}
+
+// expected-error@+2 {{parameter value must be in the range [0, 4294967294]}}
+// expected-error@+1 {{parameter value must be in the range [0, 4294967279]}}
+[RootSignature("StaticSampler(s4294967295, space = 4294967280)")]
+void bad_root_signature_7() {}
+
+// expected-error@+2 {{parameter value must be in the range [0, 4294967294]}}
+// expected-error@+1 {{parameter value must be in the range [0, 4294967279]}}
+[RootSignature("DescriptorTable(SRV(t4294967295, space = 4294967280))")]
+void bad_root_signature_8() {}
+
+// expected-error@+2 {{parameter value must be in the range [1, 4294967294]}}
+// expected-error@+1 {{parameter value must be in the range [1, 4294967294]}}
+[RootSignature("DescriptorTable(UAV(u0, numDescriptors = 0), Sampler(s0, numDescriptors = 0))")]
+void bad_root_signature_9() {}
+
+#define ErroneousStaticSampler \
+  "StaticSampler(s0" \
+  "  maxAnisotropy = 17," \
+  ")"
+
+// expected-error@+2 {{parameter value must be in the range [0, 16]}}
+// expected-error@+1 {{parameter value must be in the range [-16.000000, 15.990000]}}
+[RootSignature("StaticSampler(s0, maxAnisotropy = 17, mipLODBias = -16.000001)")]
+void bad_root_signature_10() {}
+
+// expected-error@+1 {{parameter value must be in the range [-16.000000, 15.990000]}}
+[RootSignature("StaticSampler(s0, mipLODBias = 15.990001)")]
+void bad_root_signature_11() {}

clang/lib/Sema/SemaHLSL.cpp

inbelic · 2025-06-26T18:04:00Z

This pr have been converted to a draft until #145940 and #145815 are resolved and the logic can be re-used.

inbelic · 2025-07-09T20:17:29Z

This pr is ready for review now that it can re-use the validations from RootSignatureValidations.

The applicable changes are all commits except the first: https://github.com/llvm/llvm-project/pull/145795/files/1228e743eea29978e8893690e93e3d94775e1cbf..982f84a6128d66a0d07f6e6b305f796c79b014b1.

clang/lib/Sema/SemaHLSL.cpp

clang/include/clang/Basic/DiagnosticSemaKinds.td

clang/lib/Sema/SemaHLSL.cpp

spall · 2025-07-11T21:35:35Z

clang/lib/Sema/SemaHLSL.cpp

-      assert(0 < Clause->NumDescriptors && "Verified as part of TODO(#129940)");
+      // Relevant error will have already been reported above and needs to be
+      // fixed before we can conduct range analysis, so shortcut error return
+      if (Clause->NumDescriptors == 0)


The previous "check" here was for if it was less than 0 but now we're erroring if it equals 0?

Not quite, the check before was that it was greater than 0. 0 is on the only invalid value for this uint32_t.

clang/test/CodeGenHLSL/RootSignature.hlsl

clang/lib/Sema/SemaHLSL.cpp

- it was noted, [here](#145795 (comment)), that by accidently not specifying this explicitly as a float it will cause a build warning on MSVC - this commit resolves this by explicitly specifying it as a float

…n (#148941) - it was noted, [here](llvm/llvm-project#145795 (comment)), that by accidently not specifying this explicitly as a float it will cause a build warning on MSVC - this commit resolves this by explicitly specifying it as a float

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" HLSL HLSL Language Support labels Jun 25, 2025

inbelic commented Jun 25, 2025

View reviewed changes

clang/lib/Sema/SemaHLSL.cpp Outdated Show resolved Hide resolved

joaosaffran reviewed Jun 25, 2025

View reviewed changes

clang/lib/Sema/SemaHLSL.cpp Outdated Show resolved Hide resolved

inbelic linked an issue Jun 25, 2025 that may be closed by this pull request

[HLSL] Root Signature semantic analysis - Bounded Parameter checks #129940

Closed

10 tasks

inbelic marked this pull request as draft June 26, 2025 17:38

inbelic changed the title ~~[HLSL][RootSignature] Add bounded parameter validations of Root Elements for non-flag values~~ [HLSL][RootSignature] Add basic parameter validations of Root Elements Jul 9, 2025

inbelic force-pushed the inbelic/rs-bound-params branch 2 times, most recently from c11da71 to 982f84a Compare July 9, 2025 20:16

inbelic marked this pull request as ready for review July 9, 2025 20:17

inbelic commented Jul 9, 2025

View reviewed changes

clang/lib/Sema/SemaHLSL.cpp Show resolved Hide resolved

joaosaffran approved these changes Jul 11, 2025

View reviewed changes

spall reviewed Jul 11, 2025

View reviewed changes

inbelic added 8 commits July 12, 2025 02:42

add basic integer range validations

4399179

add basic float range validations

f234a70

fix testcase to use valid mipLODBias

697f1f6

hook up flag validations

77c8d3e

self-review: fix testcase to be compliant with version 1.0 as well

5ae69b4

review: change diag to directly input the version minor

4a2761d

review: correct the check of MaxAnisotropy

153b38f

review: clarify hex value

6811f70

inbelic force-pushed the inbelic/rs-bound-params branch from d648af3 to 6811f70 Compare July 12, 2025 02:42

self-review: use formatv + small string to use stack allocation

d77ce95

inbelic merged commit 7ccdc59 into llvm:main Jul 13, 2025
10 checks passed

inbelic deleted the inbelic/rs-bound-params branch July 14, 2025 17:34

AaronBallman reviewed Jul 15, 2025

View reviewed changes

clang/lib/Sema/SemaHLSL.cpp Show resolved Hide resolved

inbelic mentioned this pull request Jul 15, 2025

[NFC][SemaHLSL] Fix typo causing float to double conversion #148941

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[HLSL][RootSignature] Add basic parameter validations of Root Elements #145795

[HLSL][RootSignature] Add basic parameter validations of Root Elements #145795

Uh oh!

inbelic commented Jun 25, 2025 •

edited

Loading

Uh oh!

llvmbot commented Jun 25, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

inbelic commented Jun 26, 2025

Uh oh!

inbelic commented Jul 9, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spall Jul 11, 2025

Uh oh!

inbelic Jul 11, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[HLSL][RootSignature] Add basic parameter validations of Root Elements #145795

[HLSL][RootSignature] Add basic parameter validations of Root Elements #145795

Uh oh!

Conversation

inbelic commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

inbelic commented Jun 26, 2025

Uh oh!

inbelic commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spall Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

inbelic Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

inbelic commented Jun 25, 2025 •

edited

Loading

llvmbot commented Jun 25, 2025 •

edited

Loading

inbelic commented Jul 9, 2025 •

edited

Loading