rodrimati1992/const_panic

Why does this crate use packed representation in a bunch of places?

Opened this issue · 5 comments

jplatte commented

A colleague of mine got this linker error today:

ld: warning: alignment (1) of atom 'l_anon.e8717737f2aafb2fc9f63ff91b30211b.2' is too small and may result in unaligned pointers 
ld: warning: pointer not aligned at address 0x10464FB19 ('l_anon.e8717737f2aafb2fc9f63ff91b30211b.2' + 1 from /Users/[...]/libmatrix_sdk_ffi.a(const_panic-ef6c6c5eecaca335.const_panic.519cbb813ef9124e-cgu.8.rcgu.o))
ld: unaligned pointer(s) for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

First I thought that it seems really unlikely for const_panic to actually be related to the problem, but when looking at the codebase a bit I found a few uses of #[repr(packed)] as well as one Packed<&'a str> in PanicVariant that seems highly suspicious. What is the reason for using packed representation, in particular there? Is it just for efficiency?

rodrimati1992 commented

Why would this error happen? Since I'm not using unsafe to pack or access the pointer, this can't cause UB (barring unsoundness bugs in rustc).

jplatte commented

I'm far from an expert on linking, so I don't really know why the linker ever cares about alignment.

But separately, I wouldn't be surprised at all if #[repr(packed)] can (still) cause UB even without unsafe accesses, similar to #[no_mangle].

  • 👀1
rodrimati1992 commented

Well, #[no_mangle] triggers the unsafe_code lint while #[repr(packed)] doesn't, so it's considered safe by the compiler.

no_mangle example

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=d9a41beab1c5737a04dc4820f54184c9

#![deny(unsafe_code)]

#[no_mangle]
fn foo(){}
error: declaration of a `no_mangle` function
 --> src/lib.rs:3:1
  |
3 | #[no_mangle]
  | ^^^^^^^^^^^^
  |
  = note: the linker's behavior with multiple libraries exporting duplicate symbol names is undefined and Rust cannot provide guarantees when you manually override them
note: the lint level is defined here
 --> src/lib.rs:1:9
  |
1 | #![deny(unsafe_code)]
  |         ^^^^^^^^^^^

#[repr(packed)] example

This example builds without triggering the lint:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=0bd9127194d0d9a1b14e35e976767f07

#![deny(unsafe_code)]

#[repr(packed)]
pub struct Foo(pub u8, pub u32, pub &'static str);

pub static F: Foo = Foo(3, 5, "hello");
jplatte commented

Right.. Well the error from the issue description has now infected our CI and my small PR fixes it there as well.

If you don't want to merge without actually finding out why it might be wrong, I can understand. Maybe my colleage who has started to implement their own linker has some insights, I'll ask them.

rodrimati1992 commented

If you don't want to merge without actually finding out why it might be wrong

Yes, I want to know why it happens, to know whether the PR is a fix for a const_panic bug or a workaround for a compiler bug.

  • 👍1