[PATCH] D55915: [Driver] Make -fno-omit-frame-pointer imply -mno-omit-leaf-frame-pointer

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:592-595
+  if (Arg *A = Args.getLastArg(options::OPT_fno_omit_frame_pointer,
+                               options::OPT_fomit_frame_pointer))
+    return A->getOption().matches(options::OPT_fno_omit_frame_pointer) ||
+           mustUseFramePointerForTarget(Triple);
----------------
tabloid.adroit wrote:
> chandlerc wrote:
> > tabloid.adroit wrote:
> > > chandlerc wrote:
> > > > This doesn't correctly handle "last-flag-wins". Consider the case of `-mno-omit-leaf-frame-pointer -fomit-frame-pointer`. That should omit the leaf frame pointer, but if I read this correctly the logic here will use a leaf frame pointer.
> > > Updated test with this case along with some other cases.
> > > 
> > > // RUN: %clang -### -S -Os -mno-omit-leaf-frame-pointer -fomit-frame-pointer %s 2>&1 | \
> > > // RUN:   FileCheck --check-prefix=OMIT-ALL5 %s
> > > // OMIT-ALL5-NOT: "-mdisable-fp-elim"
> > > // OMIT-ALL5-NOT: "-momit-leaf-frame-pointer"
> > > 
> > > This falls into lib/CodeGen/CGCall.cpp:1733, which causes TargetOptions::DisableFramePointerElim returns false for all frames.
> > > 
> > > 
> > Then I don't understand what this change is doing.
> > 
> > This function, when called with arguments `-mno-omit-leaf-frame-pointer -fomit-frame-pointer` will not hit the code you've added here, and will instead return `true`. That doesn't seem like a sensible result given the desired change to these flags. If something *else* is causing us to still not use leaf frame pointers, that doesn't make the code here correct, it makes me question how this works at all (and how we are testing it).
> I see your point here. The logic is very confusing indeed.
> 
> It looks better if
> s/shouldUseFramePointer/addFlagDisableFPElim
> s/shouldUseLeafFramePointer/addFlagOmitLeafFramePointer
> 
> to show that here only decides compiler flag instead of the final code.
> 
> I think the correct way to handle these is to replace `-mdisable-fp-elim` and `-momit-leaf-frame-pointer` compiler flags with one, say `frame-pointer-model` = {KeepAll, OmitAll, KeepNonLeaf}, and let the driver decide `frame-pointer-model` here. The downside is that it affects compiler user unless we bridge deprecating flags on to new flag with some rules.
> 
Those flags are in the "CC1" interface -- the internal interface between the driver and the compiler internals. That interface has no stability requirements and isn't something we support users leveraging directly.

So I think we could actually change this to be sensible in much the way you are suggesting. Specifically, I would have `-frame-pointers={all,non-leaf,none}`. And then all of the compatibility and mapping logic in the driver will resolve to a very simple end result.

In code, I think we should follow a similar simplification where we have a single function to determine the total strategy here, rather than the logic being separated into multiple different functions.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55915/new/

https://reviews.llvm.org/D55915