-
Notifications
You must be signed in to change notification settings - Fork 180
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got some questions about the chage
where | ||
T: 'static + Copy, | ||
{ | ||
) -> CreateBufferMapped<'a, T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to at least have T: 'a
(i.e. T type lives at least as long as 'a) unless it's inferred automatically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized... the current function is actually unsafe:
let test_buf = device.create_buffer_mapped::<&u8>(8, wgpu::BufferUsage::VERTEX);
println!("{:?}", test_buf.data); // Segmentation fault
I'll need to think about how to fix this...
@@ -888,8 +885,8 @@ where | |||
impl Buffer { | |||
pub fn map_read_async<T, F>(&self, start: BufferAddress, size: BufferAddress, callback: F) | |||
where | |||
T: 'static + FromBytes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is static
removed from here? We don't know when the call back is going to be called, and for this time frame T
has to be valid. static
is a conservative bound to enforce that
Ok, I have looked into this further and I realized that this is a much trickier issue than I thought at first. I was looking at I ended up going wayyy down the rabbit hole and I found out that even It all stems from the problem that it's hard to properly manage allocating a buffer for a user-provided type. You have to consider:
I can see that this library is still in the early stages so I'm not too concerned about security yet, but this is definitely something that has to be taken into consideration in the design (see Please let me know if I am wrong about anything, and what you think about this issue. |
Thank you for the detailed report on your mapping investigation! I'll try to answer some of the points, please ping me if something is missing. First of all, it's not completely clear yet if the upstream API will even have mapping, and in what form. See gpuweb/gpuweb#491 for an alternative. AlignmentYou are correct that we aren't doing the right thing if the structure is big. We need to either never have references into mapped area, or take the ReferencesPreviously, we tried to address this by introducing our own trait, see gfx-rs/gfx#999 . Now we use |
I've been looking into this some more. These are the basic problems I've found:
For the first problem, right now I'm leaning towards making
Note that this is true for nearly all types, notably including
About the "staging belt" proposal, I really like it (nit: maybe separate functions for providing data vs. requesting a buffer; also, will synchronously requesting a buffer really be fast enough?). As I understand it, from the perspective of the website, you are still mapping a buffer right? (even if that's not what actually happens behind the scenes). I am still looking into ways to fix the 2nd and 3rd problems (alignment and generic bounds respectively). For the last problem, there's a bit of an issue: what actually is the difference between safe and unsafe code when it comes to the GPU? The Nomicon says, in bold, "No matter what, Safe Rust can't cause Undefined Behavior". What actually is "undefined behavior"? Does that include invalid memory access on the GPU, or just the CPU? I think it really depends on if you treat the GPU as a remote black box or as something more directly integrated with your code's resources/execution. IMHO any code on the GPU should also have the same safety guarantees, e.g. never access unallocated/uninitialized/freed memory, among other things. In this case, a number of functions need to either have even safer wrappers created or be marked The majority of functions currently in
I really think that a EDIT: Also, I can't figure out for the life of me: how are you supposed to use |
I don't think this will be necessary, because we'll have validation in each function to avoid all unsafe behavior (including shader validation to prevent out-of-bounds access in a shader for example). So in general the API should (eventually) be fully safe and not require using the We already have a few |
In addition to @grovesNL comments: We are absolutely positive about making this a safe API in Rust sense: we are going to be validating all the state and either panicing or processing the errors carefully, without ever triggering an undefined behavior. We don't want the entry points to be unsafe, therefore, unless there is no other way. Mapping is a tricky subject, but we shouldn't need |
@grovesNL @kvark I've put some thought into this, and I agree that the
The point is: I feel very strongly that you cannot catch all invalid usages if you take any generic type at all. Even if you find the proper size and alignment of the type as a whole, there are just too many things that can go wrong. I know that this is kind of ironic to say considering the original goal of the PR 😅 but I have a much better understanding of how I have looked into the In the meantime, I think it's important that at least these functions be made unsafe. I think my solution of using To sum it up, possible solutions I've thought of for mapping buffers are:
I think solution #2 is the best short-term, and solution #1 is best long-term. I hope I answered your concerns, and please let me know if I missed anything or got something wrong. Also, for the time being, treat this PR more like an issue. |
Thanks for the detailed investigation!
If this turns out to be the case, we should still be able to safely expose buffer mappings as
Could we use the traits from zerocopy to have better guarantees about byte-alignment as we're doing in a couple places? In general I think it's fine to require additional trait bounds here.
We're converting
I think solution 2 is closer to what we would prefer, but with a stronger trait bound that doesn't require unsafe in order to use it. For example, using zerocopy's |
@Coder-256 The depth at which you are investigating this issue is incredible! I do have a feeling that maybe you are digging in a slightly wrong direction. You are trying to show that supporting generic
The API safety would stop right after we expose mapping as |
@grovesNL @kvark Thank you both for helping with this!
Yes, that's true... although I think that leaving it up to users to convert a slice of bytes back into their struct wouldn't be the best UX. More on that below. I agree that it is safe though (with
Still looking into this. However, while looking into the way // Total alignment for a struct is the maximum alignment.
// Total size for a packed struct is the sum of sizes.
// `Foo` has an alignment of 4 and a size of 12.
// In this case, `packed(4)` is already implied, I'm just being explicit.
#[repr(C, packed(4))]
struct Foo {
bar: u8, // align: 1, size: 1
_pad0: [u8; 3] // align: 1, size: 3
baz: f32, // align: 4, size: 4
qux: u16, // align: 2, size: 2
_pad1: [u8; 2] // align: 1, size: 2
// _pad1 makes the struct's size a multiple of its alignment
// This is necessary in order to emulate a non-packed struct
} Side note: I think the corresponding struct in SPIR-V would not need to have these padding bytes if the graphics API ensures that it is safe for the CPU to read from uninitialized padding bytes, but I haven't looked into the portability of this guarantee.
I respectfully disagree. I am not just trying to show that mapping With Rust's ridiculously complex padding/alignment issues I mentioned above, it is super easy for users to mess this all up. IMHO I feel that it is very important that if there is any way that Again, this is just my opinion, but I do feel strongly that leaving users on their own with a |
Also, completely unrelated, are you sure that this is valid?: Lines 548 to 563 in 95787f1
Couldn't Edit: What I mean to say is: shouldn't |
This is the way WebGPU works in general. The type of these mapped buffers is If we can provide some safe helpers to convert between
Currently no because the callback fires immediately inline. The callback isn't scheduled/queued anywhere yet (see gfx-rs/wgpu#375 (comment) for an explanation). We're planning to change that soon, at which point this signature and implementation would change too. |
126: Fix generic bounds on buffer mapping r=grovesNL a=kvark Smaller (and incomplete!) alternative to #119 while it's still discussed. One missing piece here is alignment. cc @Coder-256 Also, importing `wgpu-core` as just "core" wasn't the best idea 😅 : it collides with the actual `core`. Co-authored-by: Dzmitry Malyshau <[email protected]>
I am aware that it is an
That being said, I think it would be very helpful to be able to request an alignment so that you can use I guess this just comes down to the intended level of safety guarantee of Full disclosure: I am not an expert on WebGPU, I am pretty new to the proposal. Anyway, my point has been that even though WebGPU is designed for JS, it is also designed to be very low-level like Vulkan. It seems like a natural consequence that a Rust wrapper should be safe. For example, it should offer a way to transmute to a custom struct while upholding type safety if doing so is possible, and I believe it is. Exposing a raw buffer of Bottom line: I'm ok with safely exposing a |
(I rewrote the answer a couple of times before posting...)
correct
The open question (that needs to be answered for this) is whether or not we can even check the layout of uniform data that the shader uses. If SPIR-V exposes this, and would be able to check if the layout matches CPU side, in theory. It feels like we are a looong way to there, possibly past the Javelin checkpoint. It just feels like layout matching falls into a large category of "cool safety things we could do in the future", somewhere behind the generically bounds buffer and texture usages. It's not required for us to expose a safe API, it's nice, it's probably going to be complicated. So the best way would be for it to go into some sort of a wrapper crate, e.g. |
I've already looked into this a bit. SPIR-V definitely exposes this info (although possibly without field names, but that might be part of the debug info such as |
Alright, it looks like the problem will need to be attacked from another angle. Closing this, please feel free to reopen! |
Also removes unnecessary
'static
lifetimes. LMK if you would like me to undo 62bbe5f (cargo fmt
).