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

Recent nightly versions break thumb* targets #62781

Closed
jamesmunns opened this issue Jul 18, 2019 · 14 comments
Closed

Recent nightly versions break thumb* targets #62781

jamesmunns opened this issue Jul 18, 2019 · 14 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jamesmunns
Copy link
Member

Hey all,

I've noticed in nightly-2019-07-18, rustc has begun producing symbols in the .ARM.exidx region, which it did not previously do. As our standard linker script does not have a linker rule for this region, building fails with with the following error:

  = note: rust-lld: error: no memory region specified for section '.ARM.exidx'

To reproduce this, this project can be used: https://github.com/ferrous-systems/embedded-trainings/tree/master/beginner/templates/segment-1. You will need to add the thumbv7em-none-eabihf target first. This builds with the current and previous stable releases (1.32 <= version <= 1.36), but does not build with nightly-2019-07-18. This also seems fine with rustc 1.37.0-beta.3 (2ba6de7 2019-07-12). I have not yet attempted to bisect this.

We've had a previous similar issue here: rust-embedded/cortex-m-rt#157 - and the fix was to discard that section in our linker script template. However I first wanted to check if this new output was expected, because it would cause a stable to stable regression if folks don't update the cortex-m-rt crate, which includes the linker script template.

@jonas-schievink jonas-schievink added C-bug Category: This is a bug. O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-nominated labels Jul 18, 2019
@jamesmunns
Copy link
Member Author

jamesmunns commented Jul 18, 2019

Bisecting, this behavior showed up between:

  • nightly-2019-07-17 - rustc 1.38.0-nightly (07e0c36 2019-07-16) - OK
  • nightly-2019-07-18 - rustc 1.38.0-nightly (bc2e84c 2019-07-17) - NOT OK

EDIT:

link to diff of commits

@jonas-schievink
Copy link
Contributor

Possibly caused by the LLVM update in #62592 – cc @nikic

It seems unexpected to me that this section is now included, since embedded projects are built with panic = abort

@nikic
Copy link
Contributor

nikic commented Jul 18, 2019

Based on https://lists.llvm.org/pipermail/llvm-dev/2019-March/130669.html it seems like the emission of .ARM.exidx is by design, even if no unwinding is used and the suggestion there is to discard in the linker.

@jonas-schievink jonas-schievink added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Jul 18, 2019
@nikomatsakis
Copy link
Contributor

Update from compiler team triage:

I can't tell quite what's going on here. Is this a compiler bug or something to be fixed downstream? @nikic's comment suggests the latter -- @jamesmunns if that is true, can you close the issue?

@nikomatsakis
Copy link
Contributor

Marking as P-high so we check up on this

@nikomatsakis nikomatsakis added P-high High priority and removed I-nominated labels Jul 25, 2019
@jamesmunns
Copy link
Member Author

jamesmunns commented Jul 25, 2019

So in general:

  • This does appear to be an expected change in LLVM9
    • It's not a bug, just not something that was emitted by rustc/llvm before, so we have no handling for this section in our linker scripts
  • Current versions of widely (nearly universally) used crates that are fundamental for embedded systems (e.g. cortex-m-rt, which provides a linker script generation template, among other things) do not handle this change, and are broken by versions of rustc that include LLVM9.
  • We have submitted and landed a PR to cortex-m-rt to handle this (updating the linker script template we use), but have not yet released an updated crate for this
    • EDIT: We have tagged a release candidate as of 2019-07-25, but have not yet published on crates.io.
  • Users will experience broken builds with confusing errors if they update from stable to stable, unless they also update their version of cortex-m-rt, once an update has been released.

This is an odd case, since I suppose the list of linker sections produced by rustc isn't really a "stabilized decision", but this will be mildly annoying for embedded users. I'd say our best handling responses to this would be:

  • Find a way to disable this section when abort = panic, either through Rust or configuration of LLVM9
    • It is unclear if this is possible at all
  • Patch LLVM to not emit .ARM.exidx sections when abort = panic
    • This has an unknown, and likely unfortunate amount of effort associated with it
  • Do some kind of post-processing (invocation flags to LLD maybe?) to prevent this from happening
    • No idea if this is possible
  • Deal with the breakage, do our best to put out information that upgrades to cortex-m-rt will be needed, hopefully making the fix searchable, and the knowledge well-known before LLVM9 lands in stable.
    • Annoying stable-to-stable regression for embedded users

@jonas-schievink
Copy link
Contributor

Deal with the breakage, do our best to put out information that upgrades to cortex-m-rt will be needed

To help out with this, it might help to backport the fix and publish 0.5.8, since 0.5.x is still used by quite a few crates.

@skade
Copy link
Contributor

skade commented Jul 29, 2019

Phew, I see multiple lines of arguing where this would break the language semver.

While not strictly compiler breakage, I think linking behaviour is also a big part of our compiler story.

@nagisa
Copy link
Member

nagisa commented Aug 1, 2019

I personally do not thing this should be considered a breaking change (much like inference changes are not breaking), but passing on the ball to T-lang to have their say on the topic.

@nagisa nagisa added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Aug 1, 2019
@Centril
Copy link
Contributor

Centril commented Aug 1, 2019

Me, @cramertj, and @qmx (=P) discussed this on the language team meeting and we agree with @nagisa. Attendance was spotty however and @joshtriplett may have thoughts.

@Centril Centril removed I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Aug 1, 2019
@nikic
Copy link
Contributor

nikic commented Aug 1, 2019

After seeing the fix I realize that I misunderstood the original report -- this isn't about .ARM.exidx sections being emitted, but specifically about .ARM.exidx without a suffix, which is a special-case that was not handled in the cortex-m-rt linker script. (The relevant change for this on LLVM side is likely https://reviews.llvm.org/D59216.)

The ARM exception handling ABI specification says "The index table section name must be .ARM.exidx optionally followed by further characters", which permits using the name without a suffix, so LLVM isn't doing anything wrong here -- the linker script was relying on an implementation detail that was not guaranteed by the relevant specification.

@jamesmunns
Copy link
Member Author

Thanks all for taking a look at this! I'll circle up with the embedded-wg folks to make sure there isn't any other detail that I've neglected to mention, but this seems like a reasonable response.

I'm guessing the LLVM9 update is riding the trains currently, so will be in Beta in 2 weeks, and stable in 8 weeks? That gives us a timeline to make sure we have this patched and released in cortex-m-rt 0.6.x (and likely backport changes to 0.5.x), and publish some "heads up" wherever possible.

@skade
Copy link
Contributor

skade commented Aug 5, 2019

Reading @nikic response: ignore my concerns above.

@jamesmunns
Copy link
Member Author

Closing this issue, I haven't heard anything more from embedded folks. Thanks again all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants