Enable frame pointer for all non-leaf functions on riscv64 Android

[hiraditya]

Summary

Bringing parity with aarch64-android https://github.com/google/android-riscv64/issues/9#issuecomment-1535454205

[enh]

clang/lib/Driver/ToolChains/Clang.cpp

423

should this...

450

(ah, this is why you need Android early. but, yeah, probably worth moving all the Android stuff together, like with the other OSes?)

470

...be down here instead?

475–476

(take care of all this android/arm stuff where you take care of android/riscv64? arm64 has frame pointers by "default default", so we don't need to mention it?)

[hiraditya]

Addressed comments.

[enh]

clang/lib/Driver/ToolChains/Clang.cpp

424

(where? is it simpler to just add arm64 to the switch?)

530

how does this work for Android/arm64?

[hiraditya]

clang/lib/Driver/ToolChains/Clang.cpp

424

yeah we can add it for better readability but it would be redundant.

530

becaues of Triple.isAArch64(), AArch64 always has non-leaf frame pointers for all platforms.

[hiraditya]

clang/lib/Driver/ToolChains/Clang.cpp

424

seems like we don't even have a test for aarch64.*android target. I'll add a testcase.

[craig.topper]

Is there more context on why Android enables the frame pointer?

[craig.topper]

clang/lib/Driver/ToolChains/Clang.cpp

530

You can use Triple.isRISCV64()

[enh]

(lgtm with craig.topper's suggested simplification.)

clang/lib/Driver/ToolChains/Clang.cpp

530

lol. i could seem so much smarter if only i'd learn to read :-)

[MaskRay]

clang/lib/Driver/ToolChains/Clang.cpp

530

&& has a lower precedence than ==. == doesn't need parens.

clang/test/Driver/frame-pointer-elim.c

109

Use --target= for new tests. -target has been deprecated since clang 3.x, albeit without a warning.

clang/test/Driver/frame-pointer.c

61

I don't think we need so many RUN lines. -O0 and -O1 suffice.

[hiraditya]

Addressed comments.

[hiraditya]

Is there more context on why Android enables the frame pointer?

From what i gathered, this is more of an effort to have parity such that existing build flag overrides continue to be consistent.

[enh]

In D150490#4343128, @hiraditya wrote:

Is there more context on why Android enables the frame pointer?

From what i gathered, this is more of an effort to have parity such that existing build flag overrides continue to be consistent.

well, when i said that on the internal chat, i thought you were asking "why do we say what clang already says?" :-)

if the question was actually "is there more context on why Android enables the frame pointer?" i'd have said something like "because Android developers [OS and app developers alike] do so much debugging from the field, where all we get is a crash report for something we probably can't repro locally, that having good _and_ cheap unwinds is super important to us".

[craig.topper]

In D150490#4343145, @enh wrote:

In D150490#4343128, @hiraditya wrote:

Is there more context on why Android enables the frame pointer?

From what i gathered, this is more of an effort to have parity such that existing build flag overrides continue to be consistent.

well, when i said that on the internal chat, i thought you were asking "why do we say what clang already says?" :-)

if the question was actually "is there more context on why Android enables the frame pointer?" i'd have said something like "because Android developers [OS and app developers alike] do so much debugging from the field, where all we get is a crash report for something we probably can't repro locally, that having good _and_ cheap unwinds is super important to us".

Thanks. I suspected that was the answer, but I wanted to check that it made sense to copy AArch64.

[MaskRay]

In D150490#4343442, @craig.topper wrote:

In D150490#4343145, @enh wrote:

In D150490#4343128, @hiraditya wrote:

Is there more context on why Android enables the frame pointer?

From what i gathered, this is more of an effort to have parity such that existing build flag overrides continue to be consistent.

well, when i said that on the internal chat, i thought you were asking "why do we say what clang already says?" :-)

if the question was actually "is there more context on why Android enables the frame pointer?" i'd have said something like "because Android developers [OS and app developers alike] do so much debugging from the field, where all we get is a crash report for something we probably can't repro locally, that having good _and_ cheap unwinds is super important to us".

Thanks. I suspected that was the answer, but I wanted to check that it made sense to copy AArch64.

Enabling the frame pointer is fine. We probably should revert ancient conventions by using -fasynchronous-unwind-tables...

clang/lib/Driver/ToolChains/Clang.cpp

