Fix #12728: preserve front-end pointer literals during linking - #12729
Fix #12728: preserve front-end pointer literals during linking#12729kaizhangNV wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthrough
ChangesPointer literal cloning
Suggested reviewers: Merge Risk: 🟡 Moderate · up to The change preserves front-end declaration pointers during linking, but those pointers can be serialized into prelinked modules and become invalid after reload or compiler-session teardown. This creates a concrete correctness risk, so merge should wait for a stripping boundary or explicit owner acceptance. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The change preserves non-null kIROp_PtrLit values during IR cloning by using the cloned type and original pointer value. This removes the reported null-only assertion in structural ray-tracing stage composition while retaining the existing front-end stripping step. [ ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f07b0662-4c6a-46b5-9511-de6099ed1332
📒 Files selected for processing (1)
source/slang/slang-ir-link.cpp
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| // Non-null pointer literals are used by front-end-only decorations to retain an AST | ||
| // declaration for diagnostics. Linking stays within the same compiler session, so the | ||
| // declaration remains valid and must be preserved until the mandatory passes consume | ||
| // it. Front-end-only instructions are stripped before target legalization and emit. | ||
| return builder->getPtrValue( | ||
| cloneType(this, c->getFullType()), | ||
| c->value.ptrVal); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'IRPrelinkContext|prelinkIR\(|IRSpecContext::maybeCloneValue|kIROp_PtrLit|getPtrValue' \
source/slang
rg -n -C 8 \
'serialize|deserialize|IRPtrLit|kIROp_PtrLit|PtrLit|strip' \
. --glob '*.{cpp,h,hpp,slang}' --glob '!build/**' --glob '!out/**' || trueRepository: shader-slang/slang
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- changed branch and cloning callers ---'
sed -n '360,415p' source/slang/slang-ir-link.cpp
sed -n '2440,2605p' source/slang/slang-ir-link.cpp
printf '%s\n' '--- pointer literal definitions and serialization handlers ---'
rg -n -C 12 \
'class IRPtrLit|struct IRPtrLit|IRPtrLit::|getPtrValue\(|kIROp_PtrLit' \
source/slang/slang-ir-insts.h source/slang/slang-ir.cpp source/slang/slang-ir-clone.cpp \
source/slang/slang-serialize-ir.cpp source/slang/slang-serialize-ir.h
printf '%s\n' '--- prelink and module persistence entry points ---'
rg -n -C 10 \
'prelinkIR|Prelink|writeSerializedModuleIR|readSerializedModuleIR|serialize\(.*Module|deserialize' \
source/slang/slang-ir-link.cpp source/slang/slang-serialize-ir.cpp \
source/slang/slang-serialize-container.cpp source/slang/slang-module.cppRepository: shader-slang/slang
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- complete prelink flow ---'
sed -n '2594,2725p' source/slang/slang-ir-link.cpp
printf '%s\n' '--- decoration cloning and pointer-literal producers ---'
rg -n -C 16 \
'cloneDecorations\(|addHighLevelDeclDecoration\(|getPtrValue\(' \
source/slang --glob '*.cpp' --glob '*.h'
printf '%s\n' '--- all prelink callers and module serialization callers ---'
rg -n -C 12 \
'prelinkIR\(' source/slang --glob '*.cpp' --glob '*.h'
rg -n -C 12 \
'SerialContainerUtil::write\(|writeSerializedModuleIR\(' source/slang --glob '*.cpp' --glob '*.h'
printf '%s\n' '--- exact IR literal serialization implementation ---'
sed -n '454,520p' source/slang/slang-serialize-ir.cpp
sed -n '521,655p' source/slang/slang-serialize-ir.cppRepository: shader-slang/slang
Length of output: 50374
Strip non-null AST pointers before persisting prelinked IR.
When prelinkIR clones an external symbol with HighLevelDeclDecoration, cloneDecorations reaches this fallback and copies its Decl* into kIROp_PtrLit. IR serialization stores that pointer as a raw address and deserialization restores the same address. A persisted prelinked module can therefore retain a dangling AST pointer after reload or AST-session destruction. Add a stripping boundary before persistence.
🧰 Tools
🪛 GitHub Actions: Check Formatting (comment /format to auto-fix) / 0_check-formatting.txt
[error] 393-395: Formatting check failed: clang-format expects the getPtrValue call to be on one line. Run './extras/formatting.sh' or clang-format to fix the formatting.
🪛 GitHub Actions: Check Formatting (comment /format to auto-fix) / check-formatting
[error] 393-395: Formatting check failed: clang-format expects the getPtrValue call to be formatted on a single line. Run the formatting script or clang-format to fix this file.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 🟡 Has issues — 0 bugs, 2 gaps
Preserves non-null IRPtrLit values during IR cloning in IRSpecContext::maybeCloneValue instead of asserting they are null, so front-end-only decorations that stash an AST Decl* (for diagnostics) survive prelinkIR. Four reviewers (code-quality, IR-correctness, security, test-coverage) confirmed the fix is memory-safe and principled — it brings this path into line with slang-ir-clone.cpp:72, and getPtrValue(type, nullptr) is bit-identical to the old getNullPtrValue(type), so the null case is unchanged. Two non-blocking gaps remain around the relaxed invariant and test coverage.
Changes Overview
PtrLit cloning during linking (source/slang/slang-ir-link.cpp)
- Before: the
kIROp_PtrLitcase inmaybeCloneValueassertedc->value.ptrVal == nullptrand returned a null pointer literal, crashing (#12728) whenprelinkIRimported a source function whose IR still carried a front-end-onlyIRHighLevelDeclDecoration(non-nullDecl*). - After: forwards the pointer value via
builder->getPtrValue(cloneType(...), c->value.ptrVal). The decoration is stripped bystripFrontEndOnlyInstructionsbefore target legalization/emit, so the AST pointer never reaches codegen or serialization.
Findings (2 total)
| Severity | Location | Finding |
|---|---|---|
| 🟡 Gap | source/slang/slang-ir-link.cpp:392 |
Hard invariant dropped for the codegen link too; comment's correctness argument doesn't name the strip pass, and no assert guards an unexpected future producer |
| 🟡 Gap | source/slang/slang-ir-link.cpp:396 |
Changed branch is unreachable in the current tree and has no test; regression test should land with/gated on PR #12691 |
reviewed: 554d5b5 · diff sha256 ded4a137b19d
| // Non-null pointer literals are used by front-end-only decorations to retain an AST | ||
| // declaration for diagnostics. Linking stays within the same compiler session, so the | ||
| // declaration remains valid and must be preserved until the mandatory passes consume | ||
| // it. Front-end-only instructions are stripped before target legalization and emit. |
There was a problem hiding this comment.
🟡 Gap: relaxed ptr-lit path drops a hard invariant; comment's correctness argument isn't verifiable, and the guard is removed for the codegen link too
This replaces SLANG_RELEASE_ASSERT(c->value.ptrVal == nullptr) with a path that forwards any pointer value. The correctness of that now rests entirely on this comment, and two load-bearing invariants are stated abstractly:
- the only producer of a non-null
IRPtrLitreaching this clone path is a front-end-only decoration (in practiceIRHighLevelDeclDecorationviaaddHighLevelDeclDecoration,slang-ir.cpp:7393); - those instructions are always stripped before target legalization/emit, so the raw AST
Decl*is never dereferenced downstream.
Naming the pass that guarantees (2) — stripFrontEndOnlyInstructions / _shouldStripInst in slang-ir-strip.cpp:18, run in generateIRForTranslationUnit before serialization/codegen — would let a maintainer verify the lifetime claim without a codebase search.
This branch is shared by both prelinkIR (where the non-null value legitimately occurs, same session, AST alive) and the final codegen linkIR (which today only sees already-stripped modules). Removing the assert therefore also removes the loud-failure net for the codegen link: if a future front-end-only decoration carrying a Decl* were ever added to the IR but missed by the strip list, a non-null IRPtrLit would now be silently propagated into the emitted module (emit backends interpret ptrVal as a real address) instead of asserting.
Suggestion: keep the fix, but consider guarding the invariant rather than dropping it — e.g. a targeted SLANG_ASSERT/SLANG_RELEASE_ASSERT in the final-codegen (isFinalCodegenLink) case that non-null IRPtrLit values do not reach emit, so an unexpected future producer fails loudly here (per CLAUDE.md "Fail loudly on out-of-contract input" / "assert the invariant"). At minimum, name the strip pass in the comment.
| return builder->getPtrValue( | ||
| cloneType(this, c->getFullType()), | ||
| c->value.ptrVal); |
There was a problem hiding this comment.
🟡 Gap: changed linking behavior ships with no reachable path and no test
In the current tree this new branch is dead: the only producer of a non-null IRPtrLit is IRHighLevelDeclDecoration, which _shouldStripInst (slang-ir-strip.cpp:18) strips during stripFrontEndOnlyInstructions, run in generateIRForTranslationUnit before the module reaches linkIR. So no .slang compile / -cpu / INTERPRET test can reach it, and the old SLANG_RELEASE_ASSERT could never fire on any current input. The reproduction depends on draft PR #12691 introducing a front-end-only decoration that survives to link time.
That is a fair reason not to add a test here, but merging behavior that no code path and no test exercises means a later refactor could silently break it (or revert it to the null-assert) with nothing failing in CI.
Suggestion: land the regression test together with (or gated on) PR #12691 — a .slang test using its structural ray-tracing pipeline compiled far enough to run linkIR (e.g. -target spirv/-target hlsl) would have hit the old assert and pins this fix. If a smaller pin is wanted sooner, a C++ unit test that builds a tiny IRModule with a decoration whose operand is a non-null IRPtrLit and runs the clone/link path (asserting ptrVal survives, mirroring slang-ir-clone.cpp:72) is an option, though the link entry points are internal and would need a test hook.
2 participants
Fixes #12728.
Non-null
IRPtrLitvalues carry AST declaration pointers for front-end-only diagnosticdecorations. Linking happens within the same compiler session, so preserve those values while
cloning IR instead of asserting that every pointer literal is null. The existing front-end strip
still removes these declarations before target legalization and emission.
The blocking reproduction is the structural ray-tracing runtime pipeline being developed in draft
PR #12691: composing its explicit raygen, closest-hit, and miss components previously asserted
during Vulkan pipeline creation. That pipeline is used to validate this fix because the structural
entry synthesis that exposes the defect is still under review.