Skip to content

Fix #12728: preserve front-end pointer literals during linking #12729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kaizhangNV wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix #12728: preserve front-end pointer literals during linking #12729

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions source/slang/slang-ir-link.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,8 +389,13 @@ IRInst* IRSpecContext::maybeCloneValue(IRInst* originalValue)
case kIROp_PtrLit:
{
IRConstant* c = (IRConstant*)originalValue;
SLANG_RELEASE_ASSERT(c->value.ptrVal == nullptr);
return builder->getNullPtrValue(cloneType(this, c->getFullType()));
// 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.
Comment on lines +392 to +395

@github-actions github-actions Bot Aug 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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:

  1. the only producer of a non-null IRPtrLit reaching this clone path is a front-end-only decoration (in practice IRHighLevelDeclDecoration via addHighLevelDeclDecoration, slang-ir.cpp:7393);
  2. 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);
Comment on lines +392 to +398

@coderabbitai coderabbitai Bot Aug 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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/**' || true

Repository: 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.cpp

Repository: 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.cpp

Repository: 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.

Comment on lines +396 to +398

@github-actions github-actions Bot Aug 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

}
break;

Expand Down
Loading