Skip to content

Commit

Permalink
Safeness bug in StorageImage2d::write (#453)
Browse files Browse the repository at this point in the history
* Safeness bug in StorageImage2d::write

It's pretty easy to see why this is unsafe, if multiple threads write to the same coordinate race-conditions happen.

Ultimately this should be addressed through something like #216 and some higher level abstractions on top of our buffer types, but since we don't have those for now, marking this as unsafe seems to be the only thing we can do for now.

* Remove unsafe {} block for clippy
  • Loading branch information
Jasper-Bekkers committed Mar 1, 2021
1 parent 2034141 commit 2f4c67e
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 12 deletions.
4 changes: 3 additions & 1 deletion crates/spirv-builder/src/test/basic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,9 @@ fn image_write() {
#[spirv(fragment)]
pub fn main(input: Input<glam::Vec2>, image: UniformConstant<StorageImage2d>) {
let texels = *input;
image.write(glam::UVec2::new(0, 1), texels);
unsafe {
image.write(glam::UVec2::new(0, 1), texels);
}
}
"#);
}
20 changes: 9 additions & 11 deletions crates/spirv-std/src/textures.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,22 +89,20 @@ impl StorageImage2d {

/// Write a texel to an image without a sampler.
#[spirv_std_macros::gpu_only]
pub fn write<I, V, V2, const N: usize>(&self, coordinate: V, texels: V2)
pub unsafe fn write<I, V, V2, const N: usize>(&self, coordinate: V, texels: V2)
where
I: Integer,
V: Vector<I, 2>,
V2: Vector<f32, N>,
{
unsafe {
asm! {
"%image = OpLoad _ {this}",
"%coordinate = OpLoad _ {coordinate}",
"%texels = OpLoad _ {texels}",
"OpImageWrite %image %coordinate %texels",
this = in(reg) self,
coordinate = in(reg) &coordinate,
texels = in(reg) &texels,
}
asm! {
"%image = OpLoad _ {this}",
"%coordinate = OpLoad _ {coordinate}",
"%texels = OpLoad _ {texels}",
"OpImageWrite %image %coordinate %texels",
this = in(reg) self,
coordinate = in(reg) &coordinate,
texels = in(reg) &texels,
}
}
}
Expand Down

0 comments on commit 2f4c67e

Please sign in to comment.