[Validator] Add validation for createHandleFromBinding by damyanp · Pull Request #8205 · microsoft/DirectXShaderCompiler

damyanp · 2026-02-26T20:11:10Z

Previously, the validator didn't validate the parameters to createHandleFromBinding. This change adds validation that validates:

Bind parameter is constant — validates the bind struct parameter is a constant
Resource class is valid — validates resourceClass is one of CBuffer, Sampler, SRV, or UAV
Index range — validates constant indices fall within [rangeLowerBound, rangeUpperBound]

Fixes #7019

…osoft#7019) Add validation in BuildResMap() to check that the index parameter of createHandleFromBinding falls within the range [rangeLowerBound, rangeUpperBound] from the ResBind parameter. This matches the existing validation done for createHandle. Add a LIT test that verifies the validator catches an out-of-range index (index 0 with rangeLowerBound=1, rangeUpperBound=1). Co-authored-by: damyanp <8118402+damyanp@users.noreply.github.com>

…ation Extend the test to cover both edge cases: index below rangeLowerBound (0 < 1) and index above rangeUpperBound (4 > 3). Uses a range of [1,3] to ensure both boundaries are tested independently. Co-authored-by: damyanp <8118402+damyanp@users.noreply.github.com>

Co-authored-by: damyanp <8118402+damyanp@users.noreply.github.com>

damyanp · 2026-02-26T20:11:48Z

There doesn't seem to be any validation at all for CreateHandleFromBinding. Maybe the right way to resolve this would be to get all the validation in place?

bob80905

LGTM, though I agree it would be even better if moved nearby to where createhandle is validated.

…9-tests

Expand validation to check: 1. bind parameter is constant 2. resourceClass in the binding is valid (CBuffer/Sampler/SRV/UAV) 3. constant index is within [rangeLowerBound, rangeUpperBound] (existing) Add test for invalid resourceClass (value 5, outside 0-3 range). Co-authored-by: damyanp <8118402+damyanp@users.noreply.github.com>

Co-authored-by: damyanp <8118402+damyanp@users.noreply.github.com>

… into copilot/fix-issue-7019-tests # Conflicts: # docs/ReleaseNotes.md

Co-authored-by: damyanp <8118402+damyanp@users.noreply.github.com>

Consolidate the three separate createHandleFromBinding test files into two following the existing _passing/_failing convention: - createHandleFromBinding_passing.ll: valid indices at lower and upper bounds - createHandleFromBinding_failing.ll: index below/above range + invalid class Removed the non-constant bind test since it's a defensive check that can't be triggered without also triggering unrelated validation errors. Co-authored-by: damyanp <8118402+damyanp@users.noreply.github.com>

Re-add createHandleFromBinding_non_constant_bind.ll to maintain test coverage for the bind parameter constness check. Co-authored-by: damyanp <8118402+damyanp@users.noreply.github.com>

Remove mention of "non-constant bind parameters" from the description since that case is tested in the separate _non_constant_bind.ll file. Co-authored-by: damyanp <8118402+damyanp@users.noreply.github.com>

bogner · 2026-03-13T18:24:03Z

lib/DxilValidation/DxilValidationUtils.cpp

+      }
+
+      // Validate constant index is within binding range.
+      ConstantInt *cIndex = dyn_cast<ConstantInt>(hdl.get_index());


I don't fully understand what the DXIL::OpCode::CreateHandle version of this logic is doing where it handles a case for "index must be 0 for none array resource." (sic), so I'm not sure if it's relevant here, but do we need similar logic as well?

Thanks - I think we might be missing checks for non-constant indexes entirely.

I just pushed a version that handles this, but as soon as I pushed it I realized that using lower==upper to decide if something is an array or not is probably not the right thing to do.

It doesn't look like we can safely detect array resources without the extra information about the type that CreateHandle has. I think rejecting dynamic indexing on single element arrays would be undesireable.

@bogner - latest iteration is what I think is the response to this. We don't have enough information to add a new validation rule for this, but I added some _passing tests around arrays to capture some of the thinking.

lib/DxilValidation/DxilValidationUtils.cpp

tools/clang/test/LitDXILValidation/createHandleFromBinding_passing.ll

When the binding range has size 1 (rangeLowerBound == rangeUpperBound), the resource is non-array and the index must be a constant. This mirrors the existing CreateHandle validation which rejects non-constant indices on non-array resources. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The rangeLowerBound == rangeUpperBound check cannot distinguish non-array resources from 1-element arrays, so revert the non-constant index rejection for now. Keep the passing test case that verifies non-constant indices on array resources are accepted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Rename local variables to UpperCamelCase: hdl->Hdl, bind->Bind, cIndex->CIndex, index->Index. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

bob80905

LGTM

Copilot AI and others added 5 commits February 21, 2026 00:24

Initial plan

e0ec755

Add release note for CreateHandleFromBinding index validation fix

5dc365f

Co-authored-by: damyanp <8118402+damyanp@users.noreply.github.com>

Update release note to identify DXIL validation change

da8d8c3

Co-authored-by: damyanp <8118402+damyanp@users.noreply.github.com>

github-project-automation bot added this to HLSL Roadmap Feb 26, 2026

github-project-automation bot moved this to New in HLSL Roadmap Feb 26, 2026

damyanp marked this pull request as ready for review February 26, 2026 22:28

bob80905 approved these changes Feb 27, 2026

View reviewed changes

damyanp and others added 5 commits March 9, 2026 17:32

Verify code format changes come from the github-actions bot

9f17031

Merge remote-tracking branch 'origin/main' into copilot/fix-issue-701…

3c5196f

…9-tests

Fix resourceClass comment in test to match enum values

3b74288

Co-authored-by: damyanp <8118402+damyanp@users.noreply.github.com>

Merge branch 'main' of https://github.com/damyanp/DirectXShaderCompiler…

7dd68e5

… into copilot/fix-issue-7019-tests # Conflicts: # docs/ReleaseNotes.md

damyanp changed the title ~~[Validator] Detect obviously bad indices for createHandleFromBinding op~~ [Validator] Add validation for createHandleFromBinding Mar 12, 2026

Update release note wording and add test for non-constant bind parameter

ca0336e

Co-authored-by: damyanp <8118402+damyanp@users.noreply.github.com>

damyanp marked this pull request as draft March 12, 2026 02:37

Copilot AI and others added 3 commits March 12, 2026 02:51

Restore non-constant bind test as third test file

60dd05d

Re-add createHandleFromBinding_non_constant_bind.ll to maintain test coverage for the bind parameter constness check. Co-authored-by: damyanp <8118402+damyanp@users.noreply.github.com>

Fix inaccurate comment in createHandleFromBinding_failing.ll

57301f7

Remove mention of "non-constant bind parameters" from the description since that case is tested in the separate _non_constant_bind.ll file. Co-authored-by: damyanp <8118402+damyanp@users.noreply.github.com>

damyanp marked this pull request as ready for review March 13, 2026 00:57

damyanp requested review from bob80905, bogner and tex3d March 13, 2026 00:57

bogner reviewed Mar 13, 2026

View reviewed changes

bob80905 reviewed Mar 13, 2026

View reviewed changes

damyanp and others added 3 commits March 13, 2026 13:01

Apply LLVM naming conventions to CreateHandleFromBinding validation

d815050

Rename local variables to UpperCamelCase: hdl->Hdl, bind->Bind, cIndex->CIndex, index->Index. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

bob80905 approved these changes Mar 13, 2026

View reviewed changes

bogner approved these changes Mar 13, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Validator] Add validation for createHandleFromBinding#8205

[Validator] Add validation for createHandleFromBinding#8205
damyanp wants to merge 17 commits intomicrosoft:mainfrom
damyanp:copilot/fix-issue-7019-tests

damyanp commented Feb 26, 2026 •

edited

Loading

Uh oh!

damyanp commented Feb 26, 2026

Uh oh!

bob80905 left a comment •

edited

Loading

Uh oh!

bogner Mar 13, 2026

Uh oh!

damyanp Mar 13, 2026 •

edited

Loading

Uh oh!

damyanp Mar 13, 2026

Uh oh!

damyanp Mar 13, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bob80905 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

damyanp commented Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damyanp commented Feb 26, 2026

Uh oh!

bob80905 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogner Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

damyanp Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damyanp Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

damyanp Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bob80905 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

damyanp commented Feb 26, 2026 •

edited

Loading

bob80905 left a comment •

edited

Loading

damyanp Mar 13, 2026 •

edited

Loading