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.

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

Add struct chaining docs #469

Merged
merged 3 commits into from
Dec 14, 2024

Conversation

kainino0x
Copy link
Collaborator

Also add compile tests for the things I wrote in the documentation sample code.

Issue: none, #264, should have been listed in #25, related to #463

Also test the thing in the example text.

Issue: none, 264, should have been in 25, related to 463
@kainino0x kainino0x added the extensibility Adding features without breaking API changes label Dec 12, 2024
@kainino0x kainino0x requested a review from lokokung December 12, 2024 19:49
They occur if and only if:

- The `sType` of a struct in the chain is not valid for the type of the chain root.
- Multiple instances of the same `sType` value are seen in the same struct chain. (Note this also detects and disallows cycles.)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit. To clarify this a bit more (at least w.r.t the fuzzer case), it's not just that the same sType can't be in the same chain, but that the same sType should never appear in a chain or member chain on a root struct. Not sure how to word it clearly here...

Copy link
Collaborator Author

@kainino0x kainino0x Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(discussed offline)

The first requirement (sType is valid for the type of the chain root) should be the one that validates that. The only gap is if some implementation adds an extension that can actually recurse, which we probably should not allow them to do, so I'll note that.

We should also guarantee that if the implementation doesn't allow the sType in that context, it will never try to downcast the struct. It's the same as if the sType were totally bogus.

In Dawn's wire, Loko came up with a way to guarantee that invalid chains can be serialized and deserialized without hitting serialization errors: if the client doesn't allow an sType in context, convert it to something like WGPUSType_DawnInvalidChainedStruct which indicates struct WGPUDawnInvalidChainedStruct { WGPUChainedStruct chain; WGPUSType sTypeForErrorMessage; }. As long as the client and server are in sync the server can always handle this without a deserialization error, and forward it into dawn-native which will produce a validation error. If the client is out of sync or is malicious (e.g. a fuzzer) then if the server sees an sType that's not valid in context it immediately gives up and fails deserialization.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only gap is if some implementation adds an extension that can actually recurse, which we probably should not allow them to do, so I'll note that.

Actually I started trying to write this down and realized there could be legitimate use cases that pass variable-sized data as a linked list (rather than an array or array of pointers), regardless of whether they're using struct chaining. So I'm going to make this a "should", not a "must".

doc/articles/StructChaining.md Show resolved Hide resolved
@kainino0x kainino0x requested a review from lokokung December 12, 2024 23:02
@kainino0x kainino0x enabled auto-merge (squash) December 14, 2024 05:42
@kainino0x kainino0x merged commit 77a0f3d into webgpu-native:main Dec 14, 2024
5 checks passed
@kainino0x kainino0x deleted the struct-chaining-docs branch December 19, 2024 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extensibility Adding features without breaking API changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants