fix crash from static members of cbuffers - #8731
fix crash from static members of cbuffers#8731Brendan Duncan (brendan-duncan) wants to merge 5 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Pull request overview
Fixes SPIR-V generation crashes caused by static buffer members consuming layout indices.
Changes:
- Excludes static declarations from buffer layouts and indices.
- Emits static buffer members as globals.
- Adds cbuffer regression coverage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tools/clang/lib/SPIRV/DeclResultIdMapper.cpp |
Skips static layout members. |
tools/clang/lib/SPIRV/SpirvEmitter.cpp |
Emits static members separately. |
tools/clang/test/CodeGenSPIRV/cbuffer.static.member.hlsl |
Tests layout and access indices. |
8fc9e90 to
00df2c6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
tools/clang/lib/SPIRV/DeclResultIdMapper.cpp:1577
- This check occurs after the resource branch, so a static resource declared in a cbuffer/tbuffer is still passed to
createExternVar()and treated as a descriptor;doHLSLBufferDecl()then emits the same declaration again as a static global. Move the static check ahead of the resource handling so every static member bypasses buffer/resource registration.
if (isStaticBufferDecl(varDecl))
continue;
tools/clang/lib/SPIRV/SpirvEmitter.cpp:2188
- This is a user-visible compiler crash fix, so it warrants an entry in
docs/ReleaseNotes.mdunder the release-note policy inCONTRIBUTING.md:117-128. Please add the entry, or point to planned shared release-note coverage if this is part of a larger effort.
if (decl->getStorageClass() != StorageClass::SC_Static) {
// This is a VarDecl of cbuffer/tbuffer type.
doHLSLBufferDecl(bufferDecl);
return;
tools/clang/lib/SPIRV/DeclResultIdMapper.cpp:1752
- The new shader-record indexing path is not exercised by
cbuffer.static.member.hlsl, which only reachescreateCTBuffer(). Please add a shader-record cbuffer regression with a static declaration between ordinary members and verify the later access-chain indices, since this branch had the same out-of-bounds indexing failure.
if (isStaticBufferDecl(varDecl))
continue;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
docs/ReleaseNotes.md:61
- The release note overstates this fix by including ordinary structs. Struct layout already excluded static
VarDecls at the check now wrapped byisStaticBufferDecl, and this PR does not otherwise change struct handling; the functional changes are limited to HLSL buffers. Remove/structhere, or include the missing struct behavior change and regression test.
- Fixed a crash from `static` members of a `cbuffer`/`tbuffer`/struct, which were
docs/ReleaseNotes.md:64
- This links to a specific pull request, which the release-note policy explicitly disallows (
CONTRIBUTING.md:136-140). End the sentence on the preceding line and omit this PR link; a bug link can be used instead if one exists.
[#8731](https://github.com/microsoft/DirectXShaderCompiler/pull/8731).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tools/clang/test/CodeGenSPIRV/cbuffer.static.member.hlsl:22
- These regression cases only use builtin
static constvalues. Even without the new eager-emission loop,getDeclEvalInfo()can lazily recover each declaration throughtryToCreateConstantVar()(DeclResultIdMapper.cpp:1015-1023), so this test would still pass and would not protect the statedSpirvEmitter.cpp:1985-1988behavior. Please include a static that cannot be lazily constant-folded (for example, an uninitialized mutable static that is read or written) and check itsPrivateOpVariable.
static const uint a_mode = 10;
|
copilot notes have been addressed. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Can you rebase or merge main into this branch please? The release note added here gets auto-merged into the wrong section. |
Yup, merged main and moved the release not up to the new upcoming section. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tools/clang/lib/SPIRV/SpirvEmitter.cpp:1980
- The new tests do not isolate this eager-emission path: every static is a referenced scalar constant, so
getDeclEvalInfo()can recreate it lazily throughtryToCreateConstantVar()even if this loop is removed. Please add a case that cannot pass via lazy constant creation (for example, an unused static whose emitted declaration is checked, or a valid non-constant static) so the stated requirement that buffer statics remain emitted is regression-tested.
for (const auto *member : bufferDecl->decls()) {
const auto *varMember = dyn_cast<VarDecl>(member);
if (varMember && varMember->getStorageClass() == StorageClass::SC_Static)
doVarDecl(varMember);
static members of cbuffers, tbuffers, and structs are crashing the SPIR-V generator for DXC due to incorrectly being considered members of the struct and throwing off the actual member indices. DXC should not include static members when evaluating members.
Fixes #8537.
Unlike the related fix in #8538, which solves the main problem, this PR makes sure the statics are still emitted in SpirvEmitter.cpp.