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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
