[HLSL][RootSignature] Add bounded parameter validations of Root Elements for non-flag values

[inbelic]

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.

[llvmbot]

@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() {}

[inbelic]

This is duplicated since templated lambdas are a c++20 feature.

Further, we use std::to_string as StreamingDiagnostic does not have an operator<< overload for float/double. Would welcome alternatives.

[joaosaffran]

Wondering, should those validations be shared between backend and frontend? I've the same checks when parsing the metadata.

[inbelic]

I agree it would be nice to copy the same verification functionality.

I am not too sure there really is a nice place to put these functions as it currently stands. Although, we are going to do some moving of enums soon anyway.

Imo, it might not make much sense to move the definitions of the verify.* functions from DXILRootSignature to DXContainer so we would want to find a new location. Maybe something like llvm/Support/RootSignature that both the Frontend and BinaryFormat + Target/DirectX could depend on? WDYT?