Skip to content

Conversation

jonas-schievink
Copy link
Contributor

@jonas-schievink jonas-schievink commented Feb 17, 2020

This should hopefully fix issues #69231 and rust-embedded/cortex-m-rt#139.

I couldn't test this locally as the rustc I produced does not create binaries (no idea why). Resolved.

@rust-highfive
Copy link
Contributor

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 17, 2020
@jonas-schievink
Copy link
Contributor Author

r? @hanna-kruppe

@hanna-kruppe
Copy link
Contributor

LGTM (modulo nit), but I'm not 100% sure about signing off such a change to a target (family) that I don't really use or know. Can we get a second opinion from someone versed in the targets this affects? I suppose I could also rubber-stamp and we'll see if anyone complains about their program becoming more bloated.

@jonas-schievink
Copy link
Contributor Author

jonas-schievink commented Feb 17, 2020

As it stands, this will only apply to the microcontroller targets, not the other thumb targets (which are thumbv7a_pc_windows_msvc, and thumbv7 linux/android targets), so the impact should at least be limited to embedded apps only. We do care about program sizes there, of course, but I think this is a worthwhile tradeoff to make, especially since users can still override it.

Can we get a second opinion from someone versed in the targets this affects?

I mentioned the this fix to @japaric at work, who I believe added these targets originally, so perhaps I can r? @japaric

@japaric
Copy link
Contributor

japaric commented Feb 18, 2020

I checked the output machine code of libcore.rlib and the change makes what we want. diff below:

 00000000 <core::panicking::panic>:
+        b580            push    {r7, lr}
+        466f            mov     r7, sp
         b088            sub     sp, #32
         4694            mov     ip, r2
         f240 0200       movw    r2, #0

I couldn't test this locally as the rustc I produced does not create binaries (no idea why).

AFAICT, LLD does not produce a final artifact with these changes. If you change the linker to GNU LD then you get an artifact. I haven't tried the GNU LD output on hardware though; I'll try that later today.

@japaric
Copy link
Contributor

japaric commented Feb 18, 2020

AFAICT, LLD does not produce a final artifact with these changes.

Scratch that. I had a rust-lld binary in my .cargo/bin directory and that was eliminating the final artifact somehow. rustc -C linker=/path/to/rustup/toolchain/nightly/rust-lld (..) does produce an artifact.

@MabezDev
Copy link
Contributor

Just to make sure, I built and ran the test case with noth lld & GNU ld and both produced debuggable binaries with the correct backstrace.

@jonas-schievink
Copy link
Contributor Author

Nice!

@japaric
Copy link
Contributor

japaric commented Feb 18, 2020

I can also confirm that, with these changes (and rust-lld), (virtual) unwinding / backtraces work with a custom tool I wrote.

The missing artifact issue that @jonas-schievink and I observed was just a few bugs in other tools (e.g. cargo-binutils). Those have been already reported elsewhere.

This looks good to me and fixes a huge pain point for people that are using backtraces in GDB. Thanks @jonas-schievink and @MabezDev (who I believe first found out about the frame-pointer option in rustc / llvm) for working on this!

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 18, 2020

📌 Commit 27cfb2b has been approved by japaric

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 18, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 19, 2020
Don't eliminate frame pointers on thumb targets

This should hopefully fix issues rust-lang#69231 and rust-embedded/cortex-m-rt#139.

~~I couldn't test this locally as the rustc I produced does not create binaries (no idea why).~~ Resolved.
This was referenced Feb 19, 2020
bors added a commit that referenced this pull request Feb 19, 2020
Rollup of 5 pull requests

Successful merges:

 - #68863 (ci: switch macOS builders to 10.15)
 - #69142 (Add shared script for linkchecking books.)
 - #69248 (Don't eliminate frame pointers on thumb targets)
 - #69280 (Remove special case for `simd_shuffle` arg promotion)
 - #69284 (Reword OpenOptions::{create, create_new} doc.)

Failed merges:

r? @ghost
@bors bors merged commit c6ad1e2 into rust-lang:master Feb 19, 2020
@jonas-schievink jonas-schievink deleted the thumb-fp branch February 19, 2020 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants