Add force-frame-pointer flag to allow control of frame pointer ommision #48786
Conversation
rust-highfive
added
the
S-waiting-on-review
src/librustc/session/mod.rs
Outdated
| @@ -598,8 +598,12 @@ impl Session { | |||
| } | |||
|
|
|||
| pub fn must_not_eliminate_frame_pointers(&self) -> bool { | |||
| self.opts.debuginfo != DebugInfoLevel::NoDebugInfo || | |||
| !self.target.target.options.eliminate_frame_pointer | |||
| if let Some(x) = self.opts.cg.force_frame_pointers { | |||
There was a problem hiding this comment.
To me, the name "force-frame-pointers" makes it sound like a "no" value would revert to default behavior -- but I guess that's not quite right. (That is, if you give -Cforce-frame-pointer=no and you have debuginfo, it will still strip your frame pointers, right? I think I would expect it not to based on the name.)
There was a problem hiding this comment.
-Cforce-frame-pointer=no to me sounds like we're asking the compiler to not force any frame pointers. This lets the compiler use frame pointers any way it pleases.
The other proposed names for this flag sounds like they are omitting or stripping frame pointers. LLVM does not support this. You can only ask it to force frame pointers or let it decide how frame pointers are used.
There was a problem hiding this comment.
This lets the compiler use frame pointers any way it pleases.
I agree, it gives us freedom. I guess I'm saying is that what I would expect -C force-frame-pointers=no and not saying anything to be equivalent.
There was a problem hiding this comment.
To be clear, this is basically a bikeshed =) but it argues for @Mark-Simulacrum's "yes/no/auto" formulation.
There was a problem hiding this comment.
@nikomatsakis ah I see your point, however, it is something that could apply for all optionally-bool arguments, with auto being the same as if the argument was never specified anywhere in the CLI.
There was a problem hiding this comment.
@tromey @michaelwoerister do you have opinions on the naming of the option?
There was a problem hiding this comment.
As far as I understand it, you cannot completely opt out of frame pointer emission, you can only opt into always generating them, so a simple boolean flag -C always-emit-frame-pointers would suffice, I think. The name is a bit verbose though.
There was a problem hiding this comment.
I find @michaelwoerister's logic compelling, but I'm not sure it's complete. C force-frame-pointers is pretty clear -- if you provide it, it forces frame pointers, otherwise it falls back to our default behavior. I guess the question is whether we want the ability to allow frame-pointers to be optimized also in cases where we would by default include them (e.g., with debuginfo, or on mac, or whatever).
I also find @Mark-Simulacrum's formulation compelling, but it sounds a bit like work. =) I guess we would say that -Cframe-pointers=off errors when you are not in a debug build?
OTOH, if we do go with -Cforce-frame-pointers as a simple boolean, we can clearly expand later and just keep it as a deprecated alias. It seems to commit us to very little. =)
There was a problem hiding this comment.
Okay, so…
I’d like to direct you all to read the commit message:
This reworks the force-frame-pointer PR to explicitly only consider the
value of the flag if it is provided and also keeps the debug-info =
force-frame-pointer behaviour from before.Something that was tried but not kept was renaming the flag to
frame-pointer, because for flagframe-pointer=no, it is not
guaranteed, that no frame pointers will be generated. Rather, all it does
is inhibit the removal of the frame pointer.
This explains well why I didn’t choose frame-pointers wording (EDIT: though seems I have botched it at the end in terms of explanation, it should’ve read as "all it does is allow LLVM to remove the frame pointer whenever it wants to do so")
-C always emit-frame-pointers seems like a synonym to -C force-frame-pointers to me.
I read -C force-frame-pointers = no as "Don’t force frame pointers”, rather than anything else, which aligns well with it simply allowing LLVM to omit the frame pointer anywhere it wants, but not guaranteeing that.
I also didn’t add auto at this moment, because it doesn’t seem to imply "default" to me, but I could also be persuaded otherwise. Anyway it is something that can be added later, and since it is more generally useful, considered for all the flags at once.
There was a problem hiding this comment.
I confess I didn't read the commit message. 😛
I read
-C force-frame-pointers = noas "Don’t force frame pointers”, rather than anything else
Agreed!
which aligns well with it simply allowing LLVM to omit the frame pointer anywhere it wants, but not guaranteeing that.
Agreed, but it's not the whole story. I think the part that seems a bit surprising is that not specifying the option at all will also 'not force frame pointers', but under different conditions. That is, the behavior of this PR as I understand it:
-C force-frame-pointers=yes-- you get frame pointers-C force-frame-pointers=no-- you get frame pointers in debug builds, but in opt builds LLVM might optimize frame pointers out- no option -- you get frame pointers in debug builds, or with debuginfo, or on some platforms, but otherwise LLVM might optimize frame pointers out
I'd be happier if that second and third choice were the same -- I'm not sure which of the two is better. Is there a good reason that they are not the same? e.g., how bad would it be if -C force-frame-pointers=no meant the same thing as 'no option' above?
|
Well, we can always add an |
src/librustc/session/mod.rs
Outdated
| x | ||
| } else { | ||
| !self.target.target.options.eliminate_frame_pointer || | ||
| self.opts.debuginfo != DebugInfoLevel::NoDebugInfo |
There was a problem hiding this comment.
Isn't the point of this PR to not tie frame pointer emission to the debuginfo setting?
There was a problem hiding this comment.
This keeps frame pointers when debug-info is enabled by default as suggested by the linked issue:
There was a problem hiding this comment.
Most of the comments in #11906 ask that -g not affect debuginfo, e.g.: #11906 (comment), #11906 (comment), #11906 (comment).
#48785 disagrees, though maybe leaves the door open to have it be a target setting.
I tend to think it's best if the presence or absence of debuginfo does not affect code generation.
There was a problem hiding this comment.
Why did we link the two anyway, intitially? Was it something in the way we made debuginfo which is no longer relevant?
There was a problem hiding this comment.
Why did we link the two anyway, intitially? Was it something in the way we made debuginfo which is no longer relevant?
Yes, @dotdash fixed things in our LLVM IR generation so that now debuggers can handle function arguments without frame pointers being present. Before, debuggers had problems displaying variables under certain conditions.
|
☔ The latest upstream changes (presumably #48849) made this pull request unmergeable. Please resolve the merge conflicts. |
nikomatsakis
added
I-nominated
T-compiler
|
Nominating for discussion in @rust-lang/compiler meeting. I'd like to get this settled and landed! |
|
As a compromise, maybe we should just land this in some form but make it a |
|
To provide some context:
I think we should really decouple the two which would finally also fix godbolt.org: https://stackoverflow.com/a/45586160 |
|
OK, I agree with @michaelwoerister. At this point, I'm also past caring about the name of this damn option, I just want to make a decision. =) Regarding debug pointers, we're basically talking about the behavior of this function: pub fn must_not_eliminate_frame_pointers(&self) -> bool {
if let Some(x) = self.opts.cg.force_frame_pointers {
x
} else {
!self.target.target.options.eliminate_frame_pointer ||
self.opts.debuginfo != DebugInfoLevel::NoDebugInfo
}
}To decouple from debuginfo (huzzah!) we would do this: pub fn must_not_eliminate_frame_pointers(&self) -> bool {
if let Some(x) = self.opts.cg.force_frame_pointers {
x
} else {
!self.target.target.options.eliminate_frame_pointer
}
}This then means that
Not passing the option at all uses a default value (either yes or no) based on the target settings. We could eventually add a I can live with this. @nagisa -- care to make that change to remove the debuginfo dependency? |
nikomatsakis
added
S-waiting-on-author
|
Sure. Will do tonight.
…On Mar 14, 2018 21:09, "Niko Matsakis" ***@***.***> wrote:
OK, I agree with @michaelwoerister <https://github.com/michaelwoerister>.
At this point, I'm also past caring about the name of this damn option, I
just want to make a decision. =)
Regarding debug pointers, we're basically talking about the behavior of
this function:
pub fn must_not_eliminate_frame_pointers(&self) -> bool {
if let Some(x) = self.opts.cg.force_frame_pointers {
x
} else {
!self.target.target.options.eliminate_frame_pointer ||
self.opts.debuginfo != DebugInfoLevel::NoDebugInfo
}
}
To decouple from debuginfo (huzzah!) we would do this:
pub fn must_not_eliminate_frame_pointers(&self) -> bool {
if let Some(x) = self.opts.cg.force_frame_pointers {
x
} else {
!self.target.target.options.eliminate_frame_pointer
}
}
This then means that -Cforce-frame-pointers=yes means, well,
force-frame-pointers.
-Cforce-frame-pointers=no overrides the target, which means that frame
pointers may be removed, depending on the optimization settings.
Not passing the option at all uses a default value (either yes or no)
based on the target settings. We could eventually add a auto or target
keyword to explicitly request this value.
I can live with this. @nagisa <https://github.com/nagisa> -- care to make
that change to remove the debuginfo dependency?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#48786 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AApc0gDDR7n1tu0vsD0zlrym8GZClx9Vks5teWregaJpZM4SfNz->
.
|
nagisa
force-pushed
the
fp
branch
2 times, most recently
from
1ad2a8b
to
7188f41
Compare
|
@bors r+ |
1 similar comment
|
@bors r+ |
|
📌 Commit 7188f41 has been approved by |
bors
added
S-waiting-on-bors
Add force-frame-pointer flag to allow control of frame pointer ommision Rebase of rust-lang#47152 plus some changes suggested by rust-lang#48785. Fixes rust-lang#11906 r? @nikomatsakis
|
@bors try- |
|
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
nagisa
force-pushed
the
fp
branch
2 times, most recently
from
ec5706d
to
b8d764c
Compare
|
@bors try |
|
⌛ Trying commit b8d764c761830c74cf60bf3c871547f2ae1b52a1 with merge eee9e36c6c7e8b60fdfda53231b10817a6e90310... |
|
Ah, I had hoped that try build would build something else than Linux :( |
|
@nagisa You need to modify |
|
Oh no, I just had hoped to run the tests on OS X and Windows to make sure they pass. |
|
@nagisa You just need to comment out the diff --git a/.travis.yml b/.travis.yml
index 63831cd596..830eb9aa4d 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -57,7 +57,7 @@ matrix:
CI_JOB_NAME=x86_64-apple
os: osx
osx_image: xcode9.3-moar
- if: branch = auto
+ # if: branch = auto
- env: >
RUST_CHECK_TARGET=checkNo check for Windows however. We can't use AppVeyor for PR CI and try builds. |
|
☀️ Test successful - status-travis |
|
Ping from triage! What's the status on this? |
|
Seems like @nagisa still wants to try the changes on CI? |
|
The only reason I wanted to run tests is to get any information about whether the changes make test suite happy or not. It is perfectly fine to r+ the PR. In fact, it also doesn’t require me to do changes to CI configuration files to check stuff out. |
|
@nagisa ok -- well, needs rebase, but r=me |
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. Since certain situations, like profiling code profit from having frame pointers, we add a -Cforce-frame-pointers flag to always enable frame pointers. Fixes rust-lang#11906
This reworks the force-frame-pointer PR to explicitly only consider the value of the flag if it is provided, and use a target default otherwise. Something that was tried but not kept was renaming the flag to `frame-pointer`, because for flag `frame-pointer=no`, there is no guarante, that LLVM will elide *all* the frame pointers; oposite of what the literal reading of the flag would suggest.
|
@bors r=nikomatsakis Lets see if this works. |
|
📌 Commit 7ec0452 has been approved by |
bors
added
S-waiting-on-bors
Add force-frame-pointer flag to allow control of frame pointer ommision Rebase of #47152 plus some changes suggested by #48785. Fixes #11906 r? @nikomatsakis
|
☀️ Test successful - status-appveyor, status-travis |
Rebase of #47152 plus some changes suggested by #48785.
Fixes #11906
r? @nikomatsakis