Enable frame pointer for non-leaf functions on Android #97614
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-driver Author: None (yabinc) ChangesOn Android, we always want frame pointers to make debugging in the field easier. Since frame pointers are already enabled for AArch64, ARM and RISCV64, effectively this change further enables frame pointers for X86 and X86_64. Full diff: https://github.com/llvm/llvm-project/pull/97614.diff 2 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 2a4c1369f5a73..b631da002209c 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -166,7 +166,7 @@ static bool useFramePointerForTargetByDefault(const llvm::opt::ArgList &Args,
static bool useLeafFramePointerForTargetByDefault(const llvm::Triple &Triple) {
if (Triple.isAArch64() || Triple.isPS() || Triple.isVE() ||
- (Triple.isAndroid() && Triple.isRISCV64()))
+ (Triple.isAndroid() && !Triple.isARM()))
return false;
return true;
diff --git a/clang/test/Driver/frame-pointer.c b/clang/test/Driver/frame-pointer.c
index 2b4287bf447ca..2015fa520c2a2 100644
--- a/clang/test/Driver/frame-pointer.c
+++ b/clang/test/Driver/frame-pointer.c
@@ -4,6 +4,9 @@
// RUN: %clang --target=i386-pc-linux -### -S -O3 %s 2>&1 | FileCheck -check-prefix=CHECK3-32 %s
// RUN: %clang --target=i386-pc-linux -### -S -Os %s 2>&1 | FileCheck -check-prefix=CHECKs-32 %s
+// RUN: %clang --target=i386-linux-android -### -S -O0 %s 2>&1 | FileCheck -check-prefix=CHECK-ANDROID %s
+// RUN: %clang --target=i386-linux-android -### -S -O1 %s 2>&1 | FileCheck -check-prefix=CHECK-ANDROID %s
+// RUN: %clang --target=i386-linux-android -### -S -Os %s 2>&1 | FileCheck -check-prefix=CHECK-ANDROID %s
// RUN: %clang --target=x86_64-pc-linux -### -S -O0 %s 2>&1 | FileCheck -check-prefix=CHECK0-64 %s
// RUN: %clang --target=x86_64-pc-linux -### -S -O1 %s 2>&1 | FileCheck -check-prefix=CHECK1-64 %s
@@ -12,6 +15,10 @@
// RUN: %clang --target=x86_64-pc-linux -### -S -Os %s 2>&1 | FileCheck -check-prefix=CHECKs-64 %s
// RUN: %clang --target=x86_64-pc-win32-macho -### -S -O3 %s 2>&1 | FileCheck -check-prefix=CHECK-MACHO-64 %s
+// RUN: %clang --target=x86_64-linux-android -### -S -O0 %s 2>&1 | FileCheck -check-prefix=CHECK-ANDROID %s
+// RUN: %clang --target=x86_64-linux-android -### -S -O1 %s 2>&1 | FileCheck -check-prefix=CHECK-ANDROID %s
+// RUN: %clang --target=x86_64-linux-android -### -S -Os %s 2>&1 | FileCheck -check-prefix=CHECK-ANDROID %s
+
// Trust the above to get the optimizations right, and just test other targets
// that want this by default.
// RUN: %clang --target=s390x-pc-linux -### -S -O0 %s 2>&1 | FileCheck -check-prefix=CHECK0-64 %s
@@ -57,9 +64,9 @@
// RUN: %clang --target=riscv64-unknown-linux-gnu -### -S -O3 %s 2>&1 | FileCheck -check-prefix=CHECK3-64 %s
// RUN: %clang --target=riscv64-unknown-linux-gnu -### -S -Os %s 2>&1 | FileCheck -check-prefix=CHECKs-64 %s
-// RUN: %clang --target=riscv64-linux-android -### -S -O0 %s 2>&1 | FileCheck -check-prefix=CHECK-ANDROID-64 %s
-// RUN: %clang --target=riscv64-linux-android -### -S -O1 %s 2>&1 | FileCheck -check-prefix=CHECK-ANDROID-64 %s
-// RUN: %clang --target=riscv64-linux-android -### -S -Os %s 2>&1 | FileCheck -check-prefix=CHECK-ANDROID-64 %s
+// RUN: %clang --target=riscv64-linux-android -### -S -O0 %s 2>&1 | FileCheck -check-prefix=CHECK-ANDROID %s
+// RUN: %clang --target=riscv64-linux-android -### -S -O1 %s 2>&1 | FileCheck -check-prefix=CHECK-ANDROID %s
+// RUN: %clang --target=riscv64-linux-android -### -S -Os %s 2>&1 | FileCheck -check-prefix=CHECK-ANDROID %s
// RUN: %clang --target=loongarch32 -### -S -O0 %s -o %t.s 2>&1 | FileCheck -check-prefix=CHECK0-32 %s
// RUN: %clang --target=loongarch32 -### -S -O1 %s -o %t.s 2>&1 | FileCheck -check-prefix=CHECK1-32 %s
@@ -86,4 +93,4 @@
// CHECKs-64-NOT: -mframe-pointer=all
// CHECK-MACHO-64: -mframe-pointer=all
-// CHECK-ANDROID-64: -mframe-pointer=non-leaf
+// CHECK-ANDROID: -mframe-pointer=non-leaf
|
yabinc
requested review from
hiraditya,
stephenhines,
pirama-arumuga-nainar and
enh-google
enh-google
approved these changes
Jul 3, 2024
yabinc
force-pushed
the
enable-frame-pointer-for-non-leaf-functiosn-on-android
branch
from
0dfd99e to
d66a8a2
Compare
|
I forgot part of the change. Now it is fixed. |
|
Currently, we have frame pointer only for non-leaf functions for AArch64, RISCV64, and frame pointer for all functions for ARM. I feel frame pointer for leaf functions isn't needed to unwind a callstack (For the leaf function, we already have the IP register pointing to the leaf function). So I choose to have frame pointer only for non-leaf functions for X86, X86_64. But keep ARM behavior unchanged. |
enh-google
approved these changes
Jul 3, 2024
stephenhines
approved these changes
Jul 3, 2024
On Android, we always want frame pointers to make debugging in the field easier. Since frame pointers are already enabled for AArch64, ARM and RISCV64, effectively this change further enables frame pointers for X86 and X86_64.
yabinc
force-pushed
the
enable-frame-pointer-for-non-leaf-functiosn-on-android
branch
from
d66a8a2 to
3a705f4
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/66/builds/1173 Here is the relevant piece of the build log for the reference: |
lenary
reviewed
Aug 23, 2024
| @@ -166,7 +155,7 @@ static bool useFramePointerForTargetByDefault(const llvm::opt::ArgList &Args, | |||
|
|
|||
| static bool useLeafFramePointerForTargetByDefault(const llvm::Triple &Triple) { | |||
| if (Triple.isAArch64() || Triple.isPS() || Triple.isVE() || | |||
| (Triple.isAndroid() && Triple.isRISCV64())) | |||
| (Triple.isAndroid() && !Triple.isARM())) | |||
There was a problem hiding this comment.
Did you mean to miss out Thumb? isARM is only true for arm-*, armeb-* triples, you would need an isThumb call to also cover thumb-* and thumbeb-* triples.
There was a problem hiding this comment.
I didn't intentionally remove frame pointer for leaf functions for thumb in Android. But I also don't have reason to add it back:
- On Android, we can't rely on stack frame pointer based unwinding for arm32 binaries. Because arm and thumb use different registers as frame pointers.
- On other architectures that we want to support stack frame pointer based unwinding (aarch64, riscv64, x86_64), we don't need frame pointer for leaf functions.
There was a problem hiding this comment.
In https://godbolt.org/z/Krc3eq46h, I tried generating thumb instruction with and without -mno-omit-leaf-frame-pointer. They generate the same instructions not setting up frame pointers for leaf functions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On Android, we always want frame pointers to make debugging in the field easier. Since frame pointers are already enabled for AArch64, ARM and RISCV64, effectively this change further enables frame pointers for X86 and X86_64.