static bool mustUseNonLeafFramePointerForTarget(const llvm::Triple &Triple) {
   }
 }
 
 static bool useFramePointerForTargetByDefault(const ArgList &Args,
                                               const llvm::Triple &Triple) {
   if (Args.hasArg(options::OPT_pg) && !Args.hasArg(options::OPT_mfentry))
     return true;
 
+  if (Triple.isAndroid()) {
+    switch (Triple.getArch()) {
+    case llvm::Triple::aarch64:
+    case llvm::Triple::arm:
+    case llvm::Triple::armeb:
+    case llvm::Triple::thumb:
+    case llvm::Triple::thumbeb:
+    case llvm::Triple::riscv64:
+      return true;
+    default:
+      break;
+    }
+  }
+
   switch (Triple.getArch()) {
   case llvm::Triple::xcore:
   case llvm::Triple::wasm32:
   case llvm::Triple::wasm64:
   case llvm::Triple::msp430:
     // XCore never wants frame pointers, regardless of OS.
     // WebAssembly never wants frame pointers.
     return false;
   case llvm::Triple::ppc:
   case llvm::Triple::ppcle:
   case llvm::Triple::ppc64:
   case llvm::Triple::ppc64le:
   case llvm::Triple::riscv32:
   case llvm::Triple::riscv64:
   case llvm::Triple::sparc:
   case llvm::Triple::sparcel:
   case llvm::Triple::sparcv9:
   case llvm::Triple::amdgcn:
   case llvm::Triple::r600:
   case llvm::Triple::csky:
   case llvm::Triple::loongarch32:
   case llvm::Triple::loongarch64:
     return !areOptimizationsEnabled(Args);
   default:
     break;
   }
 
   if (Triple.isOSFuchsia() || Triple.isOSNetBSD()) {
     return !areOptimizationsEnabled(Args);
   }
 
   if (Triple.isOSLinux() || Triple.getOS() == llvm::Triple::CloudABI ||
       Triple.isOSHurd()) {
     switch (Triple.getArch()) {
     // Don't use a frame pointer on linux if optimizing for certain targets.
     case llvm::Triple::arm:
     case llvm::Triple::armeb:
     case llvm::Triple::thumb:
     case llvm::Triple::thumbeb:
-      if (Triple.isAndroid())
-        return true;
-      [[fallthrough]];
     case llvm::Triple::mips64:
     case llvm::Triple::mips64el:
     case llvm::Triple::mips:
     case llvm::Triple::mipsel:
     case llvm::Triple::systemz:
     case llvm::Triple::x86:
     case llvm::Triple::x86_64:
       return !areOptimizationsEnabled(Args);
     default:
getFramePointerKind(const ArgList &Args, const llvm::Triple &Triple) {
   Arg *A = Args.getLastArg(options::OPT_fomit_frame_pointer,
                            options::OPT_fno_omit_frame_pointer);
   bool OmitFP = A && A->getOption().matches(options::OPT_fomit_frame_pointer);
   bool NoOmitFP =
       A && A->getOption().matches(options::OPT_fno_omit_frame_pointer);
   bool OmitLeafFP =
       Args.hasFlag(options::OPT_momit_leaf_frame_pointer,
                    options::OPT_mno_omit_leaf_frame_pointer,
-                   Triple.isAArch64() || Triple.isPS() || Triple.isVE());
+                   Triple.isAArch64() || Triple.isPS() || Triple.isVE() ||
+                   (Triple.isAndroid() && Triple.isRISCV64()));
   if (NoOmitFP || mustUseNonLeafFramePointerForTarget(Triple) ||
       (!OmitFP && useFramePointerForTargetByDefault(Args, Triple))) {
     if (OmitLeafFP)
       return CodeGenOptions::FramePointerKind::NonLeaf;
     return CodeGenOptions::FramePointerKind::All;
   }
   return CodeGenOptions::FramePointerKind::None;
 }

clang/test/Driver/frame-pointer-elim.c

 // RUN: %clang -### -target x86_64-sie-ps5 -S %s 2>&1 | \
 // RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
 // RUN: %clang -### -target x86_64-sie-ps5 -S -O2 %s 2>&1 | \
 // RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
 // RUN: %clang -### -target aarch64-apple-darwin -arch arm64_32 -S %s 2>&1 | \
 // RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
 // RUN: %clang -### -target ve-unknown-linux-gnu -S %s 2>&1 | \
 // RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
+// RUN: %clang -### --target=aarch64-linux-android -S %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
+// RUN: %clang -### --target=aarch64-linux-android -S -O2 %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
+// RUN: %clang -### --target=aarch64-linux-android -S -Os %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
 
 // RUN: %clang -### -target powerpc64 -S %s 2>&1 | \
 // RUN:   FileCheck --check-prefix=KEEP-ALL %s
 // RUN: %clang -### -target powerpc64 -S -O1 %s 2>&1 | \
 // RUN:   FileCheck --check-prefix=KEEP-NONE %s
 
 // SPARC targets omit the frame pointer when optimizations are enabled.
 // RUN: %clang -### -target sparc -S %s 2>&1 | \

 // RUN: %clang -### -target armv7a-linux-androideabi- -marm -O1 -S %s 2>&1 | \
 // RUN:   FileCheck --check-prefix=KEEP-ALL %s
 // RUN: %clang -### -target armv7a-linux-androideabi- -mthumb -O1 -S %s 2>&1 | \
 // RUN:   FileCheck --check-prefix=KEEP-ALL %s
 // RUN: %clang -### -target armv7a-linux-androideabi- -marm -mbig-endian -O1 -S %s 2>&1 | \
 // RUN:   FileCheck --check-prefix=KEEP-ALL %s
 // RUN: %clang -### -target armv7a-linux-androideabi- -mthumb -mbig-endian -O1 -S %s 2>&1 | \
 // RUN:   FileCheck --check-prefix=KEEP-ALL %s
-
+// RUN: %clang -### --target=riscv64-linux-android -O1 -S %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
+// RUN: %clang -### --target=riscv64-linux-android -mbig-endian -O1 -S %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
 void f0() {}
 void f1() { f0(); }

clang/test/Driver/frame-pointer.c

 // RUN: %clang --target=riscv32-unknown-linux-gnu -### -S -Os %s 2>&1 | FileCheck -check-prefix=CHECKs-32 %s
 
 // RUN: %clang --target=riscv64-unknown-linux-gnu -### -S -O0 %s 2>&1 | FileCheck -check-prefix=CHECK0-64 %s
 // RUN: %clang --target=riscv64-unknown-linux-gnu -### -S -O1 %s 2>&1 | FileCheck -check-prefix=CHECK1-64 %s
 // RUN: %clang --target=riscv64-unknown-linux-gnu -### -S -O2 %s 2>&1 | FileCheck -check-prefix=CHECK2-64 %s
 // 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=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
 // RUN: %clang --target=loongarch32 -### -S -O2 %s -o %t.s 2>&1 | FileCheck -check-prefix=CHECK2-32 %s
 // RUN: %clang --target=loongarch32 -### -S -O3 %s -o %t.s 2>&1 | FileCheck -check-prefix=CHECK3-32 %s
 // RUN: %clang --target=loongarch32 -### -S -Os %s -o %t.s 2>&1 | FileCheck -check-prefix=CHECKs-32 %s
 
 // RUN: %clang --target=loongarch64 -### -S -O0 %s -o %t.s 2>&1 | FileCheck -check-prefix=CHECK0-64 %s
 // RUN: %clang --target=loongarch64 -### -S -O1 %s -o %t.s 2>&1 | FileCheck -check-prefix=CHECK1-64 %s
 // RUN: %clang --target=loongarch64 -### -S -O2 %s -o %t.s 2>&1 | FileCheck -check-prefix=CHECK2-64 %s
 // RUN: %clang --target=loongarch64 -### -S -O3 %s -o %t.s 2>&1 | FileCheck -check-prefix=CHECK3-64 %s
 // RUN: %clang --target=loongarch64 -### -S -Os %s -o %t.s 2>&1 | FileCheck -check-prefix=CHECKs-64 %s
 
 // CHECK0-32: -mframe-pointer=all
 // CHECK1-32-NOT: -mframe-pointer=all
 // CHECK2-32-NOT: -mframe-pointer=all
 // CHECK3-32-NOT: -mframe-pointer=all
 // CHECKs-32-NOT: -mframe-pointer=all
 
 // CHECK0-64: -mframe-pointer=all
 // CHECK1-64-NOT: -mframe-pointer=all
 // CHECK2-64-NOT: -mframe-pointer=all
 // CHECK3-64-NOT: -mframe-pointer=all
 // CHECKs-64-NOT: -mframe-pointer=all
 // CHECK-MACHO-64: -mframe-pointer=all
+
+// CHECK-ANDROID-64: -mframe-pointer=non-leaf