This is an archive of the discontinued LLVM Phabricator instance.

Do not force the frame pointer by default for ARM EABI
Needs ReviewPublic

Authored by chrib on Apr 12 2017, 12:51 AM.

Details

Summary

Do not force the frame pointer by default for ARM EABI

(bugzilla #32501)

Event Timeline

chrib created this revision.Apr 12 2017, 12:51 AM

Can you add a test case?

chrib updated this revision to Diff 95945.Apr 20 2017, 6:31 AM
  1. Updating D31972: Do not force the frame pointer by default for ARM EABI #

Add test case

chrib updated this revision to Diff 95958.Apr 20 2017, 7:44 AM
  • Fix thinko in test
ahatanak added inline comments.Apr 20 2017, 11:08 AM
test/CodeGen/arm-fp-eabi.c
2

Instead of generating a .s file and looking for ".setfp", you can pass -### and check the presence or absence of -mdisable-fp-elim (see other tests in test/Driver).

chrib updated this revision to Diff 96109.Apr 21 2017, 1:03 AM
  • Check not mdisable-fp-elim for arm eabi when optimizing
chrib added inline comments.Apr 21 2017, 1:07 AM
test/CodeGen/arm-fp-eabi.c
2

yes sure, I extended the existing frame-pointer.c with the test similar to other archs. Thanks

efriedma added a subscriber: efriedma.
efriedma added inline comments.
lib/Driver/ToolChains/Clang.cpp
569

Specifically checking for "llvm::Triple::EABI" is suspicious... what are you trying to distinguish?

chrib added inline comments.Jun 1 2017, 1:00 AM
lib/Driver/ToolChains/Clang.cpp
569

I'm targeting the AAPCS for bare toolsets, (we could also test EABIHF by the way)

I'm not sure about the other ABIs (such as llvm::Triple::OpenBSD) so it is probably conservative and stick to what I can test. Do you think this pertains to more, for instance to AAPCS-LINUX, without breaking anything ?

efriedma added inline comments.Jun 1 2017, 12:10 PM
lib/Driver/ToolChains/Clang.cpp
569

So... something like isTargetAEABI() in ARMSubtarget.h?

Please clarify the comment, and add a check for EABIHF.

chrib updated this revision to Diff 101543.Jun 6 2017, 4:48 AM
  • Merge branch 'master' of ssh://codex.cro.st.com/llvm-arm/clang
  • Don't need a frame pointer for EABIHF also (AAPCS)
chrib added inline comments.Jun 6 2017, 4:57 AM
lib/Driver/ToolChains/Clang.cpp
569

yes, (although I'm not sure for Darwin). The closest check for AAPCS I've found is Clang::AddARMTargetArgs.

I've updated the patch to check EABIHF as well.

efriedma edited edge metadata.Jun 6 2017, 2:36 PM

Please start a thread on cfe-dev about this; most developers don't read cfe-commits, and thinking about it a bit more, I'm not confident omitting frame pointers is really a good default. I would guess there's existing code which depends on frame pointers to come up with a stack trace, since table-based unwinding is complicated and takes up a lot of space.

Grepping over the in-tree tests, it looks like the "eabi" triples used in practice are "arm-non-eabi", "arm-netbsd-eabi" and variants of "thumbv7m-apple-darwin-eabi" (and variants of those). Please make sure you aren't changing the behavior of netbsd and "Darwin" targets.

chrib added a comment.Jun 7 2017, 2:17 AM

OK, I have created a RFE tracker (BZ #32501). I will forward to cfe-dev.

Regarding the need to avoid table-based unwinding, the way to handle those request for an explicit frame pointer when not required by the ABI is to use -fno-omit-frame-pointer flag, catching up with the GCC behavior.

Also I'm not sure that the unwind table space is such an issue (for debugging) since they are not loadable. Other uses such as profiling is not be impacted by the change, and exceptions unwinder likelibgcc or libunwind should work without the frame pointer

I'll amend the patch to check for Darwin and Netbsd.

chrib updated this revision to Diff 101675.Jun 7 2017, 2:23 AM
  • do not omit the frame pointer for netbsd-eabi and darwin-eabi
joerg added a subscriber: joerg.Jun 7 2017, 6:28 AM

Actually, for NetBSD we want to use -fomit-frame-pointer by default whenever the implicit -funwind-tables is also present. In general, that should be the justification for the default behavior: "Can I reliably unwind a binary with the available information?" Performance of stack unwinding is IMO secondary and users of sanitizers can disable it where necessary.