Skip to content

sgl: raise a catchable error when a shader cursor has no resolved type layout - #1139

Draft
nv-slang-bot[bot] wants to merge 1 commit into
mainfrom
dev/slangpy-fixer/slangpy-1138
Draft

sgl: raise a catchable error when a shader cursor has no resolved type layout#1139
nv-slang-bot[bot] wants to merge 1 commit into
mainfrom
dev/slangpy-fixer/slangpy-1138

Conversation

@nv-slang-bot

@nv-slang-bot nv-slang-bot Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Motivation

#1138 reported a SIGSEGV marshalling a Python list to a float[N] parameter on DeviceType.cpu. It has since been closed after the reported failure was no longer reproducible on a current toolchain: the crash was observed on slang 2026.4.1 (slang-rhi ee078c7) inside a full pytest slangpy/tests/slangpy_tests/test_simple_function_call.py --device-types cpu run, and is no longer reproducible on slang 2026.12.2 (that same CPU test run passes; isolated mod.first([3.0,4.0,5.0]) returns 3.0) and clean on 2026.16.1 (float/double/int/uint/int64, list & numpy, float[3]/float[5]). It is not a live defect and not a compiler escalation.

This PR is independent defensive hardening for the crash class the report surfaced, in the spirit of #1137 (turn an undefined crash into defined behavior).

Why a null layout can reach the dereference

ShaderCursor's typed write and marshalling paths dereference the cursor's TypeLayoutReflection without checking it:

  • ShaderCursor::is_valid() checks only the offset, not m_type_layout.
  • ShaderCursor::find_element (array case) builds the child element cursor by assigning m_type_layout = getElementTypeLayout() directly, not through the constructor — so a null element layout is not caught by the constructor's SGL_ASSERT(m_type_layout) (asserts are enabled by default via SGL_ENABLE_ASSERTS, but this path bypasses it).

A null layout carried this way is then dereferenced during marshalling (type_layout->getKind()) or a typed write (getElementStride/getSize), which is a null-pointer crash rather than a defined error.

Change (ShaderCursor write / marshalling paths)

  • src/slangpy_ext/device/cursor_utils.h — the nanobind write visitor SGL_CHECKs the layout before switching on its kind. This is the path Python list/array marshalling takes for a ShaderCursor; recursive per-element writes re-enter it.
  • src/sgl/device/cursor_access_wrappers.h — the write-side cursor wrapper (CursorWriteWrappers::_get_slang_type_layout(), used by ShaderCursor scalar/vector/matrix/array writes) raises a catchable error before dereferencing.
  • src/sgl/device/shader_cursor.h — initialize the previously-uninitialized m_type_layout member so a default-constructed cursor has a defined null layout (making the guard reliable).

Scope note: this guards the cursor's own resolved layout on the ShaderCursor write/marshalling paths — the reported find_element case, where a null element layout becomes the child cursor's own layout and is caught on the child's write. It does not cover:

  • the element layout deref in bulk array/vector writes — cursor_access_wrappers.h:63 (_set_array_or_vector) derefs getElementTypeLayout() unconditionally and unguarded, so a cursor with a valid array/vector layout but a null element layout would still crash there (distinct from the reported find_element path). The analogous matrix element derefs at :172/:176 are inside #ifdef SGL_ENABLE_CURSOR_TYPE_CHECKS (debug builds only);
  • ShaderCursor::len() / __len__ introspection (cursor_utils.h:1019), still an unguarded null-layout deref outside the write path;
  • BufferElementCursor (its slang_type_layout() dereferences its layout ref before any guard could run);
  • ShaderCursor resource/pointer/object setters and _set_array_unsafe.

A source-side guard in ShaderCursor::find_element (refusing to build a child when getElementTypeLayout() is null) would close the class more completely, including the introspection paths, and could name the offending element/index. It is deliberately left as a possible follow-up rather than expanding this defensive-only change.

Tests

  • New C++ regression test tests/sgl/device/test_cursors.cpp::shader_cursor_typed_write_without_type_layout_throws: a default-constructed ShaderCursor (null layout) must throw on a typed scalar set() and _set_array() instead of crashing. sgl_tests "cursors" suite passes 14/14. The test uses value-initialization (ShaderCursor{}), so it exercises the SGL write-wrapper guard (guard Test issue (slangpy): sync issue to slang #1) — not the nanobind write_internal visitor (guard Modifications + docs for release #2), which the next paragraph covers. It does not by itself prove the m_type_layout{nullptr} member initializer: value-init zeroes the member regardless. The initializer's value is to make a genuinely default-initialized ShaderCursor cursor; (declared without braces and read before any layout assignment) deterministically null rather than an indeterminate pointer, closing latent UB; that path cannot be asserted portably (reading an uninitialized pointer is UB). find_element's child cursors do not rely on it — they assign m_type_layout = getElementTypeLayout() immediately (shader_cursor.cpp:328/344), so a null there comes from a null element layout, not from the default init.
  • Marshalling no-regression: verified on a build that includes the Treat zero compute-dispatch-group limit as unbounded (fixes #1136) #1137 dispatch-limits fix (required to reach the marshalling code on CPU) — mod.first([3.0,4.0,5.0]) (deferred + eager) and mod.first(np.array([7,8,9], float32)) return the correct results with the guards present. (On main without Treat zero compute-dispatch-group limit as unbounded (fixes #1136) #1137 the CPU dispatch stops earlier at the Every compute dispatch on the CPU device fails: "Device reports zero compute dispatch groups in X" (regression in 0.43.0) #1136 "zero compute dispatch groups" throw.)

The guard-firing path through the nanobind write visitor is not unit-tested: slangpy_ext has no C++ test target, and the reported repro and all tested variants always reflected a valid layout, so a null-layout cursor could not be produced through the Python/nb API to exercise the guard end-to-end on the tested toolchains. The C++ test covers the guard at the SGL wrapper level via the one synthesizable null-layout scenario (a default-constructed cursor).

🤖 Generated by an automated SlangPy coworker — may be inaccurate. A human maintainer should verify.

…lved type layout

ShaderCursor's typed write and marshalling paths dereference the cursor's
TypeLayoutReflection without checking it. ShaderCursor::is_valid() only checks the
offset, and find_element assigns a child cursor's layout directly from
getElementTypeLayout() (which can be null) rather than through the asserting
constructor -- so a null or unresolved layout can reach the dereference even with
asserts enabled (SGL_ENABLE_ASSERTS is on by default; this path bypasses it),
crashing the host process instead of raising an error. This is the class of failure
reported for CPU array-parameter marshalling in #1138.

Guard the layout before it is dereferenced:
- src/slangpy_ext/device/cursor_utils.h: the nanobind write visitor checks the
  layout before switching on its kind -- the path Python list/array marshalling
  takes for a ShaderCursor (recursive per-element writes re-enter it).
- src/sgl/device/cursor_access_wrappers.h: the write-side cursor wrapper guards
  _get_slang_type_layout() so typed ShaderCursor writes raise a catchable error.
- src/sgl/device/shader_cursor.h: initialize the previously-uninitialized
  m_type_layout member so a default-constructed cursor has a defined null layout.

Adds a C++ regression test. This hardens a crash class rather than fixing a
reproduced fault: #1138 was observed only on slang 2026.4.1 and
is no longer reproducible on slang 2026.12.2 or the pinned toolchain.
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Automated notice (PR board sync) — do not reply to this comment.

Auto-assigned @kaizhangNV as shepherd for this Bot PR.

FYI for maintainers: committer signal on the changed files is highest for ccummingsNV among collaborators other than the assignee. They were not auto-requested; a human may optionally add them as a reviewer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants