Warning safe_packed_borrows will become a hard error

[robert-w-gries]

Warning

While restarting a garbage Travis build, I noticed the following warnings:

warning: #[derive] can't be used on a non-Copy #[repr(packed)] struct (error E0133)
 --> src/header.rs:1:10
  |
1 | #[derive(Debug)]
  |          ^^^^^
  |
  = note: #[warn(safe_packed_borrows)] 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 #46043 <https://github.com/rust-lang/rust/issues/46043>

warning: borrow of packed field requires unsafe function or block (error E0133)
 --> src/elf_sections.rs:9:5
  |
9 |     assert_eq!(9, tag.typ);
  |     ^^^^^^^^^^^^^^^^^^^^^^^
  |
  = 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 #46043 <https://github.com/rust-lang/rust/issues/46043>
  = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

There are around 20 more #[derive(Debug)] warnings that I snipped for brevity.

#[repr(packed)]

The warning is related to our usage of #[repr(packed)] in the crate. There is undefined behavior associated with derefencing unaligned structs, which rust-lang/rust#46043 will prevent with a compiler error.

How to Fix

From the rust issue:

The most common ways to fix this warnings are:

1. remove the #[repr(packed)] attribute if it is not actually needed. A few crates had unnecessary #[repr(packed)] annotations - for example, tendril.
2. copy the fields to a local first. When accessing a field of a packed struct directly without using a borrow, the compiler will make sure the access is done correctly even when the field is unaligned. The then-aligned local can then be freely used. For example:

let x = Foo { start: 0, data: Unaligned(1) };
let temp = x.data.0;
println!("{}", temp); // works
// or, to the same effect, using an `{x}` block:
println!("{}", {x.data.0}); // works
use(y);

One annoying case where this problem can appear is if a packed struct has builtin derives

If your struct already derives Copy and has no generics, the compiler will generate code that copies the fields to locals and will therefore work. Otherwise, you'll have to write the derive implementations manually.

Feedback

Before submitting a PR, I'd like feedback on the following:

1. Do our impacted structs need to use#[repr(packed)]? It seems that #[repr(packed)] should be a last resort instead of a feature liberally used.
2. Can we implement Copy for our affected structs? Or do we need to manually implement Debug for those structs?

[phil-opp]

Thanks a lot for reporting!

I think we're using repr(packed) to deliberately, at least in some cases. The compiler shouldn't insert padding unless there are misaligned fields (e.g. an u32 after an u8). So I think we should be able to remove a lot of repr(packed) attributes. To be sure that we don't break anything, we could add tests that check the struct sizes. Implementing Copy should also be possible.