Don't force-enable frame pointers when generating debug info

[dotdash]

We apparently used to generate bad/incomplete debug info causing
debuggers not to find symbols of stack allocated variables. This was
somehow worked around by having frame pointers.

With the current codegen, this seems no longer necessary, so we can
remove the code that force-enables frame pointers whenever debug info
is requested.

Fixes #11906

[rust-highfive]

r? @aturon

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

[dotdash]

Cc @eddyb @michaelwoerister @pcwalton

[michaelwoerister]

Thank you for looking into and fixing this over the last few weeks, @dotdash!

However, it seems that some targets can also disable frame pointer elimination. Can you just adapt the must_not_eliminate_frame_pointers() method instead of removing set_frame_pointer_elimination()?

[dotdash]

@michaelwoerister D'oh! The fixme comment got me thinking that the whole thing was about that issue, should have had a closer look. Thanks! Updated.

[michaelwoerister]

Great, thank you!

@bors r+

[bors]

📌 Commit 0bc466d has been approved by michaelwoerister

[michaelwoerister]

@bors r- (travis error)

[michaelwoerister]

Btw, @dotdash, did you check that this does not break backtraces?

[michaelwoerister]

I guess https://github.com/rust-lang/rust/blob/master/src/test/run-pass/backtrace.rs takes care of that.

[dotdash]

@bors r+

Removed the unused import

[bors]

📌 Commit 002c786 has been approved by dotdash

[michaelwoerister]

@bors r-

Sorry for the churn, @dotdash. @eddyb mentioned on IRC that @pcwalton might have an opinion on this and that we might want to wait for that.

[eddyb]

Talked to @pcwalton on IRC. He was primarily concerned with macOS and iOS, but we already set eliminate_frame_pointer to false in the target specs for it so that wouldn't be affected.

But he also mentioned profiling in general and that frame pointers are still required there, as DWARF isn't apparently enough in many cases. so we should have a way to turn them back on.

@dotdash @michaelwoerister Do you think we can reasonably expose a -C flag for frame pointer elimination? Or can we do it already in some way I can't find right now, perhaps?

[pcwalton]

I think ideally we would have a --profile mode that's like --release but turns on frame pointers. This is what Xcode does in its UI.

[michaelwoerister]

Do you think we can reasonably expose a -C flag for frame pointer elimination?

Sounds reasonable to me.

[bors]

