Skip to content
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

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

Open
jplatte opened this issue Jul 13, 2023 · 5 comments · May be fixed by #10
Open

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

jplatte opened this issue Jul 13, 2023 · 5 comments · May be fixed by #10

Comments

@jplatte
Copy link

jplatte commented Jul 13, 2023

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?

@jplatte jplatte linked a pull request Jul 13, 2023 that will close this issue
@rodrimati1992
Copy link
Owner

rodrimati1992 commented Jul 13, 2023

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
Copy link
Author

jplatte commented Jul 13, 2023

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].

@rodrimati1992
Copy link
Owner

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
Copy link
Author

jplatte commented Jul 14, 2023

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
Copy link
Owner

rodrimati1992 commented Jul 14, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants