Trait with Attributes

I have the following trait:

pub trait Vertex {
  pub fn attrs() -> Vec<(bool, usize, AttributeKind)>;
  pub fn new() -> Self;
}

I would like to enforce that any struct that implements that trait requires the repr() attribute like so:

#[repr(C, packed)]
pub struct Vertex2 {
  pub x: f32,
  pub y: f32,
}

impl Vertex for Vertex2 {
  fn attrs() -> Vec<(bool, usize, AttributeKind)> {
    vec![
      (false, 2, AttributeKind::Float),
    ]
  }
	
  fn new() -> Vertex {
    Vertex { x: 0, y: 0 }
  }
}

This works great, but is there any way that I can add a constraint so that if #[repr(C, packed)] was not provided, compilation would fail?

a previous thread on the same issue suggests creating an additional unsafe trait as a way for implementers to "sign off" on "yes i'm doing this correctly".

Thanks for the link. I missed that in my searches online and on these forums.

1 Like

packed is very dangerous to use correctly / without UB in Rust, why do you need it?

For this kind of pattern the best thing is a (derive) macro and an unsafe trait, the latter being auto-implemented by the macro after it has performed some checks:

#[doc(hidden)] pub use ::core; // at the root of the library

#[doc(hidden)]
#[allow(nonstandard_style)]
#[derive(Clone, Copy)]
struct struct_padding;

/// # Safety
///
/// Implementors of this marker trait must be `#[repr(C)]`
/// and have no padding
pub
unsafe trait IsReprCWithoutPadding {}

pub
trait Vertex : IsReprCWithoutPadding { ... }

macro_rules! IsReprCWithoutPadding {(
    #[repr(C)]
    $( #[$struct_meta:meta] )*
    $struct_vis:vis
    struct $StructName:ident {
        $(
            $( #[$field_meta:meta] )*
            $field_vis:vis
            $field_name:ident : $field_ty:ty
        ),* $(,)?
    }
) => (
    /// Check that the structure has no padding
    const _: [$crate::struct_padding; 0] = [$crate::struct_padding;
        $crate::core::mem::size_of::<$StructName>()
            $(- $crate::core::mem::size_of::<$field_ty>() )*
    ];

    unsafe
    impl $crate::IsReprCWithoutPadding for $StructName
    {}       
)}

and then you and downstream users can safely derive the marker trait using #[macro_rules_derive]

#[macro_use] extern crate macro_rules_attribute;

/// OK
#[macro_rules_derive(IsReprCWithoutPadding!)]
#[repr(C)]
struct Vertex2 {
    x: f32,
    y: f32,
}

/// Error, expected type `[struct_padding; 0]`,
/// found type `[struct_padding; 3]`
#[macro_rules_derive(IsReprCWithoutPadding!)]
#[repr(C)]
struct Fails {
    x: u8,
    y: f32,
}
5 Likes

By making my struct packed, is the entire struct considered unsafe at that point? I wanted to compose my Vertex struct into vectors instead of raw scalars. This way, vector positions and coordinates get all the vector math/operator benefits, but that also means that the Vector types in my 3D math library also need to be packed. Here's what a more complex Vertex type looks like:

#[repr(C, packed)]
pub struct SpriteVertex {
    pub pos: Vector3,
    pub coord: Vector2,
    pub color: Color32b,
}

What kind of vulnerabilities do packed typed cause? My vector types were safe before I packed them. When I added the packed attributes to them, I had to wrap a few of my method implementations in unsafe {} blocks, but not as much as I'd thought. I haven't used my vector types too much yet. Do I need to wrap all of those in unsafe now?

without UB in Rust, why do you need it?

What is UB?

EDIT: @Yandros I forgot to finish this post last night when I went to bed. The gl crate requires Vertex structs to be #[repr(C, packed)] because when the data is uploaded to the data to the GPU, we need to specify which parts of each vertex's data maps to which vertex attributes in the shader. The shader is a program that gets executed on the GPU. It's got different stages of execution, and the first stage is the taking vertex data, and processing them for rasterization (in most cases, but there are optional steps between vertex and rasterization).

That said, you and @TomP have convinced me that it's a bad idea to force my math vectors to be packed, C representations. I will revert that in my crate. I thought it'd be a convenience, but now it's feeling more like over-engineering at the cost of some serious compromises.

UB stands for undefined behaviour and is the main thing that rust aims to prevent.

#[repr(packed)] can cause undefined behaviour because it's fields are not necessarily aligned properly. Since the representation eliminates padding which would have made the fields properly aligned, reading the fields using references, or directly from the struct is unsafe since they may be improperly aligned.

1 Like

One way to determine whether changing a repr(C) struct to repr(C, packed) will lead to UB is to create two similar structs, one repr(C) and the other repr(C, packed), and to compare their sizes. Since the repr(C)s prevent Rust from reordering the fields, if the two are of equal size then there is no padding within the packed struct, though there might be some padding after the final field of each.

To follow up on UB, the presence of UB in your code releases the LLVM backend of the compiler from any obligation to compile your code to do what you intended. There are many threads on this forum that delve into this, explaining why highly-optimizing compilers require that there be UB, and pointing out that whether your code "works" by your definition, or not, is totally unpredictable when UB is present.

1 Like

Ok, so the C param of the repr() macro prevents the compiler from reordering fields within a struct for memory-alignment purposes, right? Then, the packed param disallows the compiler from inserting padding bytes between fields, right?

So, if I was compiling against a hardware target prefers 32-bit-aligned data, and I had 3 versions of the same struct like this:

struct Vertex {
  red: u8,
  green: u8,
  blue: u8,
  x: f32,
  y: f32,
  norm_x: u16,
  norm_y: u16,
  norm_z: u16,
}

#[repr(C)]
struct CVertex {
  red: u8,
  green: u8,
  blue: u8,
  x: f32,
  y: f32,
  norm_x: u16,
  norm_y: u16,
  norm_z: u16,
}

#[repr(C, packed)]
struct CPackedVertex {
  red: u8,
  green: u8,
  blue: u8,
  x: f32,
  y: f32,
  norm_x: u16,
  norm_y: u16,
  norm_z: u16,
}

Each struct is identical, and so it defines 17 bytes of data. Since the hardware wants data 32-bit aligned for best performance, it'll probably reorder Vertex to something like this maybe?

struct Vertex {
  red: u8,
  green: u8,
  blue: u8,
  pad_1: [u8; 1], // pad 1 byte

  norm_x: u16,
  norm_y: u16,
  norm_z: u16,
  pad_2: [u8; 2], // padding 2 bytes

  x: f32,

  y: f32,
} // 20 bytes total

Next is CVertex:

struct CVertex {
  red: u8,
  green: u8,
  blue: u8,
  pad_1: [u8; 1], // pad 1 byte

  norm_x: u16,
  norm_y: u16,
  norm_z: u16,
  pad_2: [u8; 2], // pad 2 bytes

  x: f32,

  y: f32,
} // 20 bytes total

Finally CPackedVertex:

struct CPackedVertex {
  red: u8,
  green: u8,
  blue: u8,
  x: f32,
  y: f32,
  norm_x: u16,
  norm_y: u16,
  norm_z: u16,
  pad: [u8; 3], // pad 3 bytes
} // 20 bytes total

Does that seem right? I have a little bit of understanding on how compilers might pack and pad data, but it's fuzzy at best. It looks like all structs came out to the same size in this case. I couldn't think of a case where there'd be a deviation from overall size between CVertex and CPackedVertex, but I could think of scenarios where those two structs would deviate in size when compared against Vertex. Am I missing something?

I would expect the actual layout of these to be

struct Vertex {
  x: f32,
  y: f32,
  norm_x: u16,
  norm_y: u16,
  norm_z: u16,
  red: u8,
  green: u8,
  blue: u8,
  pad: [u8; 3], // padding 3 bytes
} // 20 bytes total

where the fields have been reordered in memory relative to the order of their declaration.

For CVertex

struct CVertex {
  red: u8,
  green: u8,
  blue: u8,
  pad_1: [u8; 1], // pad 1 byte
  x: f32,
  y: f32,
  norm_x: u16,
  norm_y: u16,
  norm_z: u16,
  pad_2: [u8; 2], // padding 2 bytes
} // 20 bytes total

struct CPackedVertex would be as you have stated.

Note that in struct CPackedVertex the f32s and u16s require unaligned access, which generally leads to slower code and often more of it.

Ok, so it looks like I had my ordering backwards. Instead of ordering the fields from smaller-to-greater sizes, it's actually the opposite, which makes sense. That way, the data that has higher potential for misalignment is towards the bottom where the padding would go.

I can kind of see why u16 needs misaligned access, because it's not taking up an entire aligned block in memory (assuming 32-bit alignment), but why would that be the case for f32? My only guess is that because it's an IEEE float, and that encoding spills across byte boundaries? That's my honest stab in the dark though.

Layout examples with byte offsets from the start of the record should suffice:

struct Vertex {
0x00:   x: f32,
0x04:   y: f32,
0x08:   norm_x: u16,
0x0a:   norm_y: u16,
0x0c:   norm_z: u16,
0x0e:   red: u8,
0x0f:   green: u8,
0x10:   blue: u8,
0x11:   pad: [u8; 3], // padding 3 bytes
} // 0x14 (20) bytes total
struct CPackedVertex {
0x00:   red: u8,
0x01:   green: u8,
0x02:   blue: u8,
0x03:   x: f32,
0x07:   y: f32,
0x0b:   norm_x: u16,
0x0d:   norm_y: u16,
0x0f:   norm_z: u16,
0x11:   pad: [u8; 3], // padding 3 bytes
} // 0x14 (20) bytes total

Note that in the latter struct all f32s and u16s are misaligned.

1 Like

Oh! I was looking at the wrong struct. I somehow managed to mix up the Vertex and CPackedVertex. Jeez, I even remember thinking that as I was writing out the layouts in that lengthy reply.

Sorry about that. I get it now. I'll probably revert my 3D math crate back to being safe. That said, there probably isn't wrong with my vectors and matrices being C or packed, right? All vector components and matrix elements will be the same type, and they should always be primitives. Primitives will guarantee that each component/element size will be a power of 2, so no misalignment for things that could have been aligned otherwise.

It's the vertex struct where things get crazy, and unfortunately, I need them to be unsafe because they exist as a means to copy data to and from the GPU via the gl crate. The gl crate requires all vertex data to be C, packed vertex structures, so that we can map the vertices' vertex attribute descriptors to them. That way, the GPU knows which parts of the vertex is position data, color data, normals, etc, so it can map them to the vertex attributes in the shader (program that runs on the GPU clusters).

If you're using a GPU, I'm surprised that you're not using a 4-component RGBA pixel, which in the above would be a 32-bit value that you would align the same as the f32s. See this old post for example.

I use 32-bit pixels in practice. I omitted the alpha channel in this example to throw some more entropy into the Vertex examples to better understand where packing issues will arise.

EDIT: Thanks for posting that link too. That actually answered another question I had about converting an array of bytes into an array of structs. It's not as straightforward as a map() since the input length is a multiple of the output length (in this case, collapsing every 4 elements/bytes of the input into a single struct instance).