Add default value blob reflection API#11471
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR adds a blob-based reflection API to retrieve serialized default-initializer bytes for variables, deprecates legacy scalar accessors, implements serialization for all reflected types (scalars, special floats, vectors/matrices/arrays/structs/enums), and adds unit tests validating blob semantics and contents. ChangesDefault Value Blob Reflection API
🚥 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 |
|
|
duckdoom5
changed the title
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a reflection API to retrieve default initializer values as raw bytes (“blob”) and validates it via a new unit test covering scalar/aggregate defaults.
Changes:
- Added
spReflectionVariable_GetDefaultValueBlobimplementation to serialize default initializers into a byte blob. - Added C++ wrapper
VariableReflection::getDefaultValueBlob()and deprecated older scalar default-value helpers. - Added a unit test to validate blob contents for scalars, vectors, matrices, arrays, structs, inheritance, and enums.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tools/slang-unit-test/unit-test-link-time-type-reflection.cpp | Adds a unit test that exercises the new default-value blob reflection API across many types. |
| source/slang/slang-reflection-api.cpp | Implements expression/type walking to serialize initializer defaults into an ISlangBlob. |
| include/slang.h | Exposes getDefaultValueBlob() and deprecates older default-value APIs in the C++ wrapper. |
| include/slang-deprecated.h | Declares the new C API entry point alongside deprecated reflection functions. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 523e61ab-a47e-486d-86a7-2be4e0866054
📒 Files selected for processing (4)
include/slang-deprecated.hinclude/slang.hsource/slang/slang-reflection-api.cpptools/slang-unit-test/unit-test-link-time-type-reflection.cpp
![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.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/slang/slang-reflection-api.cpp (1)
3341-3352:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle enum-case variables in
getDefaultValueBlobto keep API contracts consistent.At Line 3994,
spReflectionVariable_GetDefaultValueBlobdowncasts toVarDeclBaseand rejects non-VarDeclBasevalues withSLANG_E_INVALID_ARG. But enum cases are also reflected asSlangReflectionVariableandspReflectionVariable_HasDefaultValuereports them as defaulted (Line 3348-Line 3352). This creates a contract break whereHasDefaultValue()can return true while blob retrieval fails for the same reflected variable kind.🔧 Suggested fix sketch
SLANG_API SlangResult spReflectionVariable_GetDefaultValueBlob(SlangReflectionVariable* inVar, ISlangBlob** outBlob) { if (!inVar || !outBlob) return SLANG_E_INVALID_ARG; *outBlob = nullptr; auto var = convert(inVar); + auto decl = var.getDecl(); + + if (auto enumCaseDecl = as<EnumCaseDecl>(decl)) + { + auto parentEnumDecl = as<EnumDecl>(enumCaseDecl->parentDecl); + if (!parentEnumDecl) + return SLANG_E_INVALID_ARG; + + auto astBuilder = getModule(parentEnumDecl)->getLinkage()->getASTBuilder(); + auto tagType = getTagType(astBuilder, DeclRef<EnumDecl>(parentEnumDecl)); + + List<uint8_t> defaultValueBytes; + if (!appendIntegerConstantDefaultValue(enumCaseDecl->tagVal, tagType, defaultValueBytes)) + return SLANG_E_NOT_AVAILABLE; + + *outBlob = ListBlob::moveCreate(defaultValueBytes).detach(); + return SLANG_OK; + } auto varDeclRef = var.as<VarDeclBase>(); auto varDecl = varDeclRef.getDecl(); if (!varDecl || !varDecl->type.type) return SLANG_E_INVALID_ARG;Also applies to: 3986-3997
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f04649bc-9def-43a7-ad9b-e57fbee991c3
📒 Files selected for processing (5)
include/slang-deprecated.hinclude/slang.hsource/slang/slang-reflection-api.cpptools/slang-unit-test/unit-test-link-time-type-reflection.cpptools/slang-unit-test/unit-test-special-scalar-reflection.cpp
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
duckdoom5
force-pushed
the
reflect-default-value-blob
branch
from
bda158b to
3e4fd53
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (5)
source/slang/slang-reflection-api.cpp (5)
3993-3997:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse a checked decl cast before accessing
VarDeclBasefields.Line 3994 rebinds via
var.as<VarDeclBase>(), which does not validate the underlying decl kind. If a non-VarDeclBasehandle reaches this API, dereferencingvarDecl->typeat Line 3996 is undefined behavior.🔧 Proposed fix
- auto var = convert(inVar); - auto varDeclRef = var.as<VarDeclBase>(); - auto varDecl = varDeclRef.getDecl(); + auto var = convert(inVar); + auto varDecl = as<VarDeclBase>(var.getDecl()); if (!varDecl || !varDecl->type.type) return SLANG_E_INVALID_ARG; + auto varDeclRef = var.as<VarDeclBase>();
3602-3611:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard module/linkage lookups before dereferencing AST builder.
Line 3602 (and similarly Lines 3841, 3909, 3974) assumes
getModule(...)->getLinkage()is always non-null. These are crash paths if a decl is unparented or reflected from partial state.🔧 Proposed hardening pattern
- auto astBuilder = getModule(varDecl)->getLinkage()->getASTBuilder(); + auto module = getModule(varDecl); + if (!module) + return false; + auto linkage = module->getLinkage(); + if (!linkage) + return false; + auto astBuilder = linkage->getASTBuilder(); + SLANG_AST_BUILDER_RAII(astBuilder);Also applies to: 3841-3841, 3909-3910, 3974-3975
3907-3917:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDo not silently zero enum defaults for non-
DeclRefExprinitializers.When the enum initializer is a valid non-
DeclRefExpr(e.g. cast/literal form), Line 3916 falls back to default construction and returns zero bytes with success semantics, which corrupts reflected defaults.🔧 Proposed fix
if (auto enumDeclRef = declRefType->getDeclRef().as<EnumDecl>()) { auto astBuilder = getModule(enumDeclRef.getDecl())->getLinkage()->getASTBuilder(); auto tagType = getTagType(astBuilder, enumDeclRef); if (auto declRefExpr = as<DeclRefExpr>(expr)) { if (appendEnumDefaultValue(declRefExpr->declRef, tagType, outBytes)) return true; } - return appendDefaultConstructedValue(tagType, outBytes); + return appendDefaultValue(tagType, expr, outBytes); }
3659-3716:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCoerce
BoolLiteralExprfor numeric scalar targets.
appendScalarDefaultValuecapturesboolLiteral, but only theBaseType::Boolbranch uses it. Numeric targets (int,float,intptr_t, etc.) rejecttrue/falseafter cast unwrapping, returningSLANG_E_NOT_AVAILABLEfor legal initializers.🔧 Proposed fix
auto intLiteral = as<IntegerLiteralExpr>(expr); auto floatLiteral = as<FloatingPointLiteralExpr>(expr); auto boolLiteral = as<BoolLiteralExpr>(expr); + IntegerLiteralExpr syntheticIntLiteral; + if (!intLiteral && boolLiteral) + { + syntheticIntLiteral.value = boolLiteral->value ? 1 : 0; + intLiteral = &syntheticIntLiteral; + }
3788-3798:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCap serialized default blob size to avoid OOM on large arrays.
Array serialization/default-construction loops are bounded only by declared constant element count. Large valid constants can force massive allocations and stall or OOM reflection callers.
Also applies to: 3818-3824, 3964-3967
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15e9b5b2-bce0-4854-836e-348ce1df7c3e
📒 Files selected for processing (5)
include/slang-deprecated.hinclude/slang.hsource/slang/slang-reflection-api.cpptools/slang-unit-test/unit-test-link-time-type-reflection.cpptools/slang-unit-test/unit-test-special-scalar-reflection.cpp
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
duckdoom5
force-pushed
the
reflect-default-value-blob
branch
from
3e4fd53 to
fd2496f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
source/slang/slang-reflection-api.cpp (1)
3872-3875:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not treat 1-element initializer-list as vector splat.
At Line 3872,
{ 4.0f }andfloat3(4.0f)are handled identically, but initializer-list semantics should zero-fill remaining elements instead of splatting.🔧 Proposed fix
static bool appendVectorDefaultValue(VectorExpressionType* vectorType, Expr* expr, List<uint8_t>& outBytes) { Index elementCount = 0; if (!getDefaultValueKnownCount(vectorType->getElementCount(), elementCount)) return false; auto elementType = vectorType->getElementType(); const auto argCount = getDefaultValueArgCount(expr); if (argCount == 0) return appendRepeatedScalarDefaultValue(elementType, expr, elementCount, outBytes); - if (argCount == 1 && elementCount > 1) + const bool isInitializerList = as<InitializerListExpr>(unwrapDefaultValueExpr(expr)) != nullptr; + if (!isInitializerList && argCount == 1 && elementCount > 1) return appendRepeatedScalarDefaultValue(elementType, getDefaultValueArg(expr, 0), elementCount, outBytes); return appendSequentialDefaultValue(elementType, elementCount, expr, outBytes); }tools/slang-unit-test/unit-test-link-time-type-reflection.cpp (1)
287-290:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explicit bool
false/0blob assertions.Current bool checks validate only
trueand1. Please also pinfalseand0so both bool-encoding branches are covered.🔧 Proposed test additions
public static const bool ScalarBool = true; public static const bool ScalarBoolFromInt = 1; + public static const bool ScalarBoolFalse = false; + public static const bool ScalarBoolFromIntZero = 0; public static const int ScalarIntFromBool = true; public static const float ScalarFloatFromBool = false;auto scalarBoolFromInt = getDefaultBlob("ScalarBoolFromInt"); SLANG_CHECK(scalarBoolFromInt->getBufferSize() == sizeof(uint32_t)); SLANG_CHECK(((const uint32_t*)scalarBoolFromInt->getBufferPointer())[0] == 1); + auto scalarBoolFalse = getDefaultBlob("ScalarBoolFalse"); + SLANG_CHECK(scalarBoolFalse->getBufferSize() == sizeof(uint32_t)); + SLANG_CHECK(((const uint32_t*)scalarBoolFalse->getBufferPointer())[0] == 0); + + auto scalarBoolFromIntZero = getDefaultBlob("ScalarBoolFromIntZero"); + SLANG_CHECK(scalarBoolFromIntZero->getBufferSize() == sizeof(uint32_t)); + SLANG_CHECK(((const uint32_t*)scalarBoolFromIntZero->getBufferPointer())[0] == 0); + auto scalarIntFromBool = getDefaultBlob("ScalarIntFromBool");Also applies to: 409-416
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4a6f98d5-9326-4fff-b9d6-b692bca9c2cd
📒 Files selected for processing (5)
include/slang-deprecated.hinclude/slang.hsource/slang/slang-reflection-api.cpptools/slang-unit-test/unit-test-link-time-type-reflection.cpptools/slang-unit-test/unit-test-special-scalar-reflection.cpp
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
duckdoom5
force-pushed
the
reflect-default-value-blob
branch
from
fd2496f to
0753056
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
duckdoom5
force-pushed
the
reflect-default-value-blob
branch
from
0753056 to
0ebaa77
Compare
There was a problem hiding this comment.
Verdict: 🔴 Has issues — 1 bug, 4 gaps, 1 question
The new spReflectionVariable_GetDefaultValueBlob API has a recurring weakness in how it enforces kMaxDefaultValueBlobBytes: the byte-cap is checked only after each recursive append, the element-count gate compares element count against a byte-count constant, and appendRepeatedScalarDefaultValue skips the in-loop capacity check entirely. Together these allow blob growth several multiples past the documented 16 MB cap. Recursion depth is bounded only for enum-alias indirection — not for struct/array nesting — so deeply nested types can stack-overflow before the byte cap engages. Tests cover scalar, vector, matrix, and flat-aggregate happy paths well; nested aggregates (array-of-struct, struct-with-array, byte-cap-only overflow, deep enum chain) are unexercised.
Changes Overview
New blob reflection API (include/slang-deprecated.h, include/slang.h, source/slang/slang-reflection-api.cpp)
- Adds
spReflectionVariable_GetDefaultValueBlob(C) andVariableReflection::getDefaultValueBlob(C++) returning anISlangBlob*of scalar-layout bytes for a variable's default initializer. Bool emitted as 4-byte uint32, matrices row-by-row, struct base fields before derived, enums in the underlying tag type, no aggregate padding. ReturnsSLANG_OKwith null blob when there is no initializer;SLANG_E_NOT_AVAILABLEwhen an initializer cannot be represented. - Marks
hasDefaultValue/getDefaultValueInt/getDefaultValueFloat(and their C counterparts)SLANG_DEPRECATEDand points doc comments at the new API.
Test additions (tools/slang-unit-test/unit-test-link-time-type-reflection.cpp, tools/slang-unit-test/unit-test-special-scalar-reflection.cpp)
- New
defaultValueBlobReflectiontest fixture covering all scalar base types, vectors with full/splat/partial/empty initializers, matrices, flat arrays, struct partial/default, derived struct, enums (value/alias/cast/default/uint8 tag),EnumCaseDecldirect lookup, no-initializer-null-blob, both null-arg paths, and a 16M+1-element overflow case. specialScalarReflectionextended to verify BFloat16 / FloatE4M3 / FloatE5M2 default-value blobs.
Findings (6 total)
| Severity | Location | Finding |
|---|---|---|
| 🔴 Bug | source/slang/slang-reflection-api.cpp:3823 |
appendRepeatedScalarDefaultValue loop never checks hasDefaultValueBlobCapacity; allows ~128 MB splats past the 16 MB cap |
| 🟡 Gap | source/slang/slang-reflection-api.cpp:3631 |
getDefaultValueKnownCount compares element count to a byte-count constant (kMaxDefaultValueBlobBytes), so the gate is element-size-blind |
| 🟡 Gap | source/slang/slang-reflection-api.cpp:3848 |
Capacity check inside appendSequentialDefaultValue runs after the recursive append; nested aggregates compound the per-level overshoot |
| 🟡 Gap | source/slang/slang-reflection-api.cpp:3793 |
Only enum indirection has a recursion-depth limit; struct/array nesting can stack-overflow before the byte cap engages |
| 🟡 Gap | tools/slang-unit-test/unit-test-link-time-type-reflection.cpp:419 |
No tests for array-of-struct / struct-with-array / byte-cap-only overflow / deep enum chain |
| 🔵 Question | include/slang-deprecated.h:741 |
New non-deprecated C declaration is placed in slang-deprecated.h, contradicting the file's documented purpose |
This PR adds a new API to reflect default initializer values. This follows DXC’s
D3D12_SHADER_VARIABLE_DESC::DefaultValuestyle, retrieving default initializer data as anISlangBlob.This new API is intended to support the entire subset of default-able values. Unlike previous implementations, this also includes non-static const fields (like those in material constant-buffers). (See the unit tests for coverage).
Implementation notes:
nullptrblob, (modelled after DXC's behavior).Fixes #11106