This is an archive of the discontinued LLVM Phabricator instance.

[Clang][A32/T32][Linux] -O1 implies -fomit-frame-pointer
ClosedPublic

Authored by nickdesaulniers on May 29 2020, 11:50 AM.

Details

Summary

An upgrade of LLVM for CrOS [0] containing [1] triggered a bunch of
errors related to writing to reserved registers for a Linux kernel's
arm64 compat vdso (which is a aarch32 image).

After a discussion on LKML [2], it was determined that
-f{no-}omit-frame-pointer was not being specified. Comparing GCC and
Clang [3], it becomes apparent that GCC defaults to omitting the frame
pointer implicitly when optimizations are enabled, and Clang does not.
ie. setting -O1 (or above) implies -fomit-frame-pointer. Clang was
defaulting to -fno-omit-frame-pointer implicitly unless -fomit-frame-pointer
was set explicitly.

Why this becomes a problem is that the Linux kernel's arm64 compat vdso
contains code that uses r7. r7 is used sometimes for the frame pointer
(for example, when targeting thumb (-mthumb)). See useR7AsFramePointer()
in llvm/llvm-project/llvm/lib/Target/ARM/ARMSubtarget.h. This is mostly
for legacy/compatibility reasons, and the 2019 Q4 revision of the ARM
AAPCS looks to standardize r11 as the frame pointer for aarch32, though
this is not yet implemented in LLVM.

Users that are reliant on the implicit value if unspecified when
optimizations are enabled should explicitly choose -fomit-frame-pointer
(new behavior) or -fno-omit-frame-pointer (old behavior).

[0] https://bugs.chromium.org/p/chromium/issues/detail?id=1084372
[1] https://reviews.llvm.org/D76848
[2] https://lore.kernel.org/lkml/20200526173117.155339-1-ndesaulniers@google.com/
[3] https://godbolt.org/z/0oY39t

Diff Detail

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 29 2020, 11:50 AM
srhines added inline comments.May 29 2020, 4:22 PM
clang/lib/Driver/ToolChains/Clang.cpp
561

For the new targets, we should only be changing the default for non-Android cases. Android targets should still default to -fno-omit-frame-pointer. This makes the logic here quite a bit more complex.

clang/lib/Driver/ToolChains/Clang.cpp
561

It shouldn't be too bad, just need these four cases to check Triple.getEnvironment() != llvm::Triple::Android before calling areOptimizationsEnabled. Will double up the unit tests though, which is fine.

psmith added a comment.Jun 1 2020, 2:29 AM

FWIW I'm happy for Clang to match GCC behaviour for Linux (non-Android) targets with respect to the frame pointer. Outside of Android I would expect most developers of linux applications to build with both GCC and clang so they should already have the -fno-omit-frame-pointer if they are using it.

nickdesaulniers marked 2 inline comments as done.Jun 1 2020, 10:43 AM
nickdesaulniers added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
22

Note to reviewers: the linter really wanted to touch this header inclusion list, since I add "llvm/Support/Compiler.h" below. Hopefully it's ok to just include with this patch?

srhines added inline comments.Jun 1 2020, 11:07 AM
clang/lib/Driver/ToolChains/Clang.cpp
565

Triple.isAndroid() is more clear.

llvm/docs/ReleaseNotes.rst
94

"non-Android Linux" is probably going to make things easier to understand here.

  • prefer Triple#isAndroid
  • update release note
nickdesaulniers marked an inline comment as done.Jun 1 2020, 11:44 AM
nickdesaulniers marked an inline comment as done.
srhines accepted this revision.Jun 1 2020, 12:10 PM

Thanks, Nick, for cleaning this up and always striving to make things more compatible.

This revision is now accepted and ready to land.Jun 1 2020, 12:10 PM
MaskRay added inline comments.
clang/test/Driver/frame-pointer-elim.c
125

-O1 might be better. Users can infer that -O2 omits frame pointers as well since -O1 omits them.

llvm/docs/ReleaseNotes.rst
97

Double backquotes.

  • double backticks in release notes, -O1 rather than -O2 in tests
nickdesaulniers marked 2 inline comments as done.Jun 1 2020, 12:22 PM
nickdesaulniers retitled this revision from [Clang][A32/T32][Linux] -O2 implies -fomit-frame-pointer to [Clang][A32/T32][Linux] -O1 implies -fomit-frame-pointer.Jun 1 2020, 12:26 PM
MaskRay accepted this revision.Jun 1 2020, 12:39 PM

(You can change [Clang] to [Driver] as [Clang] may carry less information. [Driver] emphasizes this is related to clangDriver. Nothing in sema/codegen/analyzer/etc is affected.)

nickdesaulniers added a comment.EditedJun 1 2020, 1:14 PM

(You can change [Clang] to [Driver] as [Clang] may carry less information. [Driver] emphasizes this is related to clangDriver. Nothing in sema/codegen/analyzer/etc is affected.)

Isn't that ambiguous which driver is being modified? For instance, lldb and lld's drivers aren't being modified.

Harbormaster completed remote builds in B58649: Diff 267683.
danalbert accepted this revision.Jun 1 2020, 3:02 PM

May I please have a non-Googler to review+(accept|reject) the revision?

MaskRay edited reviewers, added: ostannard; removed: olista01.Jun 2 2020, 11:54 AM
MaskRay added a subscriber: olista01.

May I please have a non-Googler to review+(accept|reject) the revision?

I guess @olista01 is an inactive account. Changed to @ostannard

psmith accepted this revision.Jun 2 2020, 1:51 PM

LGTM from an Arm person now that the Android changes have been made.

clang/lib/Driver/ToolChains/Clang.cpp
22

One possible solution here is to commit the reordering separately using an [NFC] refactoring commit, no separate review required, then commit the rest of the patch after that. Anyone doing a source code search will only pick up significant changes.

efriedma accepted this revision.Jun 2 2020, 1:56 PM
efriedma added a subscriber: efriedma.

LGTM. For non-Android, I think it makes sense to align with gcc as much as possible.

This is mostly
for legacy/compatibility reasons, and the 2019 Q4 revision of the ARM
AAPCS looks to standardize r11 as the frame pointer for aarch32, though
this is not yet implemented in LLVM.

It's also really expensive to use r11 as a frame pointer in Thumb1. I guess it's not impossible, though.

This revision was automatically updated to reflect the committed changes.