Fix #11487: lookup inherited witness-table entries in dynamic dispatch#11492
Open
nv-slang-bot[bot] wants to merge 3 commits into
Open
Fix #11487: lookup inherited witness-table entries in dynamic dispatch#11492nv-slang-bot[bot] wants to merge 3 commits into
nv-slang-bot[bot] wants to merge 3 commits into
Conversation
The dynamic-dispatch specialization pass crashed inside _replaceInstUsesWith when an existential typed as a derived interface called a base interface's default method whose body dispatches via sibling `this` methods. The crash signature was a SLANG_ASSERT(other) firing because findWitnessTableEntry returned nullptr for inherited requirement keys: the AST conformance checker stores inherited requirements on a *nested* base-interface witness table that hangs off the parent table's inheritance entry, but the lookup was flat. Adds findWitnessTableEntryInInheritanceClosure in slang-ir-util that walks nested witness-table-valued entries along the inheritance chain when the key isn't a direct entry of the parent table. Two safety constraints on the walk: - Recursion is restricted to nested tables whose `getConcreteType()` matches the outermost table's. Inheritance chains preserve the concrete type at every step (`Foo : IDerived` -> `Foo : IBase`), so every inherited entry remains reachable. Associated-conformance entries (e.g. an entry on `Foo : IDerived` whose nested table is `Bar : IBase` because `IDerived` requires an associated type `A : IBase`) carry a different concrete type and are not traversed: IR requirement keys are globally-unique IRStructKeys and can collide across the two nested tables, so an unconstrained walk could otherwise return an associated-type's IBase implementation in place of `Self : IBase`. - A visited set guards termination because IR witness tables are not always trees: buildDifferentiablePairWitness intentionally creates self-referential entries on differential-pair conformances. Wires the helper into the five lookup sites that were vulnerable to the inherited-key miss: - specializeWitnessLookup at slang-ir-translate.cpp (the actual crash on the issue repro, reached via TranslationContext::resolveInst from TypeFlowSpecializationContext::analyzeCall) - specializeLookupWitnessMethod, analyzeLookupWitnessMethod, and the Lookup branch of getDispatcher at slang-ir-typeflow-specialize.cpp - lowerGetTagForMappedSet at slang-ir-lower-dynamic-dispatch-insts.cpp, to keep the lowering's source-side lookup consistent with the inheritance-closure-resolved destination set the typeflow specializer builds. Each site keeps its existing flow shape but now succeeds for inherited keys. Defensive null-handling mirrors the maybeSpecializeWitnessLookup pattern in slang-ir-specialize.cpp:1219 so a residual miss leaves the lookup unspecialized rather than tripping the assertion. The dispatcher's Lookup action carries a SLANG_RELEASE_ASSERT for the residual-miss case (with an in-source comment naming the precondition: analyzeLookupWitnessMethod has already validated each set element against the same key via the same closure walk) so any future regression surfaces with a clear message rather than a cast crash on the next dispatch action. Two adjacent passes (slang-ir-specialize.cpp's static specializer and slang-ir-inline.cpp's resolveLookups) intentionally remain on flat lookup; their miss paths leave the lookup intact and the typeflow specializer's closure walk resolves inherited keys later. Comments at those sites document the deliberate scope discipline. Tests: - tests/language-feature/dynamic-dispatch/dispatch-default-method-inherited.slang exercises the issue repro shape (createDynamicObject<IBSDF> with Lambert and Ggx conformances) on -cpu compute (behavior validation for both conformances) plus SPIR-V/HLSL/GLSL compile-only (crash coverage on each of the three target backends called out in the original report). - tests/language-feature/dynamic-dispatch/dispatch-default-method-inherited-3level.slang forces depth-2 traversal of the inheritance closure walk via a 3-level chain ITop : IMid : IBase, with the default method on the deepest base dispatched through an ITop existential. - tests/language-feature/dynamic-dispatch/dispatch-default-method-with-assoc-type.slang exercises the concrete-type-filter contract: a derived interface inherits from IBase AND requires an associated type that itself conforms to IBase. The walk must select the inheritance-chain entry, not the associated-conformance entry, and the test verifies this by observing that defaultDouble() on each conforming struct returns twice its own getValue() rather than 2 * Helper::getValue().
nv-slang-bot
Bot
force-pushed
the
fix/issue-11487
branch
from
5835799 to
8a001ff
Compare
szihs
changed the title
Collaborator
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
Contributor
|
Worried about impact? Review this PR in Change Stack to explore blast radius before you approve or request changes. Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request fixes a segfault during dynamic-dispatch specialization of inherited default interface methods by introducing a new witness-table inheritance-closure lookup that recursively walks nested tables to resolve inherited interface requirements while avoiding associated-type conformance paths. ChangesWitness-table inheritance-closure resolution for dynamic dispatch
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aea857f7-2dc6-4535-8592-a16ea4c9d11b
📒 Files selected for processing (10)
source/slang/slang-ir-inline.cppsource/slang/slang-ir-lower-dynamic-dispatch-insts.cppsource/slang/slang-ir-specialize.cppsource/slang/slang-ir-translate.cppsource/slang/slang-ir-typeflow-specialize.cppsource/slang/slang-ir-util.cppsource/slang/slang-ir-util.htests/language-feature/dynamic-dispatch/dispatch-default-method-inherited-3level.slangtests/language-feature/dynamic-dispatch/dispatch-default-method-inherited.slangtests/language-feature/dynamic-dispatch/dispatch-default-method-with-assoc-type.slang
Comment on lines
+3431
to
+3438
| // Walk nested base-interface witness tables for inherited | ||
| // requirements (#11487). Skip on a residual miss so a stray | ||
| // null does not land in the result set. An empty result set | ||
| // emits poison downstream (specializeLookupWitnessMethod's | ||
| // isEmpty branch); a non-empty set is specialized normally. | ||
| if (auto satisfyingVal = | ||
| findWitnessTableEntryInInheritanceClosure(witnessTab, key)) | ||
| results.add(satisfyingVal); |
Contributor
There was a problem hiding this comment.
Don't silently drop miss branches from the mapped result set.
If one table misses this closure lookup while another hits, results shrinks and can even collapse to a singleton. The singleton path starting at Line 5814 then replaces the dynamic lookup with the surviving entry for every runtime tag, so a partial miss here becomes a wrong-dispatch specialization instead of a deferred lookup or an internal-consistency failure.
Suggested fix
+ bool sawResidualMiss = false;
forEachInSet(
module,
cast<IRWitnessTableSet>(elementOfSetType->getSet()),
[&](IRInst* table)
{
@@
- if (auto satisfyingVal =
- findWitnessTableEntryInInheritanceClosure(witnessTab, key))
- results.add(satisfyingVal);
+ if (auto satisfyingVal =
+ findWitnessTableEntryInInheritanceClosure(witnessTab, key))
+ {
+ results.add(satisfyingVal);
+ }
+ else
+ {
+ sawResidualMiss = true;
+ }
});
+
+ if (sawResidualMiss)
+ {
+ module->getContainerPool().free(&results);
+ return none();
+ }
Comment on lines
+5789
to
+5795
| // Walk inherited witness tables (#11487); leave unspecialized on a | ||
| // miss so we don't call replaceUsesWith(null). | ||
| auto satisfyingVal = | ||
| findWitnessTableEntryInInheritanceClosure(witnessTable, inst->getRequirementKey()); | ||
| if (!satisfyingVal) | ||
| return false; | ||
| inst->replaceUsesWith(satisfyingVal); |
Contributor
There was a problem hiding this comment.
Fail loudly on a concrete-table miss here.
Once inst->getWitnessTable() is already a concrete IRWitnessTable, no later typeflow step can make this lookup start succeeding. Returning false leaves the unresolved lookupWitnessMethod in IR, and the post-pass walker starting at Line 3518 skips concrete witness-table operands, so this path silently escapes the local diagnostic machinery.
Suggested fix
auto satisfyingVal =
findWitnessTableEntryInInheritanceClosure(witnessTable, inst->getRequirementKey());
- if (!satisfyingVal)
- return false;
+ SLANG_RELEASE_ASSERT(satisfyingVal);
inst->replaceUsesWith(satisfyingVal);analyzeLookupWitnessMethod: a partial closure-walk miss could shrink the result set to a strict subset of the runtime tags. Downstream specialization would then pick the singleton-shortcut path and replace the dynamic lookup with the surviving entry for every tag - silently mis-dispatching the tags whose tables actually missed. Track residual misses and return none() so the lookup stays dynamic instead. specializeLookupWitnessMethod: once the lookup operand is already a concrete IRWitnessTable, no later typeflow step can resurrect a missing entry, and the post-pass diagnostic walker skips concrete-table operands. Returning false therefore left an unresolved lookupWitnessMethod that escaped the local diagnostic machinery. Promote the residual-miss case to a release assert so any future regression surfaces with a clear message instead of silent IR corruption.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A P1 segfault in dynamic-dispatch specialization fired when an existential typed as a derived interface (e.g.
IBSDF : IShadingFunction) was constructed viacreateDynamicObject<IBSDF>(typeId, 0)and called the inherited default methodsample_evalwhose body dispatches via siblingthismethods. The crash hit on-target {spirv,hlsl,glsl}with the stack_replaceInstUsesWith→TranslationContext::resolveInst→TypeFlowSpecializationContext::analyzeCall, asserting becausefindWitnessTableEntryreturnednullptrfor the inherited requirement key.Diagnosis
The AST conformance checker (
SemanticsVisitor::findWitnessForInterfaceRequirement) stores inherited interface requirements on a nested base-interface witness table that hangs off the parent's inheritance entry. Lowering (lowerWitnessTable'sRequirementWitness::Flavor::witnessTablebranch) emits this as a childIRWitnessTablereachable through the parent's witness-table-valued entries. Multiple call sites offindWitnessTableEntrydid flat-only lookup and crashed on the resulting null whenever the requirement key originated from an inherited base interface.The triage memo flagged
slang-ir-typeflow-specialize.cpp:5777as the crash site, but-dump-irverification showed the actual crash was inspecializeWitnessLookup(inslang-ir-translate.cpp); both the typeflow specializer and translation needed the fix.Approach
Adds
findWitnessTableEntryInInheritanceClosureinslang-ir-utilthat walks nested witness-table-valued entries when the key isn't a direct entry of the parent table, with two safety constraints:getConcreteType()matches the outermost table's. Inheritance chains preserve the concrete type at every step (Foo : IDerived→Foo : IBase), so every inherited entry remains reachable. Associated-conformance entries (e.g. an entry onFoo : IDerivedwhose nested table isBar : IBasebecauseIDerivedrequires an associated typeA : IBase) carry a different concrete type and are not traversed — IR requirement keys are globally-uniqueIRStructKeys and can collide across the two nested tables, so an unconstrained walk could otherwise return an associated-type's IBase implementation in place ofSelf : IBase.buildDifferentiablePairWitnessintentionally creates self-referential entries on differential-pair conformances, so an unguarded recursion would stack-overflow on differentiable code.Wired through every flat-lookup site whose key can come from an inherited interface (5 sites across 3 files):
specializeWitnessLookupinslang-ir-translate.cpp— the actual crash site for the issue repro.specializeLookupWitnessMethod,analyzeLookupWitnessMethod, and theLookupaction ingetDispatcherinslang-ir-typeflow-specialize.cpp.lowerGetTagForMappedSetinslang-ir-lower-dynamic-dispatch-insts.cpp— keeps the lowering's source-side lookup consistent with the inheritance-closure-resolved destination set the typeflow specializer builds.Miss-handling differs by site, matching each site's role:
specializeWitnessLookupinslang-ir-translate.cpp, plusspecializeLookupWitnessMethodandanalyzeLookupWitnessMethodinslang-ir-typeflow-specialize.cpp) skip the rewrite on a residual miss, mirroring the existing pattern inmaybeSpecializeWitnessLookup(theif (!satisfyingVal) return false;guard atslang-ir-specialize.cpp:1234). The inst is left in place for a later pass.Lookupaction ingetDispatcher(slang-ir-typeflow-specialize.cpp) andlowerGetTagForMappedSetinslang-ir-lower-dynamic-dispatch-insts.cppare consistency checks downstream of the analyzers above — by the time those run, every set element has already been validated against the same key via the same constrained walk. A residual miss there is internally-corrupt IR, so each fails loudly: the dispatcher withSLANG_RELEASE_ASSERT(val)(in-source comment names the precondition), and the lowering with the existingSLANG_UNEXPECTED("Element not found in destination collection"). Either surfaces with a clear message instead of a downstream cast crash.The constrained walk strengthens both guards: an unrelated associated-type entry can no longer mask a real miss, so the assertions only fire on genuinely-corrupt IR.
Two adjacent passes (
slang-ir-specialize.cpp's static specializer andslang-ir-inline.cpp'sresolveLookups) intentionally remain on flat lookup. Their miss paths leave the lookup intact and the typeflow specializer's closure walk resolves inherited keys later. Comments at those sites document the deliberate scope discipline so future readers don't have to rediscover it.Alternatives considered and rejected:
slang-emit-*) — the IR-pass philosophy keeps emission simple; the lookup gap belongs in the dispatch-specialization passes.Note on the type comparison:
getConcreteType()is compared via pointer equality, matching existing local practice inslang-ir-autodiff.cpp:274-275and elsewhere. If future generic/specialized witness-table lowering ever creates semantically-identical but non-identical IR type nodes, this helper would miss inherited entries. Worth flagging but not blocking — would surface as a CI regression and be addressed by structural type comparison in a follow-up.Files changed
10 files, +403/−8.
Helper:
source/slang/slang-ir-util.h— declaration + contract.source/slang/slang-ir-util.cpp— recursive walk implementation with concrete-type filter and cycle guard.Wiring (closure walk replaces flat lookup):
source/slang/slang-ir-translate.cppsource/slang/slang-ir-typeflow-specialize.cpp(3 sites)source/slang/slang-ir-lower-dynamic-dispatch-insts.cppComment-only / intentionally flat:
source/slang/slang-ir-specialize.cpp(2 spots)source/slang/slang-ir-inline.cpp(1 spot)Tests:
tests/language-feature/dynamic-dispatch/dispatch-default-method-inherited.slang— issue repro shape (createDynamicObject<IBSDF>with Lambert and Ggx; cpu compute + spirv-asm/hlsl/glsl compile-only).tests/language-feature/dynamic-dispatch/dispatch-default-method-inherited-3level.slang— depth-2 inheritance closure walk viaITop : IMid : IBase.tests/language-feature/dynamic-dispatch/dispatch-default-method-with-assoc-type.slang— concrete-type-filter contract: a derived interface that inheritsIBaseAND requires an associated typeA : IBase; the walk must select the inheritance-chain entry, not the associated-conformance entry.Tests
dispatch-default-method-inherited.slang— 4/4 directives PASS (cpu compute behavior validation + spirv-asm/hlsl/glsl crash coverage).dispatch-default-method-inherited-3level.slang— 1/1 PASS.dispatch-default-method-with-assoc-type.slang— 1/1 PASS (verifies the walk selectsSelf : IBase's entry, not the associated-type's IBase witness).tests/language-feature/dynamic-dispatch/— 517/517 pass with the three new regression files included; 0 failures.tests/language-feature/interfaces/— 31/31 pass.tests/autodiff/— 365/365 pass (confirms the cycle-guard + concrete-type filter don't disturbbuildDifferentiablePairWitness's self-referential paths).clang-format,gersemi,shfmt) are not installed in this environment, so./extras/formatting.shcould not run end-to-end locally. Maintainers should treat CI's format check as the first authoritative gate.Risk
buildDifferentiablePairWitnessself-references) won't stack-overflow and associated-conformance entries can't shadow inheritance entries.findWitnessTableEntry's body insource/slang/slang-ir-autodiff.cpp:25-30(pre-existing tech debt unrelated to this crash; left for a follow-up).Closes #11487.
🤖 Generated by an automated Slang coworker — may be inaccurate. A human maintainer should verify.