Fix incorrect offset in get_mapped_range #3233
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3233 +/- ##
=======================================
Coverage 65.40% 65.40%
=======================================
Files 82 82
Lines 39451 39454 +3
=======================================
+ Hits 25802 25805 +3
Misses 13649 13649
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
@cwfitzgerald Perhaps this one fixes a sufficiently bad issue to warrant being rebased on top of the current release and publishing it as |
wgpu/tests/buffer.rs
Outdated
| while !status.load(Ordering::SeqCst) { | ||
| ctx.device.poll(wgpu::MaintainBase::Poll); | ||
| } |
There was a problem hiding this comment.
What's the point of this vs just waiting?
There was a problem hiding this comment.
Are you referring to using MaintainBase::Wait instead of Poll ? I don't know, both appear to work, changing it to Wait.
wgpu/tests/buffer.rs
Outdated
| write_buf | ||
| .slice(32..) | ||
| .map_async(wgpu::MapMode::Write, move |result| { | ||
| assert!(result.is_ok()); |
There was a problem hiding this comment.
lets do result.unwrap() as it gives a better message
wgpu/tests/buffer.rs
Outdated
| for _ in 0..10 { | ||
| ctx.device.poll(wgpu::MaintainBase::Poll); | ||
| } |
There was a problem hiding this comment.
That's in the old unfinished stuff, I'm not sure what I meant by "poll ten times". My guess is that I probably wanted the test to get a chance to run unfinished async code in case there is anything queued up while not have anything in particular to wait for, if that makes any sense.
There was a problem hiding this comment.
I would just do Wait everywhere, otherwise there's still technically a potential race.
wgpu/tests/buffer.rs
Outdated
| .map_async(wgpu::MapMode::Read, move |result| { | ||
| assert!(result.is_ok()); | ||
| done.store(true, Ordering::SeqCst); | ||
| }); | ||
|
|
||
| while !status.load(Ordering::SeqCst) { |
There was a problem hiding this comment.
By waiting, you guarentee that all pending map_asyncs are finished, so this atomic code can all go away.
wgpu/tests/buffer.rs
Outdated
| for _ in 0..10 { | ||
| ctx.device.poll(wgpu::MaintainBase::Poll); | ||
| } |
There was a problem hiding this comment.
I would just do Wait everywhere, otherwise there's still technically a potential race.
wgpu/tests/buffer.rs
Outdated
| @@ -91,18 +97,100 @@ fn test_empty_buffer_range(ctx: &TestingContext, buffer_size: u64, label: &str) | |||
|
|
|||
| b1.unmap(); | |||
|
|
|||
| for _ in 0..10 { | |||
| for _ in 0..10 { | |||
| ctx.device.poll(wgpu::MaintainBase::Poll); | |||
There was a problem hiding this comment.
| ctx.device.poll(wgpu::MaintainBase::Poll); | |
| ctx.device.poll(wgpu::Maintain::Wair); |
|
The linux failure looks like an unrelated driver sneeze. |
Checklist
cargo clippy.Connections
Found the issue while fuzzing Gecko's WebGPU integration.
Description
This one is kind of serious.
get_mapped_range(spec) takes a range that is relative to the beginning of the buffer. The implementation in webgpu seems to want to agree judging by how the validation checks that the range is contained in the mapped one. However internally theptrstored inBufferMapState::Activeis actually relative to the beginning of the mapped range (rather than beginning of the buffer). This means that we have to subtract the mapped offset to the one provided inget_mapped_range, otherwise seriously scary things happen.Testing
Added a simple test.
Note that in the process of adding the test I found out that tests/buffer.rs was not run (oops!). I hadn't finished it when it landed a little while ago, it was not quite working yet because of how wgpu does not accept empty slices, I had put it on hold for a bit and when it landed anyway without breaking the build I didn't think too much of it but the version that landed was not part of the build. So this commit puts the file back in the build with a #[ignore] slapped on it, and of course it got reformatted in the process.