Add a diagnostic check to warn when special types leak from param groups#10158
Add a diagnostic check to warn when special types leak from param groups#10158zangold-nv wants to merge 1 commit intoshader-slang:masterfrom
Conversation
zangold-nv
force-pushed
the
rtas-cbuff-leak
branch
from
9d26d32 to
04f49e6
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds two diagnostics and emits them during type legalization when parameter-group element types (e.g., RTAS) cannot stay in the same binding, plus a test exercising a push_constant cbuffer that triggers the warnings. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/slang/slang-legalize-types.cpp`:
- Around line 1227-1244: The loop assumes
legalElementType.getPair()->specialType is a tuple and can crash; before
iterating its .getTuple()->elements, check/normalize the specialType flavor
(e.g., confirm specialType.flavor == LegalType::Flavor::tuple or re-run
legalizeType/unwrap implicit-deref) and only iterate when it is actually a
tuple; otherwise skip the element iteration or handle the non-tuple case (e.g.,
diagnose a single non-tuple element) so getTuple() cannot assert. Ensure you
reference legalElementType, LegalType::Flavor::pair, getPair(), specialType,
getTuple(), and the diagnostic calls (context->m_sink->diagnose /
findFirstUseLoc) when adding the guard.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
source/slang/slang-diagnostic-defs.hsource/slang/slang-legalize-types.cpptests/diagnostics/rtas-cbuff-leak.slang
zangold-nv
force-pushed
the
rtas-cbuff-leak
branch
from
04f49e6 to
f9abac9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/diagnostics/rtas-cbuff-leak.slang.expected`:
- Around line 3-8: Update the diagnostic expected output to reference the actual
test input filename: in the expected output file change both occurrences of
"shader.slang" to "rtas-cbuff-leak.slang" so the two warning lines match the
test filename (the lines containing "warning 31106" and "warning 31107" that
currently start with "shader.slang(3)" and "shader.slang(5)"); ensure only the
filename text is replaced and the rest of each diagnostic line remains
unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
source/slang/slang-diagnostic-defs.hsource/slang/slang-legalize-types.cpptests/diagnostics/rtas-cbuff-leak.slangtests/diagnostics/rtas-cbuff-leak.slang.expected
🚧 Files skipped from review as they are similar to previous changes (2)
- source/slang/slang-legalize-types.cpp
- tests/diagnostics/rtas-cbuff-leak.slang
zangold-nv
force-pushed
the
rtas-cbuff-leak
branch
from
f9abac9 to
5c09841
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/diagnostics/rtas-cbuff-leak.slang.expected (1)
3-7:⚠️ Potential issue | 🟠 MajorFix expected diagnostic filename to match the test source.
The expected output still referencesshader.slang; it should referencertas-cbuff-leak.slangto align with the actual test file.🩹 Suggested fix
-shader.slang(5): warning 31106: Parameter group type includes some members with types which cannot be included in the same binding. These types will be moved into another parameter binding slot. +rtas-cbuff-leak.slang(5): warning 31106: Parameter group type includes some members with types which cannot be included in the same binding. These types will be moved into another parameter binding slot. ... -shader.slang(7): warning 31107: Structure member will leak into separate binding. +rtas-cbuff-leak.slang(7): warning 31107: Structure member will leak into separate binding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/diagnostics/rtas-cbuff-leak.slang.expected` around lines 3 - 7, Update the expected diagnostic file to reference the actual test source name: open tests/diagnostics/rtas-cbuff-leak.slang.expected and replace occurrences of "shader.slang" with "rtas-cbuff-leak.slang" so the diagnostic lines (e.g., the warning lines shown) match the test input filename and the test will compare against the correct source.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/diagnostics/rtas-cbuff-leak.slang.expected`:
- Around line 3-7: Update the expected diagnostic file to reference the actual
test source name: open tests/diagnostics/rtas-cbuff-leak.slang.expected and
replace occurrences of "shader.slang" with "rtas-cbuff-leak.slang" so the
diagnostic lines (e.g., the warning lines shown) match the test input filename
and the test will compare against the correct source.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
source/slang/slang-diagnostic-defs.hsource/slang/slang-legalize-types.cpptests/diagnostics/rtas-cbuff-leak.slangtests/diagnostics/rtas-cbuff-leak.slang.expected
🚧 Files skipped from review as they are similar to previous changes (1)
- source/slang/slang-legalize-types.cpp
zangold-nv
force-pushed
the
rtas-cbuff-leak
branch
from
5c09841 to
0eb8781
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
source/slang/slang-legalize-types.cpp (1)
1237-1240:⚠️ Potential issue | 🟠 MajorGuard
specialType.getTuple()with a flavor check.Line 1238 assumes the special type is always a tuple; if it isn’t (e.g., implicit-deref wrapped),
getTuple()can assert. Add a flavor guard/unwrapping before iterating.🛠️ Suggested defensive guard
- for (auto specialElement : - legalElementType.getPair()->specialType.getTuple()->elements) + auto specialType = legalElementType.getPair()->specialType; + if (specialType.flavor == LegalType::Flavor::implicitDeref) + specialType = specialType.getImplicitDeref()->valueType; + if (specialType.flavor == LegalType::Flavor::tuple) + { + for (auto specialElement : specialType.getTuple()->elements) + { + context->m_sink->diagnose( + specialElement.key, + Diagnostics::specialTypeLeaksFromParameterGroupElement); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/slang/slang-legalize-types.cpp` around lines 1237 - 1240, The loop assumes legalElementType.getPair()->specialType is a Tuple; before calling getTuple() check the type flavor (e.g. test specialType.getFlavor() == TypeFlavor::Tuple or use an isTuple()/asTuple() helper) and if the specialType is a wrapper like implicit-deref, unwrap it (follow the decorated/inner type or call the canonical/underlying type) until you have the Tuple flavor, then iterate its elements; update the code around legalElementType.getPair()->specialType.getTuple()->elements to perform this guard/unwrap to avoid assertions when specialType is not a tuple.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/slang/slang-legalize-types.cpp`:
- Around line 1229-1245: The check for leaking special types only handles
LegalType::Flavor::pair, so when legalElementType.flavor ==
LegalType::Flavor::tuple no warning is emitted; update the condition around the
diagnostic (the block using legalElementType, findFirstUseLoc(type), and
context->m_sink->diagnose with Diagnostics::specialTypeLeaksFromParameterGroup)
to also handle LegalType::Flavor::tuple (e.g., change the if to test for pair OR
tuple or switch on legalElementType.flavor), and when flavor is tuple iterate
over legalElementType.getTuple()->specialType.getTuple()->elements (instead of
getPair()) to emit Diagnostics::specialTypeLeaksFromParameterGroupElement for
each specialElement.key so users see the warning in the all-special/tuple-only
case.
---
Duplicate comments:
In `@source/slang/slang-legalize-types.cpp`:
- Around line 1237-1240: The loop assumes
legalElementType.getPair()->specialType is a Tuple; before calling getTuple()
check the type flavor (e.g. test specialType.getFlavor() == TypeFlavor::Tuple or
use an isTuple()/asTuple() helper) and if the specialType is a wrapper like
implicit-deref, unwrap it (follow the decorated/inner type or call the
canonical/underlying type) until you have the Tuple flavor, then iterate its
elements; update the code around
legalElementType.getPair()->specialType.getTuple()->elements to perform this
guard/unwrap to avoid assertions when specialType is not a tuple.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
source/slang/slang-diagnostic-defs.hsource/slang/slang-legalize-types.cpptests/diagnostics/rtas-cbuff-leak.slang
🚧 Files skipped from review as they are similar to previous changes (2)
- source/slang/slang-diagnostic-defs.h
- tests/diagnostics/rtas-cbuff-leak.slang
| // When special types leak out of a parameter group, they need to | ||
| // be bound differently. Warn the user when this happens. | ||
| if (legalElementType.flavor == LegalType::Flavor::pair) | ||
| { | ||
| context->m_sink->diagnose( | ||
| findFirstUseLoc(type), | ||
| Diagnostics::specialTypeLeaksFromParameterGroup); | ||
|
|
||
| // indicate which elements cannot be part of the parameter group | ||
| for (auto specialElement : | ||
| legalElementType.getPair()->specialType.getTuple()->elements) | ||
| { | ||
| context->m_sink->diagnose( | ||
| specialElement.key, | ||
| Diagnostics::specialTypeLeaksFromParameterGroupElement); | ||
| } | ||
| } |
There was a problem hiding this comment.
Warn when legalization yields a tuple-only leak.
If the element type legalizes to LegalType::Flavor::tuple (e.g., struct with only special fields), leakage occurs but no warning is emitted because the check is pair-only. Consider covering tuple as well so users get the warning in the “all-special” case.
✅ Suggested coverage expansion
- if (legalElementType.flavor == LegalType::Flavor::pair)
+ if (legalElementType.flavor == LegalType::Flavor::pair ||
+ legalElementType.flavor == LegalType::Flavor::tuple)
{
context->m_sink->diagnose(
findFirstUseLoc(type),
Diagnostics::specialTypeLeaksFromParameterGroup);
- // indicate which elements cannot be part of the parameter group
- for (auto specialElement :
- legalElementType.getPair()->specialType.getTuple()->elements)
+ // indicate which elements cannot be part of the parameter group
+ LegalType specialType = (legalElementType.flavor == LegalType::Flavor::pair)
+ ? legalElementType.getPair()->specialType
+ : legalElementType;
+ if (specialType.flavor == LegalType::Flavor::implicitDeref)
+ specialType = specialType.getImplicitDeref()->valueType;
+ if (specialType.flavor == LegalType::Flavor::tuple)
{
- context->m_sink->diagnose(
- specialElement.key,
- Diagnostics::specialTypeLeaksFromParameterGroupElement);
+ for (auto specialElement : specialType.getTuple()->elements)
+ {
+ context->m_sink->diagnose(
+ specialElement.key,
+ Diagnostics::specialTypeLeaksFromParameterGroupElement);
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/slang/slang-legalize-types.cpp` around lines 1229 - 1245, The check
for leaking special types only handles LegalType::Flavor::pair, so when
legalElementType.flavor == LegalType::Flavor::tuple no warning is emitted;
update the condition around the diagnostic (the block using legalElementType,
findFirstUseLoc(type), and context->m_sink->diagnose with
Diagnostics::specialTypeLeaksFromParameterGroup) to also handle
LegalType::Flavor::tuple (e.g., change the if to test for pair OR tuple or
switch on legalElementType.flavor), and when flavor is tuple iterate over
legalElementType.getTuple()->specialType.getTuple()->elements (instead of
getPair()) to emit Diagnostics::specialTypeLeaksFromParameterGroupElement for
each specialElement.key so users see the warning in the all-special/tuple-only
case.
0eb8781 to
0e722c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
source/slang/slang-legalize-types.cpp (1)
1231-1247:⚠️ Potential issue | 🟠 MajorTuple-only leak (all-special element struct) produces no diagnostic.
When
originalElementTypeis a struct whose every field is special (e.g., a struct containing onlyRaytracingAccelerationStructuremembers),TupleTypeBuilder::getResult()returnsLegalType::Flavor::tupledirectly (not apair). The guard at line 1231 is never entered, so neitherspecialTypeLeaksFromParameterGroupnorspecialTypeLeaksFromParameterGroupElementfires, silently dropping all fields from the binding.🔧 Suggested fix to cover the all-special case
- if (legalElementType.flavor == LegalType::Flavor::pair) + if (legalElementType.flavor == LegalType::Flavor::pair || + legalElementType.flavor == LegalType::Flavor::tuple) { context->m_sink->diagnose( findFirstUseLoc(type), Diagnostics::specialTypeLeaksFromParameterGroup); - // indicate which elements cannot be part of the parameter group - if (auto specialTuple = legalElementType.getPair()->specialType.getTuple()) + LegalType specialType = (legalElementType.flavor == LegalType::Flavor::pair) + ? legalElementType.getPair()->specialType + : legalElementType; + if (specialType.flavor == LegalType::Flavor::tuple) { - for (auto specialElement : specialTuple->elements) + for (auto specialElement : specialType.getTuple()->elements) { context->m_sink->diagnose( specialElement.key, Diagnostics::specialTypeLeaksFromParameterGroupElement); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/slang/slang-legalize-types.cpp` around lines 1231 - 1247, The current code only diagnoses when legalElementType.flavor == LegalType::Flavor::pair and misses the case when the legalized element is a tuple (all-special fields), so add similar handling for LegalType::Flavor::tuple: when legalElementType.flavor == LegalType::Flavor::tuple, call context->m_sink->diagnose(findFirstUseLoc(type), Diagnostics::specialTypeLeaksFromParameterGroup) and then iterate the tuple returned by legalElementType.getTuple(), emitting Diagnostics::specialTypeLeaksFromParameterGroupElement for each element key (mirroring the pair branch); place this new branch alongside the existing pair branch so both pair and tuple flavors produce the same diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/slang/slang-legalize-types.cpp`:
- Around line 1238-1246: The current code calls specialType.getTuple() via
legalElementType.getPair()->specialType without checking the LegalType flavor,
which can trigger SLANG_ASSERT in getTuple(); update the guard to first test
that legalElementType.getPair()->specialType.flavor == LegalType::Flavor::tuple
before calling getTuple(), then retrieve specialTuple with getTuple() and run
the existing loop that calls context->m_sink->diagnose(...,
Diagnostics::specialTypeLeaksFromParameterGroupElement) for each specialElement.
---
Duplicate comments:
In `@source/slang/slang-legalize-types.cpp`:
- Around line 1231-1247: The current code only diagnoses when
legalElementType.flavor == LegalType::Flavor::pair and misses the case when the
legalized element is a tuple (all-special fields), so add similar handling for
LegalType::Flavor::tuple: when legalElementType.flavor ==
LegalType::Flavor::tuple, call context->m_sink->diagnose(findFirstUseLoc(type),
Diagnostics::specialTypeLeaksFromParameterGroup) and then iterate the tuple
returned by legalElementType.getTuple(), emitting
Diagnostics::specialTypeLeaksFromParameterGroupElement for each element key
(mirroring the pair branch); place this new branch alongside the existing pair
branch so both pair and tuple flavors produce the same diagnostics.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
source/slang/slang-diagnostic-defs.hsource/slang/slang-legalize-types.cpptests/diagnostics/rtas-cbuff-leak.slang
🚧 Files skipped from review as they are similar to previous changes (1)
- source/slang/slang-diagnostic-defs.h
| if (auto specialTuple = legalElementType.getPair()->specialType.getTuple()) | ||
| { | ||
| for (auto specialElement : specialTuple->elements) | ||
| { | ||
| context->m_sink->diagnose( | ||
| specialElement.key, | ||
| Diagnostics::specialTypeLeaksFromParameterGroupElement); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find LegalType::getTuple() implementation
rg -n "getTuple" source/slang/slang-legalize-types.cpp -A 10 -B 2Repository: shader-slang/slang
Length of output: 3327
🏁 Script executed:
#!/bin/bash
# Look for LegalType class definition and getTuple method
ast-grep --pattern 'class LegalType {
$$$
getTuple()
$$$
}'Repository: shader-slang/slang
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Get broader context around lines 1238-1246
sed -n '1220,1260p' source/slang/slang-legalize-types.cppRepository: shader-slang/slang
Length of output: 1975
🏁 Script executed:
#!/bin/bash
# Search for LegalType class definition and getTuple method
rg -n "class LegalType" source/slang/slang-legalize-types.cpp -A 100 | head -200Repository: shader-slang/slang
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search for getTuple() method in the file more broadly
rg -n "::getTuple\(\)" source/slang/slang-legalize-types.cpp -B 5 -A 10Repository: shader-slang/slang
Length of output: 474
🏁 Script executed:
#!/bin/bash
# Look in header files for the definition
fd -e h source/slang/ | xargs rg -l "getTuple" | head -5Repository: shader-slang/slang
Length of output: 645
🏁 Script executed:
#!/bin/bash
# Search for LegalType struct/class definition
rg -n "struct LegalType\|class LegalType" source/slang/ -A 80 | head -300Repository: shader-slang/slang
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search for getTuple in header files
fd . source/slang -type f -name "*.h" | xargs rg "getTuple" -B 2 -A 5Repository: shader-slang/slang
Length of output: 50562
🏁 Script executed:
#!/bin/bash
# Check for LegalType and its getTuple in any file
rg -n "LegalType.*getTuple\|getTuple.*{" source/slang/slang-legalize-types.cpp -B 3 -A 8Repository: shader-slang/slang
Length of output: 189
🏁 Script executed:
#!/bin/bash
# Search for LegalType struct definition more carefully
rg -n "struct LegalType\|class LegalType" source/slang/slang-legalize-types.cpp -A 100 | head -400Repository: shader-slang/slang
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search for getTuple method specifically with better regex
rg -n "getTuple\(\)" source/slang/slang-legalize-types.cpp -B 5 -A 8 | grep -A 8 "LegalType\|LegalElementWrapping"Repository: shader-slang/slang
Length of output: 2086
🏁 Script executed:
#!/bin/bash
# Look for LegalType in header files
fd . source/slang -type f \( -name "*.h" -o -name "*.hpp" \) -exec grep -l "LegalType" {} \; | head -5Repository: shader-slang/slang
Length of output: 232
🏁 Script executed:
#!/bin/bash
# Find header files that might contain LegalType definition
find source/slang -type f -name "*.h" | xargs grep -l "struct LegalType\|class LegalType" 2>/dev/null | head -5Repository: shader-slang/slang
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Look in all header files for getTuple method on LegalType
find source/slang -type f -name "*.h" | xargs grep -A 5 "LegalType.*getTuple\|getTuple.*const" 2>/dev/null | head -100Repository: shader-slang/slang
Length of output: 1548
🏁 Script executed:
#!/bin/bash
# Check the top of slang-legalize-types.cpp for includes to find where LegalType comes from
head -100 source/slang/slang-legalize-types.cpp | grep -E "^#include|LegalType"Repository: shader-slang/slang
Length of output: 788
🏁 Script executed:
#!/bin/bash
# Now examine the LegalType definition in the header file
rg -n "struct LegalType\|class LegalType" source/slang/slang-legalize-types.h -A 150 | head -300Repository: shader-slang/slang
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Specifically look for getTuple on LegalType in the header
rg -n "getTuple" source/slang/slang-legalize-types.h -B 3 -A 8Repository: shader-slang/slang
Length of output: 1177
🏁 Script executed:
#!/bin/bash
# Get broader context around the problematic code to understand the code flow
sed -n '1220,1270p' source/slang/slang-legalize-types.cppRepository: shader-slang/slang
Length of output: 2393
🏁 Script executed:
#!/bin/bash
# Search for where getPair() is called to understand what specialType can be
rg -n "getPair\(\)" source/slang/slang-legalize-types.cpp -B 3 -A 5 | head -100Repository: shader-slang/slang
Length of output: 2976
🏁 Script executed:
#!/bin/bash
# Look at the PairPseudoType structure to understand specialType field
rg -n "struct PairPseudoType\|class PairPseudoType" source/slang/slang-legalize-types.h -A 20Repository: shader-slang/slang
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Get the struct definition for PairPseudoType to see specialType field
rg -n "struct PairPseudoType" source/slang/slang-legalize-types.h -A 15Repository: shader-slang/slang
Length of output: 1180
🏁 Script executed:
#!/bin/bash
# Check if there are any guards on specialType flavor before getTuple() calls elsewhere
rg -n "specialType.*getTuple\|specialType\.flavor" source/slang/slang-legalize-types.cpp | head -30Repository: shader-slang/slang
Length of output: 44
Guard specialType.getTuple() with a flavor check to prevent assertion failures.
The if (auto specialTuple = ...) guard prevents null-dereference but not assertion failures. LegalType::getTuple() contains SLANG_ASSERT(flavor == Flavor::tuple) (line 103 in slang-legalize-types.h), so calling it on a non-tuple flavor will assert in debug builds. Since specialType in a PairPseudoType can have any flavor, add a flavor check before calling getTuple():
if (specialType.flavor == LegalType::Flavor::tuple)
{
auto specialTuple = specialType.getTuple();
for (auto specialElement : specialTuple->elements)
{
context->m_sink->diagnose(
specialElement.key,
Diagnostics::specialTypeLeaksFromParameterGroupElement);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/slang/slang-legalize-types.cpp` around lines 1238 - 1246, The current
code calls specialType.getTuple() via legalElementType.getPair()->specialType
without checking the LegalType flavor, which can trigger SLANG_ASSERT in
getTuple(); update the guard to first test that
legalElementType.getPair()->specialType.flavor == LegalType::Flavor::tuple
before calling getTuple(), then retrieve specialTuple with getTuple() and run
the existing loop that calls context->m_sink->diagnose(...,
Diagnostics::specialTypeLeaksFromParameterGroupElement) for each specialElement.
5c97fb6 to
a84ba98
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
zangold-nv
force-pushed
the
rtas-cbuff-leak
branch
from
a84ba98 to
6b36891
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
github-actions
bot
dismissed
their stale review
Automated review dismissed: bot reviews must use COMMENT only, not APPROVE or REQUEST_CHANGES.
As part of type legalization, certain special types may be removed from
parameter group types. For example, in the following structure:
[vk::push_constants]
cbuffer PushConstants {
int i;
RaytracingAccelerationStructure rtas;
}
`rtas` cannot be included in this structure, and will leak out to a
different binding. Warn the user when this happens.
Fixes shader-slang#8818
zangold-nv
force-pushed
the
rtas-cbuff-leak
branch
from
6b36891 to
e6b0623
Compare
There was a problem hiding this comment.
PR Review: Add diagnostic check for special types leaking from parameter groups
Good PR addressing a real user-facing pain point (#8818). The implementation is cleanly scoped to diagnostics (no behavioral changes to legalization), placed in the right location in legalizeTypeImpl, and includes a test with filecheck validation.
Issue: Missing coverage for all-special-fields structs
The main issue is that LegalType::pair() has a short-circuit at lines 49-52: when ordinaryType.flavor == Flavor::none (i.e., the struct has no ordinary fields), it returns specialType directly — which will be Flavor::tuple, not Flavor::pair. The check at line 1231 (legalElementType.flavor == LegalType::Flavor::pair) will miss this case entirely.
Concretely, a cbuffer containing only resource-type members (e.g., two RaytracingAccelerationStructure fields and nothing else) will silently move all members to different bindings with no warning. See inline comment for a suggested fix.
Minor nit
- Diagnostic 31107 has an empty summary string
"", which is unconventional compared to all other diagnostics in the file. See inline comment.
What looks good
- Correct use of
findFirstUseLoc(type)to get meaningful source locations from IR types - Defensive
specialType.flavor == Flavor::tuplecheck before callinggetTuple()— good practice - Clean test case covering the primary use case with filecheck for both the group-level (31106) and member-level (31107) warnings
- Existing tests that intentionally use resources in cbuffers are properly suppressed with
-warnings-disable 31106,31107
|
|
||
| // When special types leak out of a parameter group, they need to | ||
| // be bound differently. Warn the user when this happens. | ||
| if (legalElementType.flavor == LegalType::Flavor::pair) | ||
| { | ||
| context->m_sink->diagnose(Diagnostics::SpecialTypeLeaksFromParameterGroup{ | ||
| .location = findFirstUseLoc(type)}); | ||
|
|
||
| // indicate which elements cannot be part of the parameter group | ||
| auto& specialType = legalElementType.getPair()->specialType; | ||
| if (specialType.flavor == LegalType::Flavor::tuple) | ||
| { | ||
| auto specialTuple = specialType.getTuple(); | ||
| for (auto specialElement : specialTuple->elements) | ||
| { | ||
| context->m_sink->diagnose( | ||
| Diagnostics::SpecialTypeMemberLeaksFromParameterGroup{ | ||
| .member = specialElement.key}); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing coverage for all-special-fields structs.
LegalType::pair(ordinaryType, specialType, pairInfo) has a short-circuit: when ordinaryType.flavor == Flavor::none, it returns specialType directly (lines 49-52 of this file). This means that when a parameter group struct contains only special types and no ordinary data, TupleTypeBuilder::getResult() will return a Flavor::tuple, not a Flavor::pair.
For example, this would produce no warning:
[vk::push_constant]
cbuffer PushConstants {
RaytracingAccelerationStructure rtas1;
RaytracingAccelerationStructure rtas2;
};Consider also checking for Flavor::tuple:
| // When special types leak out of a parameter group, they need to | |
| // be bound differently. Warn the user when this happens. | |
| if (legalElementType.flavor == LegalType::Flavor::pair) | |
| { | |
| context->m_sink->diagnose(Diagnostics::SpecialTypeLeaksFromParameterGroup{ | |
| .location = findFirstUseLoc(type)}); | |
| // indicate which elements cannot be part of the parameter group | |
| auto& specialType = legalElementType.getPair()->specialType; | |
| if (specialType.flavor == LegalType::Flavor::tuple) | |
| { | |
| auto specialTuple = specialType.getTuple(); | |
| for (auto specialElement : specialTuple->elements) | |
| { | |
| context->m_sink->diagnose( | |
| Diagnostics::SpecialTypeMemberLeaksFromParameterGroup{ | |
| .member = specialElement.key}); | |
| } | |
| } | |
| } | |
| // When special types leak out of a parameter group, they need to | |
| // be bound differently. Warn the user when this happens. | |
| if (legalElementType.flavor == LegalType::Flavor::pair || | |
| legalElementType.flavor == LegalType::Flavor::tuple) | |
| { | |
| context->m_sink->diagnose(Diagnostics::SpecialTypeLeaksFromParameterGroup{ | |
| .location = findFirstUseLoc(type)}); | |
| // indicate which elements cannot be part of the parameter group | |
| LegalType specialType = (legalElementType.flavor == LegalType::Flavor::pair) | |
| ? legalElementType.getPair()->specialType | |
| : legalElementType; | |
| if (specialType.flavor == LegalType::Flavor::tuple) | |
| { | |
| auto specialTuple = specialType.getTuple(); | |
| for (auto specialElement : specialTuple->elements) | |
| { | |
| context->m_sink->diagnose( | |
| Diagnostics::SpecialTypeMemberLeaksFromParameterGroup{ | |
| .member = specialElement.key}); | |
| } | |
| } | |
| } |
It would also be good to add a test case for this edge case once addressed.
| ) | ||
|
|
||
| warning( | ||
| "special-type-member-leaks-from-parameter-group", | ||
| 31107, | ||
| "", |
There was a problem hiding this comment.
Nit: The summary string (third argument) is empty "". Every other diagnostic in this file has a non-empty summary. While the span message provides the user-facing text, the summary is used in diagnostic catalogs / documentation. Consider something like:
| ) | |
| warning( | |
| "special-type-member-leaks-from-parameter-group", | |
| 31107, | |
| "", | |
| warning( | |
| "special-type-member-leaks-from-parameter-group", | |
| 31107, | |
| "member will leak into a separate binding slot", | |
| span { loc = "member:IRInst", message = "This member will leak into a separate binding slot." } | |
| ) |
github-actions
bot
dismissed
their stale review
Automated review dismissed: bot reviews must use COMMENT only, not APPROVE or REQUEST_CHANGES.
1 participant
As part of type legalization, certain special types may be removed from parameter group types. For example, in the following structure:
rtascannot be included in this structure, and will leak out to a different binding. Warn the user when this happens.Fixes #8818