From 7fea44b4da3e2f968ec9b547039e40b612e85953 Mon Sep 17 00:00:00 2001 From: Eric Culp Date: Fri, 28 Sep 2018 21:43:09 -0700 Subject: [PATCH] Reject spirv arrays that have incorrect stride in rust (#1050) SPIR-V allows the array stride and size of a type to differ, but rust defines them to be the same. Thus certain types when represented in rust will have the wrong layout. E.g. an array of vec3 can have an array stride of 16 in SPIR-V, but an array of [f32;3] in rust would have a stride of 12. Thus using one for the other would cause corruption. This suggests a workaround by using a wrapping struct or upgrading the size of the type to one where the size is the array stride. I considered generating the wrapping struct for the user, but that seems very confusing for the user. We could generate wrapping structs for all vec and mat types in arrays, but that would be a large API change. See #298. --- CHANGELOG.md | 2 +- vulkano-shaders/src/lib.rs | 63 ++++++++++++++++++++++++++++++++++ vulkano-shaders/src/structs.rs | 20 ++++++++++- 3 files changed, 83 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b9a05de..24704276 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,8 +5,8 @@ - Fix instance_count when using draw_index with instance buffers - Added a `reinterpret` function to `BufferSlice` - Made `AttributeInfo` derive `Copy`, `Clone` and `Debug` - - Use [google/shaderc](https://github.com/google/shaderc-rs) for shader compilation +- Reject generation of rust types for SPIR-V arrays that would have incorrect array stride. # Version 0.10.0 (2018-08-10) diff --git a/vulkano-shaders/src/lib.rs b/vulkano-shaders/src/lib.rs index e3123715..1db188eb 100644 --- a/vulkano-shaders/src/lib.rs +++ b/vulkano-shaders/src/lib.rs @@ -536,3 +536,66 @@ fn capability_name(cap: &enums::Capability) -> Option<&'static str> { enums::Capability::CapabilityMultiViewport => Some("multi_viewport"), } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_bad_alignment() { + // vec3/mat3/mat3x* are problematic in arrays since their rust + // representations don't have the same array stride as the SPIR-V + // ones. E.g. in a vec3[2], the second element starts on the 16th + // byte, but in a rust [[f32;3];2], the second element starts on the + // 12th byte. Since we can't generate code for these types, we should + // create an error instead of generating incorrect code. + let comp = compile(" + #version 450 + struct MyStruct { + vec3 vs[2]; + }; + layout(binding=0) uniform UBO { + MyStruct s; + }; + void main() {} + ", ShaderKind::Vertex).unwrap(); + let doc = parse::parse_spirv(comp.as_binary()).unwrap(); + let res = std::panic::catch_unwind(|| structs::write_structs(&doc)); + assert!(res.is_err()); + } + #[test] + fn test_trivial_alignment() { + let comp = compile(" + #version 450 + struct MyStruct { + vec4 vs[2]; + }; + layout(binding=0) uniform UBO { + MyStruct s; + }; + void main() {} + ", ShaderKind::Vertex).unwrap(); + let doc = parse::parse_spirv(comp.as_binary()).unwrap(); + structs::write_structs(&doc); + } + #[test] + fn test_wrap_alignment() { + // This is a workaround suggested in the case of test_bad_alignment, + // so we should make sure it works. + let comp = compile(" + #version 450 + struct Vec3Wrap { + vec3 v; + }; + struct MyStruct { + Vec3Wrap vs[2]; + }; + layout(binding=0) uniform UBO { + MyStruct s; + }; + void main() {} + ", ShaderKind::Vertex).unwrap(); + let doc = parse::parse_spirv(comp.as_binary()).unwrap(); + structs::write_structs(&doc); + } +} diff --git a/vulkano-shaders/src/structs.rs b/vulkano-shaders/src/structs.rs index 6746affd..9354ac12 100644 --- a/vulkano-shaders/src/structs.rs +++ b/vulkano-shaders/src/structs.rs @@ -386,6 +386,7 @@ pub fn type_from_id(doc: &parse::Spirv, searched: u32) -> (String, Option } if result_id == searched => { debug_assert_eq!(mem::align_of::<[u32; 3]>(), mem::align_of::()); let (t, t_size, t_align) = type_from_id(doc, type_id); + let t_size = t_size.expect("array components must be sized"); let len = doc.instructions .iter() .filter_map(|e| match e { @@ -399,7 +400,24 @@ pub fn type_from_id(doc: &parse::Spirv, searched: u32) -> (String, Option .next() .expect("failed to find array length"); let len = len.iter().rev().fold(0u64, |a, &b| (a << 32) | b as u64); - return (format!("[{}; {}]", t, len), t_size.map(|s| s * len as usize), t_align); // FIXME: + let stride = doc.instructions.iter().filter_map(|e| match e { + parse::Instruction::Decorate{ + target_id, + decoration: enums::Decoration::DecorationArrayStride, + ref params, + } if *target_id == searched => { + Some(params[0]) + }, + _ => None, + }) + .next().expect("failed to find ArrayStride decoration"); + if stride as usize > t_size { + panic!("Not possible to generate a rust array with the correct alignment since the SPIR-V \ + ArrayStride is larger than the size of the array element in rust. Try wrapping \ + the array element in a struct or rounding up the size of a vector or matrix \ + (e.g. increase a vec3 to a vec4)") + } + return (format!("[{}; {}]", t, len), Some(t_size * len as usize), t_align); }, &parse::Instruction::TypeRuntimeArray { result_id, type_id } if result_id == searched => {