Skip to content

Implemented derives fail when packed #2221

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chrysn opened this issue Jun 7, 2022 · 1 comment
Open

Implemented derives fail when packed #2221

chrysn opened this issue Jun 7, 2022 · 1 comment

Comments

@chrysn
Copy link
Contributor

chrysn commented Jun 7, 2022

This is the "it also applies to impl_derive" version of #2200. Long story short: The compiler now catches what used to be UB-but-worked when accessing packed fields through possibly unaligned pointers, and is now strict about that, breaking Debug on packed structs all around.

Input C Header

struct packed_wo_copy {
        short f1;
        char f2[0];
} __attribute__((packed));

This is exactly copied from #2200.

That's not exactly the case that triggered errors for me, but that one involved nested types with attribute aligned and whatsonot -- effect should be the same.

Bindgen Invocation

Diverging from #2200, we now use impl_debug:

$ bindgen test.h --impl-debug

Actual Results

[...]
impl ::std::fmt::Debug for packed_wo_copy {
    fn fmt(&self, f: &mut ::std::fmt::Formatter<'_>) -> ::std::fmt::Result {
        write!(
            f,
            "packed_wo_copy {{ f1: {:?}, f2: {:?} }}",
            self.f1, self.f2
        )
    }
}
[...]

and when compiling:

error: reference to packed field is unaligned
  --> /dev/stdin:90:13
   |
90 |             self.f1, self.f2
   |             ^^^^^^^
   |
   = note: `#[deny(unaligned_references)]` on by default
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #82523 <https://github.com/rust-lang/rust/issues/82523>
   = note: fields of packed structs are not properly aligned, and 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)
   = note: this error originates in the macro `$crate::format_args` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

Expected Results

For the f1 part, bindgen should see that it is Copy and thus apply the common misalignment workaround of putting it in curly braces when used:

        write!(
            f,
            "packed_wo_copy {{ f1: {:?}, f2: {:?} }}",
            { self.f1 }, self.f2
        )

For the f2 argument, a good fallback might be to not emit anything, or to emit a text value (eg. resulting in packed_wo_copy { f1: 42, f2: not printable }). There might be sharper ways to salvage even that case (eg. check whether it is pinned, if not "temporarily move it out"), but restoring the regressed behavior to give some implementation of Debug is probably more pressing.


Some notes on where to fix this were already left in #1491 (comment) (from where this issue is split out).

chrysn added a commit to RIOT-OS/rust-riot-sys that referenced this issue Jun 8, 2022
This restores buildability with nightly releases after around 2022-04
when it became stricter around unaligned access.

Workaround-For: rust-lang/rust-bindgen#2221
See-Also: #3
@TheButlah
Copy link

TheButlah commented Jun 26, 2023

This bit me today, as it seems to be a hard error as of rust 1.69 (latest stable is 1.70 now). There is probably more old code out there in the wild that is breaking as we speak, without an easy migration path

chrysn added a commit to RIOT-OS/rust-riot-sys that referenced this issue Dec 28, 2024
These are more instances where [2221] bites, which will be introduced
when RIOT updates to Nimble 1.8 (as is being worked on in [21108])

[2221]: rust-lang/rust-bindgen#2221
[21108]: RIOT-OS/RIOT#21108
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants