Skip to content

Commit

Permalink
Make buffer_map and buffer_unmap check for device validity, add tests. (
Browse files Browse the repository at this point in the history
#4212)

* Make buffer_unmap check for device validity, add tests.

This patch makes buffer_unmap check for a valid device, and corrects
buffer_map to return the appropriate error for an invalid device.
Tests are added for both operations.

It also adds device validity checks to device_maintain_ids and to the
functions that get and set buffer sub data.

* Update changelog and test comment.

* Run rustfmt.

* Update test device_lose_then_more to specify more buffer usages to keep Vulkan happy.
  • Loading branch information
bradwerth authored Oct 7, 2023
1 parent c85cbea commit e6097ce
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 7 deletions.
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ By @teoxoy in [#4185](https://github.com/gfx-rs/wgpu/pull/4185)
- `wgpu::CreateSurfaceError` and `wgpu::RequestDeviceError` now give details of the failure, but no longer implement `PartialEq` and cannot be constructed. By @kpreid in [#4066](https://github.com/gfx-rs/wgpu/pull/4066) and [#4145](https://github.com/gfx-rs/wgpu/pull/4145)
- Make `WGPU_POWER_PREF=none` a valid value. By @fornwall in [4076](https://github.com/gfx-rs/wgpu/pull/4076)
- Support dual source blending in OpenGL ES, Metal, Vulkan & DX12. By @freqmod in [4022](https://github.com/gfx-rs/wgpu/pull/4022)
- Add stub support for device destroy and device validity. By @bradwerth in [4163](https://github.com/gfx-rs/wgpu/pull/4163)
- Add stub support for device destroy and device validity. By @bradwerth in [4163](https://github.com/gfx-rs/wgpu/pull/4163) and in [4212](https://github.com/gfx-rs/wgpu/pull/4212)
- Add trace-level logging for most entry points in wgpu-core By @nical in [4183](https://github.com/gfx-rs/wgpu/pull/4183)
- Add `Rgb10a2Uint` format. By @teoxoy in [4199](https://github.com/gfx-rs/wgpu/pull/4199)
- Validate that resources are used on the right device. By @nical in [4207](https://github.com/gfx-rs/wgpu/pull/4207)
Expand Down
32 changes: 27 additions & 5 deletions tests/tests/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,18 @@ fn device_destroy_then_more() {
usage: wgpu::BufferUsages::MAP_READ | wgpu::BufferUsages::COPY_DST,
mapped_at_creation: false,
});
let buffer_for_map = ctx.device.create_buffer(&wgpu::BufferDescriptor {
label: None,
size: 256,
usage: wgpu::BufferUsages::MAP_WRITE | wgpu::BufferUsages::COPY_SRC,
mapped_at_creation: false,
});
let buffer_for_unmap = ctx.device.create_buffer(&wgpu::BufferDescriptor {
label: None,
size: 256,
usage: wgpu::BufferUsages::MAP_WRITE | wgpu::BufferUsages::COPY_SRC,
mapped_at_creation: true,
});

// Create a bind group layout.
let bind_group_layout =
Expand Down Expand Up @@ -215,16 +227,14 @@ fn device_destroy_then_more() {
ctx.device.destroy();

// TODO: verify the following operations will return an invalid device error:
// * Run a compute pass
// * Run a render pass
// * Run a compute or render pass
// * Finish a render bundle encoder
// * Create a texture from HAL
// * Create a buffer from HAL
// * Create a sampler
// * Validate a surface configuration
// * Start capture
// * Stop capture
// * Buffer map
// * Start or stop capture
// * Get or set buffer sub data

// TODO: figure out how to structure a test around these operations which panic when
// the device is invalid:
Expand Down Expand Up @@ -439,6 +449,18 @@ fn device_destroy_then_more() {
entry_point: "",
});
});

// Buffer map should fail.
fail(&ctx.device, || {
buffer_for_map
.slice(..)
.map_async(wgpu::MapMode::Write, |_| ());
});

// Buffer unmap should fail.
fail(&ctx.device, || {
buffer_for_unmap.unmap();
});
},
)
}
15 changes: 14 additions & 1 deletion wgpu-core/src/device/global.rs
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,9 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
let device = device_guard
.get(device_id)
.map_err(|_| DeviceError::Invalid)?;
if !device.valid {
return Err(DeviceError::Invalid.into());
}
let buffer = buffer_guard
.get_mut(buffer_id)
.map_err(|_| BufferAccessError::Invalid)?;
Expand Down Expand Up @@ -436,6 +439,9 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
let device = device_guard
.get(device_id)
.map_err(|_| DeviceError::Invalid)?;
if !device.valid {
return Err(DeviceError::Invalid.into());
}
let buffer = buffer_guard
.get_mut(buffer_id)
.map_err(|_| BufferAccessError::Invalid)?;
Expand Down Expand Up @@ -2400,6 +2406,9 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
let mut token = Token::root();
let (device_guard, mut token) = hub.devices.read(&mut token);
let device = device_guard.get(device_id).map_err(|_| InvalidDevice)?;
if !device.valid {
return Err(InvalidDevice);
}
device.lock_life(&mut token).triage_suspected(
hub,
&device.trackers,
Expand Down Expand Up @@ -2711,7 +2720,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {

let device = &device_guard[buffer.device_id.value];
if !device.valid {
return Err((op, BufferAccessError::Invalid));
return Err((op, DeviceError::Invalid.into()));
}

if let Err(e) = check_buffer_usage(buffer.usage, pub_usage) {
Expand Down Expand Up @@ -2766,6 +2775,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
};

let device = &device_guard[device_id];
// Validity of device was confirmed in the code block that set device_id.
device
.lock_life(&mut token)
.map(id::Valid(buffer_id), ref_count);
Expand Down Expand Up @@ -2955,6 +2965,9 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
.get_mut(buffer_id)
.map_err(|_| BufferAccessError::Invalid)?;
let device = &mut device_guard[buffer.device_id.value];
if !device.valid {
return Err(DeviceError::Invalid.into());
}

closure = self.buffer_unmap_inner(buffer_id, buffer, device)
}
Expand Down

0 comments on commit e6097ce

Please sign in to comment.