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

Synchronous validation in getMappedRange #64

Closed
kainino0x opened this issue Jul 16, 2020 · 8 comments · Fixed by #377
Closed

Synchronous validation in getMappedRange #64

kainino0x opened this issue Jul 16, 2020 · 8 comments · Fixed by #377
Labels
buffer mapping MapAsync and friends errors Error reporting has resolution Issue is resolved, just needs to be done

Comments

@kainino0x
Copy link
Collaborator

How should synchronous validation errors be exposed in getMappedRange? It can return null, but that doesn't tell the caller what went wrong. This would probably be fine, except e.g. Chromium needs to know what string to attach to the DOMException it generates. To do that now it needs to duplicate all of the validation that's already in getMappedRange.

@kainino0x
Copy link
Collaborator Author

(Which is especially problematic for the overlapping-range validation.)

@Kangz
Copy link
Collaborator

Kangz commented Jul 16, 2020

IMHO the overlapping range validation should be on the Blink side only, because it stems from limitations of ArrayBuffer. Same thing for the alignment restrictions.

@kainino0x
Copy link
Collaborator Author

Good point. Perhaps no issue here then.

@kainino0x
Copy link
Collaborator Author

from last week's meeting: At least the safe Rust API will keep this validation because it's required for Rust safety guarantees. wgpu-core and the C API don't necessarily have to, still.

@Kangz
Copy link
Collaborator

Kangz commented Jun 29, 2023

Do we have an explanation of why this is needed for the Rust safety guarantees? It seems it would only be to get mutable ranges?

@kainino0x
Copy link
Collaborator Author

We didn't specifically talk about const ranges, maybe for const ranges overlaps are fine? @cwfitzgerald

@cwfitzgerald
Copy link
Collaborator

Correct, const overlaps are fine, mutable overlaps are not.

@kainino0x
Copy link
Collaborator Author

kainino0x commented Sep 7, 2023

In June we had concluded:

make this Result-like, error is either an enum or a string (who owns the string?)

However we discussed this more today in the broader context of reporting synchronous errors and came to a simpler conclusion:

  • CF: Can’t do an enum because in WASM we can’t convert the JS Error to an enum.
  • Could return a struct that you have to FreeMembers. Very annoying because you have to do this for every single GetMappedRange call.
  • Out param for the error, and if you pass it then you have to FreeMembers?
  • CF: Worried about inconsistencies across the API
  • KN: Just return a two-state enum so we don’t have to convert JS error to enum? (Actually that’s the same as returning nullptr)
    • CF: Implementation-defined logging
  • KN: There’s two error cases: overlapping (mutable) ranges, and asking for a mutable pointer to immutable mapping.
  • conclusion: Return nullptr on error, with implementation-defined logging

@kainino0x kainino0x added the has resolution Issue is resolved, just needs to be done label Sep 7, 2023
kainino0x added a commit to kainino0x/webgpu-headers that referenced this issue Oct 19, 2024
kainino0x added a commit to kainino0x/webgpu-headers that referenced this issue Oct 19, 2024
kainino0x added a commit to kainino0x/webgpu-headers that referenced this issue Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer mapping MapAsync and friends errors Error reporting has resolution Issue is resolved, just needs to be done
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants