Derive `defmt::Format` for structures with unaligned fields?

[tobywf]

When a structure has unaligned fields, e.g. to read structures from wire formats, #[derive(defmt::Format)] causes an unaligned read error with recent Rust versions (e.g. on 1.77.1 or 1.78.0):

#[repr(C, packed)]
#[derive(defmt::Format)]
pub struct Packet {
    pub length: u8,
    pub ty: u8,
    pub data: u16,
    // ...
}

error[E0793]: reference to packed field is unaligned
   --> src/main.rs:12:9
    |
120 |     pub data: u16,
    |         ^^^^
    |
    = note: packed structs are only aligned by one byte, and many modern architectures penalize unaligned field accesses
    = note: creating a misaligned reference is undefined behavior (even if that reference is never dereferenced)
    = help: copy the field contents to a local variable, or replace the reference with a raw pointer and use `read_unaligned`/`write_unaligned` (loads and stores via `*p` must be properly aligned even when using raw pointers)

Expanding the macro, it's because of the destructuring of the fields:

impl defmt::Format for Packet {
    fn format(&self, f: defmt::Formatter) {
        defmt::unreachable!()
    }
    fn _format_tag() -> defmt::Str {
        // ...
    }
    fn _format_data(&self) {
        match self {
            Self {
                length,
                ty,
                data, // <--- here
            } => {
                defmt::export::u8(length);
                defmt::export::u8(ty);
                defmt::export::u16(data);
            }
        }
    }
}

A solution could be something like this:

fn _format_data(&self) {
    defmt::export::u8(&{ self.length });
    defmt::export::u8(&{ self.ty });
    defmt::export::u16(&{ self.data });
}

Obviously, for most cases that is worse since it copies the fields and requires fields to be Copy. Maybe you'd consider supporting it via an attribute on the existing derive proc-macro or a new proc-macro?

Understandable if you don't think it's worth it. My current work-around is a proc-macro to derive a custom defmt::write! implementation, which works fine. Ultimately, I don't know how common this use-case it.

[Urhengulas]

I think this is a good addition.

Is is possible to detect if a struct is packed from within a derive macro? Then we could just generate a different implementation based on that information.

Otherwise I think we have three options for the API:

1. New derive macro FormatPacked
2. Derive helper attribute #[defmt(packed)] on the struct
3. Derive helper attribute #[defmt(packed)] on the field

// Option 1
#[repr(C, packed)]
#[derive(defmt::FormatPacked)]
pub struct Packet {
    pub length: u8,
    pub ty: u8,
    pub data: u16,
}

// Option 2
#[repr(C, packed)]
#[derive(defmt::Format)]
#[defmt(packed)]
pub struct Packet {
    pub length: u8,
    pub ty: u8,
    pub data: u16,
}

// Option 3
#[repr(C, packed)]
#[derive(defmt::Format)]
pub struct Packet {
    pub length: u8,
    pub ty: u8,
    #[defmt(packed)]
    pub data: u16,
}

I prefer option 2, because it keeps things simple by not introducing another macro. Also it allows adding more options in a non-breaking way.

While option 3 would allow to only copy the necessary fields, I think it is too tedious to mark all the fields that are not u8. And maybe we can still minimize copying by checking the type.

What do you think?

[tobywf]

Is is possible to detect if a struct is packed from within a derive macro? Then we could just generate a different implementation based on that information.

I just looked into this a bit. syn::DeriveInput does include the struct's attributes. So it's possible to check if the struct has #[repr(packed)] (or similar). There are two alignment modifiers (The Rust Reference), but as we'll see later, rustc only uses packed for Debug.

I don't really have a strong opinion. Option 1 is nice because it's explicit, but don't "just work". Option 2 is nice because it's automatic, but introduces Copy bounds on types and incurs a slight penalty. Option 3 is nice, because only some fields would need the extra copy... wait, right?

Ugh, so turns out #[repr(packed)] is weirder than I expected, but likely you knew this already? The Rustonomicon warns about using it, and e.g. Rust issue #87368 is an example. To summarize, packed also changes how the structure can be aligned. Take the example I gave. Intuitively, access to the data field should be aligned, because on many platforms, u16 has an alignment of 2. Yet because packed also sets the structures alignment, Packet could have any offset, so anything other than u8 (or the same layout) could be misaligned. I think a lot of code misuses packed, like the code I was working with. Anyway, so packed should probably not be used as much as I thought, and that does change how valuable supporting #[repr(packed)] is.

Given all the issues around packed, I think it would also be reasonable to not implement support for that.

---

If defmt wants to follow how std::fmt::Debug/core::fmt::Debug works, then option 2 makes the most sense. I expanded the #[derive(Debug)] for a packed/not packed struct:

// #[repr(C)]
impl ::core::fmt::Debug for Packet {
    #[inline]
    fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
        ::core::fmt::Formatter::debug_struct_field3_finish(
            f,
            "Packet",
            "length",
            &self.length,
            "ty",
            &self.ty,
            "data",
            &&self.data,
        )
    }
}

// #[repr(C, packed)]
impl ::core::fmt::Debug for Packet {
    #[inline]
    fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
        ::core::fmt::Formatter::debug_struct_field3_finish(
            f,
            "Packet",
            "length",
            &{ self.length },
            "ty",
            &{ self.ty },
            "data",
            &&{ self.data },
        )
    }
}

I'm unsure why the last field is expanded to &&, but it happens consistently. Note that the expansion must be done with e.g. cargo expand or rustc --profile=check -- -Zunpretty=expanded, as rust-analyzer incorrectly expands Debug for packed structs.

rustc only checks if the struct has the packed attribute. Also interesting is create_struct_field_access_fields, which has two exceptions for str and [u8].

So that would suggest option 2.

[Urhengulas]

If std::fmt::Debug supports it, it is reasonable for us to do so as well. We try to follow std::fmt where it makes sense.

[Urhengulas]

Are you interested in implementing this? I do not have capacity for this right now.

[Urhengulas]

Well ... I asked around and using repr(packed) seems indeed to be frowned upon. Let's not support it in the derive macro then. If people really want to use it, they can always implement defmt::Format manually.