Author: Finn Plummer Date: 2025-07-13T10:23:38-07:00 New Revision: 7ccdc595f8ecca0bc477c3e17683c52dca440845
URL: https://github.com/llvm/llvm-project/commit/7ccdc595f8ecca0bc477c3e17683c52dca440845 DIFF: https://github.com/llvm/llvm-project/commit/7ccdc595f8ecca0bc477c3e17683c52dca440845.diff LOG: [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: https://github.com/llvm/llvm-project/issues/129940 Added: clang/test/SemaHLSL/RootSignature-flags-err.hlsl Modified: clang/include/clang/Basic/DiagnosticSemaKinds.td clang/lib/Sema/SemaHLSL.cpp clang/test/AST/HLSL/RootSignatures-AST.hlsl clang/test/CodeGenHLSL/RootSignature.hlsl clang/test/SemaHLSL/RootSignature-err.hlsl Removed: ################################################################################ diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index b285309e0b3ca..577adc30ba2fa 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -13094,6 +13094,9 @@ 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_value : Error<"value must be in the range [%0, %1]">; +def err_hlsl_invalid_rootsig_flag : Error< "invalid flags for version 1.%0">; + 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 9af32138f9385..4875c25c76988 100644 --- a/clang/lib/Sema/SemaHLSL.cpp +++ b/clang/lib/Sema/SemaHLSL.cpp @@ -43,7 +43,9 @@ #include "llvm/Support/Casting.h" #include "llvm/Support/DXILABI.h" #include "llvm/Support/ErrorHandling.h" +#include "llvm/Support/FormatVariadic.h" #include "llvm/TargetParser/Triple.h" +#include <cmath> #include <cstddef> #include <iterator> #include <utility> @@ -1083,6 +1085,92 @@ void SemaHLSL::ActOnFinishRootSignatureDecl( bool SemaHLSL::handleRootSignatureElements( ArrayRef<hlsl::RootSignatureElement> Elements) { + // Define some common error handling functions + bool HadError = false; + auto ReportError = [this, &HadError](SourceLocation Loc, uint32_t LowerBound, + uint32_t UpperBound) { + HadError = true; + this->Diag(Loc, diag::err_hlsl_invalid_rootsig_value) + << LowerBound << UpperBound; + }; + + auto ReportFloatError = [this, &HadError](SourceLocation Loc, + float LowerBound, + float UpperBound) { + HadError = true; + this->Diag(Loc, diag::err_hlsl_invalid_rootsig_value) + << llvm::formatv("{0:f}", LowerBound).sstr<6>() + << llvm::formatv("{0:f}", UpperBound).sstr<6>(); + }; + + auto VerifyRegister = [ReportError](SourceLocation Loc, uint32_t Register) { + if (!llvm::hlsl::rootsig::verifyRegisterValue(Register)) + ReportError(Loc, 0, 0xfffffffe); + }; + + auto VerifySpace = [ReportError](SourceLocation Loc, uint32_t Space) { + if (!llvm::hlsl::rootsig::verifyRegisterSpace(Space)) + ReportError(Loc, 0, 0xffffffef); + }; + + const uint32_t Version = + llvm::to_underlying(SemaRef.getLangOpts().HLSLRootSigVer); + const uint32_t VersionEnum = Version - 1; + auto ReportFlagError = [this, &HadError, VersionEnum](SourceLocation Loc) { + HadError = true; + this->Diag(Loc, diag::err_hlsl_invalid_rootsig_flag) + << /*version minor*/ VersionEnum; + }; + + // Iterate through the elements and do basic validations + for (const hlsl::RootSignatureElement &RootSigElem : Elements) { + SourceLocation Loc = RootSigElem.getLocation(); + const llvm::hlsl::rootsig::RootElement &Elem = RootSigElem.getElement(); + if (const auto *Descriptor = + std::get_if<llvm::hlsl::rootsig::RootDescriptor>(&Elem)) { + VerifyRegister(Loc, Descriptor->Reg.Number); + VerifySpace(Loc, Descriptor->Space); + + if (!llvm::hlsl::rootsig::verifyRootDescriptorFlag( + Version, llvm::to_underlying(Descriptor->Flags))) + ReportFlagError(Loc); + } else if (const auto *Constants = + std::get_if<llvm::hlsl::rootsig::RootConstants>(&Elem)) { + VerifyRegister(Loc, Constants->Reg.Number); + VerifySpace(Loc, Constants->Space); + } else if (const auto *Sampler = + std::get_if<llvm::hlsl::rootsig::StaticSampler>(&Elem)) { + VerifyRegister(Loc, Sampler->Reg.Number); + VerifySpace(Loc, 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 (!llvm::hlsl::rootsig::verifyMaxAnisotropy(Sampler->MaxAnisotropy)) + ReportError(Loc, 0, 16); + if (!llvm::hlsl::rootsig::verifyMipLODBias(Sampler->MipLODBias)) + ReportFloatError(Loc, -16.f, 15.99); + } else if (const auto *Clause = + std::get_if<llvm::hlsl::rootsig::DescriptorTableClause>( + &Elem)) { + VerifyRegister(Loc, Clause->Reg.Number); + VerifySpace(Loc, Clause->Space); + + if (!llvm::hlsl::rootsig::verifyNumDescriptors(Clause->NumDescriptors)) { + // NumDescriptor could techincally be ~0u but that is reserved for + // unbounded, so the diagnostic will not report that as a valid int + // value + ReportError(Loc, 1, 0xfffffffe); + } + + if (!llvm::hlsl::rootsig::verifyDescriptorRangeFlag( + Version, llvm::to_underlying(Clause->Type), + llvm::to_underlying(Clause->Flags))) + ReportFlagError(Loc); + } + } + using RangeInfo = llvm::hlsl::rootsig::RangeInfo; using OverlappingRanges = llvm::hlsl::rootsig::OverlappingRanges; using InfoPairT = std::pair<RangeInfo, const hlsl::RootSignatureElement *>; @@ -1130,7 +1218,10 @@ bool SemaHLSL::handleRootSignatureElements( &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 - diff --git a/clang/test/AST/HLSL/RootSignatures-AST.hlsl b/clang/test/AST/HLSL/RootSignatures-AST.hlsl index 27c40430c9d0a..df06165f1f1f9 100644 --- a/clang/test/AST/HLSL/RootSignatures-AST.hlsl +++ b/clang/test/AST/HLSL/RootSignatures-AST.hlsl @@ -13,14 +13,14 @@ #define SampleRS "RootFlags( ALLOW_INPUT_ASSEMBLER_INPUT_LAYOUT | " \ "DENY_VERTEX_SHADER_ROOT_ACCESS), " \ - "CBV(b0, space = 1, flags = DATA_STATIC), " \ + "CBV(b0, space = 1, flags = DATA_VOLATILE), " \ "SRV(t0), " \ "UAV(u0), " \ "DescriptorTable( CBV(b1), " \ "SRV(t1, numDescriptors = 8, " \ - " flags = DESCRIPTORS_VOLATILE), " \ + " flags = DATA_VOLATILE | DESCRIPTORS_VOLATILE), " \ "UAV(u1, numDescriptors = unbounded, " \ - " flags = DESCRIPTORS_VOLATILE)), " \ + " flags = DATA_VOLATILE | DESCRIPTORS_VOLATILE)), " \ "DescriptorTable(Sampler(s0, space=1, numDescriptors = 4)), " \ "RootConstants(num32BitConstants=3, b10), " \ "StaticSampler(s1)," \ @@ -34,7 +34,7 @@ // CHECK-SAME: RootElements{ // CHECK-SAME: RootFlags(AllowInputAssemblerInputLayout | DenyVertexShaderRootAccess), // CHECK-SAME: RootCBV(b0, -// CHECK-SAME: space = 1, visibility = All, flags = DataStatic +// CHECK-SAME: space = 1, visibility = All, flags = DataVolatile // CHECK-SAME: ), // CHECK-SAME: RootSRV(t0, // CHECK-SAME: space = 0, visibility = All, @@ -50,10 +50,10 @@ // CHECK-V1_1-SAME: flags = DataStaticWhileSetAtExecute // CHECK-SAME: ), // CHECK-SAME: SRV( -// CHECK-SAME: t1, numDescriptors = 8, space = 0, offset = DescriptorTableOffsetAppend, flags = DescriptorsVolatile +// CHECK-SAME: t1, numDescriptors = 8, space = 0, offset = DescriptorTableOffsetAppend, flags = DescriptorsVolatile | DataVolatile // CHECK-SAME: ), // CHECK-SAME: UAV( -// CHECK-SAME: u1, numDescriptors = unbounded, space = 0, offset = DescriptorTableOffsetAppend, flags = DescriptorsVolatile +// CHECK-SAME: u1, numDescriptors = unbounded, space = 0, offset = DescriptorTableOffsetAppend, flags = DescriptorsVolatile | DataVolatile // CHECK-SAME: ), // CHECK-SAME: DescriptorTable( // CHECK-SAME: numClauses = 3, visibility = All @@ -92,14 +92,14 @@ void same_rs_main() {} #define SampleSameRS \ "RootFlags( ALLOW_INPUT_ASSEMBLER_INPUT_LAYOUT | " \ "DENY_VERTEX_SHADER_ROOT_ACCESS), " \ - "CBV(b0, space = 1, flags = DATA_STATIC), " \ + "CBV(b0, space = 1, flags = DATA_VOLATILE), " \ "SRV(t0), " \ "UAV(u0), " \ "DescriptorTable( CBV(b1), " \ - "SRV(t1, numDescriptors = 8, " \ - " flags = DESCRIPTORS_VOLATILE), " \ - "UAV(u1, numDescriptors = unbounded, " \ - " flags = DESCRIPTORS_VOLATILE)), " \ + " SRV(t1, numDescriptors = 8, " \ + " flags = DATA_VOLATILE | DESCRIPTORS_VOLATILE), " \ + " UAV(u1, numDescriptors = unbounded, " \ + " flags = DATA_VOLATILE | DESCRIPTORS_VOLATILE)), " \ "DescriptorTable(Sampler(s0, space=1, numDescriptors = 4)), " \ "RootConstants(num32BitConstants=3, b10), " \ "StaticSampler(s1)," \ diff --git a/clang/test/CodeGenHLSL/RootSignature.hlsl b/clang/test/CodeGenHLSL/RootSignature.hlsl index 6618ca741aa9d..bc40bdd79ce59 100644 --- a/clang/test/CodeGenHLSL/RootSignature.hlsl +++ b/clang/test/CodeGenHLSL/RootSignature.hlsl @@ -76,7 +76,8 @@ void RootDescriptorsEntry() {} // CHECK-SAME: i32 2, i32 3, i32 5, // checking mipLODBias, maxAnisotropy, comparisonFunc, borderColor -// CHECK-SAME: float 0x40403999A0000000, i32 9, i32 3, i32 2, +// note: the hex value is the float bit representation of 12.45 +// CHECK-SAME: float 0x4028E66660000000, i32 9, i32 3, i32 2, // checking minLOD, maxLOD // CHECK-SAME: float -1.280000e+02, float 1.280000e+02, @@ -90,7 +91,7 @@ void RootDescriptorsEntry() {} " addressU = TEXTURE_ADDRESS_MIRROR, " \ " addressV = TEXTURE_ADDRESS_CLAMP, " \ " addressW = TEXTURE_ADDRESS_MIRRORONCE, " \ - " mipLODBias = 32.45f, maxAnisotropy = 9, " \ + " mipLODBias = 12.45f, maxAnisotropy = 9, " \ " comparisonFunc = COMPARISON_EQUAL, " \ " borderColor = STATIC_BORDER_COLOR_OPAQUE_WHITE, " \ " minLOD = -128.f, maxLOD = 128.f, " \ diff --git a/clang/test/SemaHLSL/RootSignature-err.hlsl b/clang/test/SemaHLSL/RootSignature-err.hlsl index 4b53a127d2adb..e517274d937b0 100644 --- a/clang/test/SemaHLSL/RootSignature-err.hlsl +++ b/clang/test/SemaHLSL/RootSignature-err.hlsl @@ -103,3 +103,39 @@ void bad_root_signature_22() {} // expected-error@+1 {{invalid value of RootFlags}} [RootSignature("RootFlags(local_root_signature | root_flag_typo)")] void bad_root_signature_23() {} + +// Basic validation of register value and space + +// expected-error@+2 {{value must be in the range [0, 4294967294]}} +// expected-error@+1 {{value must be in the range [0, 4294967279]}} +[RootSignature("CBV(b4294967295, space = 4294967280)")] +void basic_validation_0() {} + +// expected-error@+2 {{value must be in the range [0, 4294967294]}} +// expected-error@+1 {{value must be in the range [0, 4294967279]}} +[RootSignature("RootConstants(b4294967295, space = 4294967280, num32BitConstants = 1)")] +void basic_validation_1() {} + +// expected-error@+2 {{value must be in the range [0, 4294967294]}} +// expected-error@+1 {{value must be in the range [0, 4294967279]}} +[RootSignature("StaticSampler(s4294967295, space = 4294967280)")] +void basic_validation_2() {} + +// expected-error@+2 {{value must be in the range [0, 4294967294]}} +// expected-error@+1 {{value must be in the range [0, 4294967279]}} +[RootSignature("DescriptorTable(SRV(t4294967295, space = 4294967280))")] +void basic_validation_3() {} + +// expected-error@+2 {{value must be in the range [1, 4294967294]}} +// expected-error@+1 {{value must be in the range [1, 4294967294]}} +[RootSignature("DescriptorTable(UAV(u0, numDescriptors = 0), Sampler(s0, numDescriptors = 0))")] +void basic_validation_4() {} + +// expected-error@+2 {{value must be in the range [0, 16]}} +// expected-error@+1 {{value must be in the range [-16.00, 15.99]}} +[RootSignature("StaticSampler(s0, maxAnisotropy = 17, mipLODBias = -16.000001)")] +void basic_validation_5() {} + +// expected-error@+1 {{value must be in the range [-16.00, 15.99]}} +[RootSignature("StaticSampler(s0, mipLODBias = 15.990001)")] +void basic_validation_6() {} diff --git a/clang/test/SemaHLSL/RootSignature-flags-err.hlsl b/clang/test/SemaHLSL/RootSignature-flags-err.hlsl new file mode 100644 index 0000000000000..9449d33cee1ad --- /dev/null +++ b/clang/test/SemaHLSL/RootSignature-flags-err.hlsl @@ -0,0 +1,57 @@ +// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -fsyntax-only \ +// RUN: -fdx-rootsignature-version=rootsig_1_0 %s -verify=v10 +// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -fsyntax-only \ +// RUN: -fdx-rootsignature-version=rootsig_1_1 %s -verify=v11 + +// Root Descriptor Flags: + +// v10-error@+1 {{invalid flags for version 1.0}} +[RootSignature("CBV(b0, flags = DATA_STATIC)")] +void bad_root_descriptor_flags_0() {} + +// v10-error@+1 {{invalid flags for version 1.0}} +[RootSignature("CBV(b0, flags = DATA_STATIC_WHILE_SET_AT_EXECUTE)")] +void bad_root_descriptor_flags_1() {} + +// v10-error@+2 {{invalid flags for version 1.0}} +// v11-error@+1 {{invalid flags for version 1.1}} +[RootSignature("CBV(b0, flags = DATA_STATIC | DATA_VOLATILE)")] +void bad_root_descriptor_flags_2() {} + +// Descriptor Range Flags: + +// v10-error@+1 {{invalid flags for version 1.0}} +[RootSignature("DescriptorTable(CBV(b0, flags = DATA_VOLATILE))")] +void bad_descriptor_range_flags_0() {} + +// v10-error@+1 {{invalid flags for version 1.0}} +[RootSignature("DescriptorTable(CBV(b0, flags = DATA_STATIC))")] +void bad_descriptor_range_flags_1() {} + +// v10-error@+1 {{invalid flags for version 1.0}} +[RootSignature("DescriptorTable(CBV(b0, flags = DATA_STATIC_WHILE_SET_AT_EXECUTE | DESCRIPTORS_VOLATILE))")] +void bad_descriptor_range_flags_2() {} + +// v10-error@+1 {{invalid flags for version 1.0}} +[RootSignature("DescriptorTable(CBV(b0, flags = DESCRIPTORS_VOLATILE))")] +void bad_descriptor_range_flags_3() {} + +// v10-error@+1 {{invalid flags for version 1.0}} +[RootSignature("DescriptorTable(CBV(b0, flags = DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS))")] +void bad_descriptor_range_flags_4() {} + +// v10-error@+2 {{invalid flags for version 1.0}} +// v11-error@+1 {{invalid flags for version 1.1}} +[RootSignature("DescriptorTable(CBV(b0, flags = DATA_STATIC | DATA_STATIC_WHILE_SET_AT_EXECUTE))")] +void bad_descriptor_range_flags_5() {} + +// v10-error@+2 {{invalid flags for version 1.0}} +// v11-error@+1 {{invalid flags for version 1.1}} +[RootSignature("DescriptorTable(CBV(b0, flags = DESCRIPTORS_VOLATILE | DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS))")] +void bad_descriptor_range_flags_6() {} + +// v10-error@+2 {{invalid flags for version 1.0}} +// v11-error@+1 {{invalid flags for version 1.1}} +[RootSignature("DescriptorTable(CBV(b0, flags = DESCRIPTORS_VOLATILE | DATA_STATIC))")] +void bad_descriptor_range_flags_7() {} + _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