☔ The latest upstream changes (presumably #47235) made this pull request unmergeable. Please resolve the merge conflicts.

[michaelwoerister]

Do you think we can reasonably expose a -C flag for frame pointer elimination?

How do we move forward with this without making the flag insta-stable? It would be a bit lame if you could only profile code that was built with nightly.

[michaelwoerister]

cc @rust-lang/compiler

[nikomatsakis]

I'd be ok with exposing such a flag, even if it were insta-stable, but I also think it's ok to land such a flag unstable, and then do a standard stabilization (perhaps just start it immediately). do we have much precedent for flags of these kind? I confess I don't pay that much attention to -C. =)

[kennytm]

@rust-lang/compiler I'm confused with the current status of this PR 😄 Is it OK to land this or not? Maybe it needs an FCP? 😉

Anyway, @dotdash please fix the merge conflict.

[nikomatsakis]

It seems like an FCP is a good idea.

@rfcbot fcp merge

I propose that we merge this PR, which will add an insta-stable -C flag for controlling frame pointer elimination. It would be good if @dotdash (or @michaelwoerister) can give a summary of the precise flag and its behavior, though.

The reason to make it instastable is basically that we want people to be able to use it on stable so they can do profiling, which is an important use case. It also seems like a pretty easy thing to commit to supporting in some form going forward (and, at worst, it can be deprecated and become a sort of no-op). I would also be ok with adding a -Z flag and doing a stabilization period, though, so that is the primary alternative.

UPDATE: I realize now that the state of the PR is not what I described. My bad. To be clear, what we are voting on is:

1. approving a new insta-stable option -C force-frame-pointer
2. including frame pointers by default when debugging

As @michaelwoerister points out here, we may want some nice cargo tooling around this too.

[rfcbot]

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

• @Zoxc
• @arielb1
• @cramertj
• @eddyb
• @estebank
• @jseyfried
• @michaelwoerister
• @nikomatsakis
• @nrc
• @petrochenkov
• @pnkfelix

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[dotdash]

Checking clang, I noticed that they have a slightly more complex logic regarding frame pointers, using a combination of defaults, flags and forcing frame pointers for certain archs (just ARM Darwin).

I'll update this with a similar approach. I'd also like to use -Comit-frame-pointers=0/1 instead of the one way flag to only enable them. Any complaints about that?

[kennytm]

(For record, the latest error is likely legit.

[01:55:03] failures:
[01:55:03] 
[01:55:03] ---- [debuginfo-lldb] debuginfo/macro-stepping.rs stdout ----
[01:55:03] 	NOTE: compiletest thinks it is using LLDB version 370
[01:55:03] 
[01:55:03] error: line not found in debugger output: [...]#loc5[...]
[01:55:03] status: exit code: 0
[01:55:03] command: "/usr/bin/python" "/Users/travis/build/rust-lang/rust/src/etc/lldb_batchmode.py" "/Users/travis/build/rust-lang/rust/build/i686-apple-darwin/test/debuginfo/macro-stepping.stage2-i686-apple-darwin" "/Users/travis/build/rust-lang/rust/build/i686-apple-darwin/test/debuginfo/macro-stepping.debugger.script"
[01:55:03] stdout:
[01:55:03] ------------------------------------------
[01:55:03] LLDB batch-mode script
[01:55:03] ----------------------
[01:55:03] Debugger commands script is '/Users/travis/build/rust-lang/rust/build/i686-apple-darwin/test/debuginfo/macro-stepping.debugger.script'.
[01:55:03] Target executable is '/Users/travis/build/rust-lang/rust/build/i686-apple-darwin/test/debuginfo/macro-stepping.stage2-i686-apple-darwin'.
[01:55:03] Current working directory is '/Users/travis/build/rust-lang/rust'
[01:55:03] Creating a target for '/Users/travis/build/rust-lang/rust/build/i686-apple-darwin/test/debuginfo/macro-stepping.stage2-i686-apple-darwin'
[01:55:03] settings set auto-confirm true
[01:55:03] 
[01:55:03] version
[01:55:03] lldb-370.0.42 Swift-3.1 
[01:55:03] command script import /Users/travis/build/rust-lang/rust/./src/etc/lldb_rust_formatters.py
[01:55:03] type summary add --no-value --python-function lldb_rust_formatters.print_val -x ".*" --category Rust
[01:55:03] type category enable Rust
[01:55:03] 
[01:55:03] breakpoint set --file 'macro-stepping.rs' --line 111
[01:55:03] Breakpoint 1: where = macro-stepping.stage2-i686-apple-darwin`macro_stepping::main::h3d381aa36e9afe65 at macro-stepping.rs:111, address = 0x00002e20 
[01:55:03] breakpoint set --file 'macro-stepping.rs' --line 126
[01:55:03] Breakpoint 2: where = macro-stepping.stage2-i686-apple-darwin`macro_stepping::main::h3d381aa36e9afe65 + 402 at macro-stepping.rs:126, address = 0x00002fb2 
[01:55:03] set set stop-line-count-before 0
[01:55:03] 
[01:55:03] set set stop-line-count-after 1
[01:55:03] 
[01:55:03] run
[01:55:03] Hit breakpoint 1.1: where = macro-stepping.stage2-i686-apple-darwin`macro_stepping::main::h3d381aa36e9afe65 at macro-stepping.rs:111, address = 0x00002e20, resolved, hit count = 1 
[01:55:03] Process 45431 stopped * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1 frame #0: 0x00002e20 macro-stepping.stage2-i686-apple-darwin`macro_stepping::main::h3d381aa36e9afe65 at macro-stepping.rs:111 -> 111 zzz(); // #break 112 Process 45431 launched: '/Users/travis/build/rust-lang/rust/build/i686-apple-darwin/test/debuginfo/macro-stepping.stage2-i686-apple-darwin' (i386) 
[01:55:03] next
[01:55:03] frame select
[01:55:03] frame #0: 0x00002e50 macro-stepping.stage2-i686-apple-darwin`macro_stepping::main::h3d381aa36e9afe65 at macro-stepping.rs:113 -> 113 foo!(); // #loc1 114 
[01:55:03] next
[01:55:03] frame select
[01:55:03] frame #0: 0x00002e68 macro-stepping.stage2-i686-apple-darwin`macro_stepping::main::h3d381aa36e9afe65 at macro-stepping.rs:115 -> 115 foo2!(); // #loc2 116 
[01:55:03] next
[01:55:03] frame select
[01:55:03] frame #0: 0x00002ea0 macro-stepping.stage2-i686-apple-darwin`macro_stepping::main::h3d381aa36e9afe65 at macro-stepping.rs:117 -> 117 let x = vec![42]; // #loc3 118 
[01:55:03] next
[01:55:03] frame select
[01:55:03] frame #0: 0x00002ee1 macro-stepping.stage2-i686-apple-darwin`macro_stepping::main::h3d381aa36e9afe65 at macro-stepping.rs:117 -> 117 let x = vec![42]; // #loc3 118 
[01:55:03] next
[01:55:03] frame select
[01:55:03] frame #0: 0x00002ee4 macro-stepping.stage2-i686-apple-darwin`macro_stepping::main::h3d381aa36e9afe65 at macro-stepping.rs:119 -> 119 new_scope!(); // #loc4 120 
[01:55:03] continue
[01:55:03] Hit breakpoint 2.1: where = macro-stepping.stage2-i686-apple-darwin`macro_stepping::main::h3d381aa36e9afe65 + 402 at macro-stepping.rs:126, address = 0x00002fb2, resolved, hit count = 1 
[01:55:03] step
[01:55:03] frame select
[01:55:03] frame #0: 0x00002ff0 macro-stepping.stage2-i686-apple-darwin`macro_stepping::included::h329c014ece1df705 at macro-stepping.inc:12 -> 12 foo!(); // #inc-loc1 13 
[01:55:03] next
[01:55:03] frame select
[01:55:03] frame #0: 0x0000300b macro-stepping.stage2-i686-apple-darwin`macro_stepping::included::h329c014ece1df705 at macro-stepping.inc:14 -> 14 foo2!(); // #inc-loc2 15 
[01:55:03] next
[01:55:03] frame select
[01:55:03] frame #0: 0x00003043 macro-stepping.stage2-i686-apple-darwin`macro_stepping::included::h329c014ece1df705 at macro-stepping.inc:16 -> 16 zzz(); // #inc-loc3 17 } 
[01:55:03] quit
[01:55:03] 
[01:55:03] 
[01:55:03] ------------------------------------------
[01:55:03] stderr:
[01:55:03] ------------------------------------------
[01:55:03] warning: (i386) /Users/travis/build/rust-lang/rust/build/i686-apple-darwin/test/debuginfo/macro-stepping.stage2-i686-apple-darwin.lldb.aux/libmacro_stepping.dylib empty dSYM file detected, dSYM was created with an executable with no debug info.
[01:55:03] 
[01:55:03] ------------------------------------------
[01:55:03] 
[01:55:03] thread '[debuginfo-lldb] debuginfo/macro-stepping.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:2883:9
[01:55:03] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:55:03] 
[01:55:03] 
[01:55:03] failures:
[01:55:03]     [debuginfo-lldb] debuginfo/macro-stepping.rs

)

[dotdash]

Oh, yeah, that's why I looked into clang. They never disable frame pointers on Darwin by default. We only force them for Darwin x86_64, but I don't feel comfortable doing that for i686 as well, because with the current setup, we can only force frame pointers, without any chance for the user to disable them. So I'd like to make the target option for frame pointers a mere default, which can be overridden.

[nikomatsakis]

I'd also like to use -Comit-frame-pointers=0/1 instead of the one way flag to only enable them. Any complaints about that?

makes sense, except that we should use our usual convention (which accepts all kinds of things, like yes, no, true, false, right?)

[dotdash]

Right, I had that in mind. I should have written 0/1/... or something like that. 2018-02-12 21:30 GMT+01:00 Niko Matsakis <notifications@github.com>:

…

I'd also like to use -Comit-frame-pointers=0/1 instead of the one way flag to only enable them. Any complaints about that? makes sense, except that we should use our usual convention (which accepts all kinds of things, like yes, no, true, false, right?) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#47152 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOGMvjP51uzCEzTkG4BU24lwVHcHz3Tks5tUJ9mgaJpZM4RRmZR> .

[Zoxc]

I'm not sure why we need to force frame pointers by default on Darwin. What happens if we don't?

I'd also like to use -Comit-frame-pointers=0/1 instead of the one way flag to only enable them. Any complaints about that?

It does not make sense to ask the compiler to omit frame pointers. This should still be called -C force-frame-pointers.

[Mark-Simulacrum]

Why doesn't it make sense? Or, rather, is there any harm to allowing it? -C frame-pointers=[auto|yes|no] would make the most sense to me, with the "auto" being the default on all platforms, currently.

[Zoxc]

LLVM cannot generate code which does not use frame pointers. You can just ask it to always use frame pointers.

[Mark-Simulacrum]

GCC has the -fomit-frame-pointer option, default off on "32-bit GNU/Linux x86 and 32-bit Darwin x86 targets", but otherwise on it seems if optimizing. Clang also at least superficially seems to support it, clang -fomit-frame-pointer does not error. https://bugs.llvm.org/show_bug.cgi?id=9825 implies that you need to pass both -fno-omit-frame-pointer and -mno-omit-leaf-frame-pointer in order to truly not omit any frame pointers. It'd be good to check that we don't need two separate options, and that yes/no arguments are doing what we want them to (not sure, but maybe we need two toggles or roll them into one somehow).

[retep998]

As another reference point, by default cl.exe will always use frame pointers, unless you specify the /Oy flag (or imply it via /Ox /O1 or /O2) which will omit frame pointers when possible (/Oy- disables the omission).

[petrochenkov]

As another reference point, by default cl.exe will always use frame pointers

Probably due to different debug info format. Debugging on MSVC is pretty much impossible without frame pointers (or at least it was a few years ago, on 32-bit).

[dotdash]

I'm not sure why we need to force frame pointers by default on Darwin. What happens if we don't?

The test failure that we saw in the last run for this PR, because the debugger apparently can't properly resolve the stack frames on that target without frame pointers. If it wasn't for clang always defaulting to frame pointers for Darwin, I'd guess that we still emit incomplete/broken debug info, but as it is, I assume that clang is right in what it is doing there.

It does not make sense to ask the compiler to omit frame pointers. This should still be called -C force-frame-pointers.

I'd argue that -Comit-frame-pointers=0/1/... makes more sense. LLVM happens to default to omitting frame pointers, but e.g. clang defaults to emitting them by default unless optimizations are enabled and I'd like rustc to do the same. So I need an option that can do both, tell the compiler to omit frame pointers (overriding the default for non-optimizing builds), as well as asking it not to omit them (overriding the default for optimizing builds). IMHO this fits an omit-frame-pointers option just fine, but a force-frame-pointers flag only allows to go one way. And even as an on/off option, "force-frame-pointers=0" doesn't sound like it would override a default that already emits frame pointers. And finally, the omit-frame-pointers option would be more familiar to folks coming from e.g. C/C++.

[arazabishov]

@dotdash Any updates on this PR? :)

[shepmaster]

Ping from triage, @dotdash ! We haven't heard from you in a few weeks, will you have a chance to address the test failures soon?

[shepmaster]

Oops, I missed that the previous comment was also from triage. That means I'm going to go ahead and close this PR for now to keep things tidy. Please feel free to reopen this PR when you have a chance to work on it again! Thanks for your hard work!

[nikomatsakis]

Argh. I'd really like to see this change land -- I opened #48785 to track it