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

panic_implementation: no_mangle attribute needed? #51342

Closed
phil-opp opened this issue Jun 4, 2018 · 14 comments · Fixed by #51687
Closed

panic_implementation: no_mangle attribute needed? #51342

phil-opp opened this issue Jun 4, 2018 · 14 comments · Fixed by #51687

Comments

@phil-opp
Copy link
Contributor

phil-opp commented Jun 4, 2018

When using the new panic_implementation attribute I get a linker error:

  = note: lld: error: undefined symbol: rust_begin_unwind
          >>> referenced by core8-82115649ca0ebad190f7336e649e4ba4.rs
          >>>               core-4aa79020766f2e95.core8-82115649ca0ebad190f7336e649e4ba4.rs.rcgu.o:(core::panicking::panic_fmt::h73e9bd85b453f13d) in archive /…/target/sysroot/lib/rustlib/x86_64-blog_os/lib/libcore-4aa79020766f2e95.rlib

(I'm linking with LLD)

Adding the #[no_mangle] attribute seems to fix this linker error.

Is no_mangle required for the panic function or is this a bug?

@steveklabnik
Copy link
Member

I'm seeing this as well.

Additionally, adding no_mangle gives

warning: function is marked #[no_mangle], but not exported
 --> src\panic.rs:6:1
  |
6 |   fn panic(_info: &PanicInfo) -> ! {
  |   ^
  |   |
  |  _help: try making it public: `pub`
  | |
7 | |     unsafe { intrinsics::abort() }
8 | | }
  | |_^
  |
  = note: #[warn(private_no_mangle_fns)] on by default

but it still works. This implies to me that no_mangle shouldn't be needed, as the function doesn't need to be public to work.

@crawford
Copy link

I'm seeing the exact same behavior. I'm using arm-none-eabi-ld (2.28.0.20170620) to link.

@japaric
Copy link
Member

japaric commented Jun 20, 2018

Can we get more details (steps to reproduce)? Are you all using an allocator? I know there's a linker regression caused by the new-ish oom lang item, which I have reported in #51647. It could be that you are experiencing that bug and that this isn't related to the #[panic_implementation] stuff.

EDIT: I have been using #[panic_implementation] since it landed and have not run into any linker issue but I'm not using an allocator.

@steveklabnik
Copy link
Member

steveklabnik commented Jun 20, 2018

https://github.com/intermezzos/kernel is pretty small; download it, remove https://github.com/intermezzOS/kernel/blob/e6508a00ec0bc9ffa417d003d81ae63eceac2cf9/src/panic.rs#L5, and bootimage build

It pretty much happens on 'hello world'; you could strip even more out of the above, if it doesn't warn for you for some reason, I can minimize even further.

Not using an allocator or anything.

@japaric
Copy link
Member

japaric commented Jun 21, 2018

@steveklabnik thanks for the sample code. I see at least one bug there: rust_begin_unwind is not emitted when crate-type = bin -- I have reported this with minimal reproduction steps in #51671. I think --{start,end}-group is also missing in the linker invocation but that could be a different issue (or it may fix itself once #51671 is fixed).

japaric added a commit to japaric/rust that referenced this issue Jun 21, 2018
bors added a commit that referenced this issue Jun 28, 2018
translate / export weak lang items

see #51671 for details

fixes #51671
fixes #51342

r? @michaelwoerister or @alexcrichton
@phil-opp
Copy link
Contributor Author

phil-opp commented Jul 9, 2018

So I tried it with the latest nightly [1] and the issue still exists for me. To reproduce, try the panic-without-no-mangle branch of https://github.com/phil-opp/blog_os with either bootimage build or env RUST_TARGET_PATH=$(pwd) xargo build --target x86_64-blog_os. Edit: The travis build for that branch also shows the same error.

I also still get the error for intermezzOS.

[1]: rustc 1.29.0-nightly (960f604 2018-07-08)

@crawford
Copy link

Ah, thanks for reminding me to verify the fix. I tried removing #[no_mangle] from my code compiled with rustc 1.29.0-nightly (e06c87544 2018-07-06) and I'm hitting the same error as before.

@oli-obk oli-obk reopened this Jul 23, 2018
@japaric
Copy link
Member

japaric commented Jul 27, 2018

This might be related to #52795.

@alexcrichton
Copy link
Member

I believe this is fixed in #52993, where the answer is "no" you shouldn't need #[no_mangle]

alexcrichton added a commit to alexcrichton/rust that referenced this issue Aug 7, 2018
This commit tweaks the linker-level visibility of some lang items that rustc
uses and defines. Notably this means that `#[panic_implementation]` and
`#[alloc_error_handler]` functions are never marked as `internal`. It's up to
the linker to eliminate these, not rustc.

Additionally `#[global_allocator]` generated symbols are no longer forced to
`Default` visibility (fully exported), but rather they're relaxed to `Hidden`
visibility). This symbols are *not* needed across DLL boundaries, only as a
local implementation detail of the compiler-injected allocator symbols, so
`Hidden` should suffice.

Closes rust-lang#51342
Closes rust-lang#52795
bors added a commit that referenced this issue Aug 8, 2018
rustc: Tweak visibility of some lang items

This commit tweaks the linker-level visibility of some lang items that rustc
uses and defines. Notably this means that `#[panic_implementation]` and
`#[alloc_error_handler]` functions are never marked as `internal`. It's up to
the linker to eliminate these, not rustc.

Additionally `#[global_allocator]` generated symbols are no longer forced to
`Default` visibility (fully exported), but rather they're relaxed to `Hidden`
visibility). This symbols are *not* needed across DLL boundaries, only as a
local implementation detail of the compiler-injected allocator symbols, so
`Hidden` should suffice.

Closes #51342
Closes #52795
@phil-opp
Copy link
Contributor Author

I can confirm that it now works without no_mangle and pub. Thanks @alexcrichton!

@phil-opp
Copy link
Contributor Author

Seems like no_mangle is still required in release mode without LTO:

error: linking with `rust-lld` failed: exit code: 1
  |
  = note: rust-lld: error: undefined symbol: rust_oom
          >>> referenced by alloc.rs:224 (liballoc/alloc.rs:224)
          >>>               alloc-c69b28e77453f7a5.alloc.4j3hju2s-cgu.11.rcgu.o:(.text._ZN5alloc5alloc18handle_alloc_error17hcb467c6fec4cebfdE+0x0) in archive /home/philipp/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/thumbv7em-none-eabihf/lib/liballoc-c69b28e77453f7a5.rlib

          
          rust-lld: error: undefined symbol: rust_begin_unwind
          >>> referenced by panicking.rs:77 (libcore/panicking.rs:77)
          >>>               core-59aaa68b41c4baf4.core.82zizo12-cgu.9.rcgu.o:(core::panicking::panic_fmt::h380215f9f52a35ae) in archive /home/philipp/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/thumbv7em-none-eabihf/lib/libcore-59aaa68b41c4baf4.rlib

It works fine in debug mode and in release mode with LTO. When I add no_mangle, it also works in release mode without LTO. I'm using rustc 1.30.0-nightly (33b923fd4 2018-08-18).

@alexcrichton
Copy link
Member

@phil-opp oh dear! Is it possible to get an example I could poke around with?

@phil-opp
Copy link
Contributor Author

I created a minimal embedded project here: https://github.com/phil-opp/rust-issue-51342-example

> rustup override add nightly
> rustup target add thumbv7em-none-eabihf
> cargo build                                 # works fine
> cargo rustc --release -- -C lto             # works fine
> cargo build --release                       # errors

@alexcrichton
Copy link
Member

Oops, thanks for the report! That should be fixed in #53640

alexcrichton added a commit to alexcrichton/rust that referenced this issue Aug 26, 2018
In investigating [an issue][1] with `panic_implementation` defined in an
executable that's optimized I once again got to rethinking a bit about the
`rustc_std_internal_symbol` attribute as well as weak lang items. We've sort of
been non-stop tweaking these items ever since their inception, and this
continues to the trend.

The crux of the bug was that in the reachability we have a [different branch][2]
for non-library builds which meant that weak lang items (and std internal
symbols) weren't considered reachable, causing them to get eliminiated by
ThinLTO passes. The fix was to basically tweak that branch to consider these
symbols to ensure that they're propagated all the way to the linker.

Along the way I've attempted to erode the distinction between std internal
symbols and weak lang items by having weak lang items automatically configure
fields of `CodegenFnAttrs`. That way most code no longer even considers weak
lang items and they're simply considered normal functions with attributes about
the ABI.

In the end this fixes the final comment of rust-lang#51342

[1]: rust-lang#51342 (comment)
[2]: https://github.com/rust-lang/rust/blob/35bf1ae25799a4e62131159f052e0a3cbd27c960/src/librustc/middle/reachable.rs#L225-L238
bors added a commit that referenced this issue Aug 27, 2018
…ister

rustc: Continue to tweak "std internal symbols"

In investigating [an issue][1] with `panic_implementation` defined in an
executable that's optimized I once again got to rethinking a bit about the
`rustc_std_internal_symbol` attribute as well as weak lang items. We've sort of
been non-stop tweaking these items ever since their inception, and this
continues to the trend.

The crux of the bug was that in the reachability we have a [different branch][2]
for non-library builds which meant that weak lang items (and std internal
symbols) weren't considered reachable, causing them to get eliminiated by
ThinLTO passes. The fix was to basically tweak that branch to consider these
symbols to ensure that they're propagated all the way to the linker.

Along the way I've attempted to erode the distinction between std internal
symbols and weak lang items by having weak lang items automatically configure
fields of `CodegenFnAttrs`. That way most code no longer even considers weak
lang items and they're simply considered normal functions with attributes about
the ABI.

In the end this fixes the final comment of #51342

[1]: #51342 (comment)
[2]: https://github.com/rust-lang/rust/blob/35bf1ae25799a4e62131159f052e0a3cbd27c960/src/librustc/middle/reachable.rs#L225-L238
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.

6 participants