Do not force the frame pointer by default for ARM EABI

[chrib]

Summary

Do not force the frame pointer by default for ARM EABI

(bugzilla #32501)

[ahatanak]

Can you add a test case?

[chrib]

1. Updating D31972: Do not force the frame pointer by default for ARM EABI #

Add test case

[chrib]

• Fix thinko in test

[ahatanak]

test/CodeGen/arm-fp-eabi.c

1 ↗

(On Diff #95958)

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]

• Check not mdisable-fp-elim for arm eabi when optimizing

[chrib]

test/CodeGen/arm-fp-eabi.c

1 ↗

(On Diff #95958)

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

[efriedma]

lib/Driver/ToolChains/Clang.cpp

569

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

[chrib]

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]

lib/Driver/ToolChains/Clang.cpp

569

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

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

[chrib]

• Merge branch 'master' of ssh://codex.cro.st.com/llvm-arm/clang
• Don't need a frame pointer for EABIHF also (AAPCS)

[chrib]

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]

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]

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]

• do not omit the frame pointer for netbsd-eabi and darwin-eabi

[joerg]

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.

lib/Driver/ToolChains/Clang.cpp

case llvm::Triple::thumb:
       return true;
     default:
       // All other supported Windows ISAs use xdata unwind information, so frame
       // pointers are not generally useful.
       return false;
     }
   }
 
+  if (!Triple.isOSDarwin() && !Triple.isOSNetBSD() &&
+      (Triple.getEnvironment() == llvm::Triple::EABI ||
+       Triple.getEnvironment() == llvm::Triple::EABIHF)) {
+    // Don't use a frame pointer on AAPCS when optimizing.
+    switch (Triple.getArch()) {
+    case llvm::Triple::arm:
+    case llvm::Triple::thumb:
+      return !areOptimizationsEnabled(Args);
+    default:
+      return true;
+    }
+  }
+
   return true;
 }
 
 static bool shouldUseFramePointer(const ArgList &Args,
                                   const llvm::Triple &Triple) {
   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) ||

test/Driver/frame-pointer.c

 // RUN: %clang -target mips-linux-gnu -### -S -O1 %s -o %t.s 2>&1 | FileCheck -check-prefix=CHECK1-32 %s
 // RUN: %clang -target mipsel-linux-gnu -### -S -O0 %s -o %t.s 2>&1 | FileCheck -check-prefix=CHECK0-32 %s
 // RUN: %clang -target mipsel-linux-gnu -### -S -O1 %s -o %t.s 2>&1 | FileCheck -check-prefix=CHECK1-32 %s
 // RUN: %clang -target mips64-linux-gnu -### -S -O0 %s -o %t.s 2>&1 | FileCheck -check-prefix=CHECK0-32 %s
 // RUN: %clang -target mips64-linux-gnu -### -S -O1 %s -o %t.s 2>&1 | FileCheck -check-prefix=CHECK1-32 %s
 // RUN: %clang -target mips64el-linux-gnu -### -S -O0 %s -o %t.s 2>&1 | FileCheck -check-prefix=CHECK0-32 %s
 // RUN: %clang -target mips64el-linux-gnu -### -S -O1 %s -o %t.s 2>&1 | FileCheck -check-prefix=CHECK1-32 %s
 
+// RUN: %clang -target arm-none-eabi -### -S -O0 %s -o %t.s 2>&1 | FileCheck -check-prefix=CHECK0-32 %s
+// RUN: %clang -target arm-none-eabi -### -S -O1 %s -o %t.s 2>&1 | FileCheck -check-prefix=CHECK1-32 %s
+
 // CHECK0-32: -mdisable-fp-elim
 // CHECK1-32-NOT: -mdisable-fp-elim
 // CHECK2-32-NOT: -mdisable-fp-elim
 // CHECK3-32-NOT: -mdisable-fp-elim
 // CHECKs-32-NOT: -mdisable-fp-elim
 
 // CHECK0-64: -mdisable-fp-elim
 // CHECK1-64-NOT: -mdisable-fp-elim
 // CHECK2-64-NOT: -mdisable-fp-elim
 // CHECK3-64-NOT: -mdisable-fp-elim
 // CHECKs-64-NOT: -mdisable-fp-elim
 // CHECK-MACHO-64: -mdisable-fp-elim