Skip to content
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

Jump to bottom

WGSL: Nullpointer dereference in WGSLSourceEmitter::emitStructFieldAttributes #5264

Closed
aleino-nv opened this issue Oct 11, 2024 · 2 comments · Fixed by #5525
Closed

WGSL: Nullpointer dereference in WGSLSourceEmitter::emitStructFieldAttributes #5264

aleino-nv opened this issue Oct 11, 2024 · 2 comments · Fixed by #5525
Assignees
@aleino-nv
Labels
goal:forward looking Feature needed at a later date, not connected to a specific use case. kind:bug something doesn't work like it should siggraphasia-2024
Milestone
Q4 2024 (Fall)

Comments

@aleino-nv
Copy link
Collaborator

aleino-nv commented Oct 11, 2024 •
edited
Loading

Affected tests:

  • tests/autodiff/existential-1.slang
  • tests/autodiff/existential-2.slang
  • tests/autodiff/material/diff-bwd-falcor-material-system.slang
  • tests/autodiff/material2/diff-bwd-falcor-material-system.slang

This returnsl nullptr:

    IROffsetDecoration *const fieldOffsetDecoration =
        field->findDecoration<IROffsetDecoration>();

slang-test -bindir %BINDIR% -category full -api wgpu tests/autodiff/existential-1.slang
This was referenced Oct 14, 2024
Closed
Closed
@aleino-nv aleino-nv self-assigned this Oct 14, 2024
@bmillsNV bmillsNV added this to the Q4 2024 (Fall) milestone Oct 17, 2024
@bmillsNV bmillsNV added kind:bug something doesn't work like it should goal:forward looking Feature needed at a later date, not connected to a specific use case. labels Oct 17, 2024
@aleino-nv
Copy link
Collaborator Author

Here follows my summary of what's been discovered so far.

It seems that during struct legalization we create a new struct that inherits the 'size and alignment decoration', which is supposed to imply that the struct fields have got offsets calculated and added.
However, the new struct is created in such a way that the offsets did not get added, and so we crash during WGSL emission.

The fix will be to make sure we copy over the offsets for the fields that survive legalization.
(The legalization just removes fields that don't have any bearing on memory layout, so the offsets should still be valid.)

aleino-nv added a commit to aleino-nv/slang that referenced this issue Nov 8, 2024
@aleino-nv
Transfer offset decorations when legalizing laid-out structs
c8b88c3 
Struct legalization removing fields not representable in memory should transfer offset
decorations in case the struct has already had offsets calculated.

Closes shader-slang#5264.
@aleino-nv aleino-nv mentioned this issue Nov 8, 2024
Merged
@aleino-nv
Copy link
Collaborator Author

Currently testing the propsed fix in #5525

aleino-nv added a commit to aleino-nv/slang that referenced this issue Nov 11, 2024
@aleino-nv
Transfer offset decorations when legalizing laid-out structs
24aec7f 
Struct legalization removing fields not representable in memory should transfer all
decorations in case the struct has already had offsets calculated.

Closes shader-slang#5264.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aleino-nv aleino-nv

Labels
goal:forward looking Feature needed at a later date, not connected to a specific use case. kind:bug something doesn't work like it should siggraphasia-2024
Projects
None yet
Milestone
Q4 2024 (Fall)
Development

Successfully merging a pull request may close this issue.

Transfer offset decorations when legalizing laid-out structs aleino-nv/slang
2 participants
@aleino-nv @bmillsNV