[Driver] -pg -mfentry should respect target specific decisions for -mframe-pointer=all

[nickdesaulniers]

Summary

$ clang -O2 -pg -mfentry foo.c

was adding frame pointers to all functions. This was exposed via
compiling the Linux kernel for x86_64 with CONFIG_FUNCTION_TRACER
enabled.

-pg was unconditionally setting the equivalent of -fno-omit-frame-pointer,
regardless of the presence of -mfentry or optimization level. After this
patch, frame pointers will only be omitted at -O0 or if
-fno-omit-frame-pointer is explicitly set for -pg -mfentry.

See also:
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=3c5273a96ba8dbf98c40bc6d9d0a1587b4cfedb2;hp=c9d75a48c4ea63ab27ccdb40f993236289b243f2#patch2
(modification to ix86_frame_pointer_required())

Fixes: pr/44934

[nickdesaulniers]

• add test line

[MaskRay]

When -mfentry is specified, why should frame pointers be disabled? Is that because the Linux kernel has assumption about the exact code sequence? call __fentry__ is the first instruction. Isn't that sufficient?

(There is another difference. GCC emits call *__fentry__@GOTPCREL(%rip) in -fpie/-fpic mode. At first glance, this looks suboptimal to me. I don't expect __fentry__ to be interposed.)
(This may be another example demonstrating that piggybacking an option (-mfentry) on top of an existing one (-pg) can turn out to be a bad idea...)

[nickdesaulniers]

In D74698#1881034, @MaskRay wrote:

When -mfentry is specified, why should frame pointers be disabled?

It doesn't disable them; -pg was force enabling them for all functions, when in GCC does not enable them for the combination of -pg -mfentry. This patch is simply matching the behavior of GCC. If you want the existing behavior, then -pg -mfentry -fno-omit-frame-pointer works with this patch applied. -pg should not be setting -fno-omit-frame-pointers in the presence of -mfentry.

Is that because the Linux kernel has assumption about the exact code sequence? call __fentry__ is the first instruction. Isn't that sufficient?

It's not that the current implementation is broken or doesn't work, it's that it's inefficient from the Kernel's perspective. The kernel does not want frame pointers as it has its own means for unwinding (though there are many unwinders in the kernel, sometimes per architecture, some do rely on frame pointers but explicitly add -fno-omit-frame-pointer if necessary). When configuring a kernel to be able to trace kernel execution at runtime, -pg -mfentry are added for x86_64, specifically because they don't add frame pointers. So it seems like Clang is strictly worse than GCC in this regard, as when enabling kernel tracing, suddenly clang starts emitting unwanted frame pointer instructions.

(This may be another example demonstrating that piggybacking an option (-mfentry) on top of an existing one (-pg) can turn out to be a bad idea...)

Agreed.

Also, more context:
https://www.linuxjournal.com/content/simplifying-function-tracing-modern-gcc
https://lore.kernel.org/patchwork/patch/1072232/
https://linux.kernel.narkive.com/X1y4Jcj4/rfc-how-to-handle-function-tracing-frame-pointers-and-mfentry

[MaskRay]

In D74698#1881111, @nickdesaulniers wrote:

In D74698#1881034, @MaskRay wrote:

When -mfentry is specified, why should frame pointers be disabled?

It doesn't disable them; -pg was force enabling them for all functions, when in GCC does not enable them for the combination of -pg -mfentry. This patch is simply matching the behavior of GCC. If you want the existing behavior, then -pg -mfentry -fno-omit-frame-pointer works with this patch applied. -pg should not be setting -fno-omit-frame-pointers in the presence of -mfentry.

Is that because the Linux kernel has assumption about the exact code sequence? call __fentry__ is the first instruction. Isn't that sufficient?

It's not that the current implementation is broken or doesn't work, it's that it's inefficient from the Kernel's perspective. The kernel does not want frame pointers as it has its own means for unwinding (though there are many unwinders in the kernel, sometimes per architecture, some do rely on frame pointers but explicitly add -fno-omit-frame-pointer if necessary). When configuring a kernel to be able to trace kernel execution at runtime, -pg -mfentry are added for x86_64, specifically because they don't add frame pointers. So it seems like Clang is strictly worse than GCC in this regard, as when enabling kernel tracing, suddenly clang starts emitting unwanted frame pointer instructions.

(This may be another example demonstrating that piggybacking an option (-mfentry) on top of an existing one (-pg) can turn out to be a bad idea...)

Agreed.

Also, more context:
https://www.linuxjournal.com/content/simplifying-function-tracing-modern-gcc
https://lore.kernel.org/patchwork/patch/1072232/
https://linux.kernel.narkive.com/X1y4Jcj4/rfc-how-to-handle-function-tracing-frame-pointers-and-mfentry

(I happened to have investigated these stuff 2 weeks ago... https://maskray.me/blog/2020-02-01-fpatchable-function-entry in case someone can read Chinese)

I believe -pg was invented in 1980s or earlier. I can find it in the first few commits of GCC SVN (circa 1990, after the repository was converted from RCS).

-pg has a good reason that it needs to retain frame pointers. Implementations usually read the caller return address via the frame pointer, e.g. glibc/sysdeps/x86_64/_mcount.S

	/* Get frompc via the frame pointer.  */
	movq	8(%rbp),%rdi
	call C_SYMBOL_NAME(__mcount_internal)

I really hope GCC r162651 (2010) (GCC 4.6) did not piggyback -mfentry on top of -pg...
Anyway, it is too late to change anything...

The patch matches gcc/config/i386/i386.c

static bool
ix86_frame_pointer_required (void)
{
...
  if (crtl->profile && !flag_fentry) /// -pg but not -mfentry
    return true;

  return false;
}

I also hope x86 maintainers can migrate to -fpatchable-function-entry= at some point, so that we can avoid linux/scripts/recordmcount*...

clang/test/CodeGen/fentry.c

5 ↗

(On Diff #244916)

--implicit-check-not='"frame-pointer"="all"' may be better

[nickdesaulniers]

• prefer implicit-check-not, add test for -O2 vs -O0

[nickdesaulniers]

• add test for explicitly re-enabling -fno-omit-frame-pointer at -O2

[MaskRay]

clang/test/CodeGen/fentry.c

6 ↗

(On Diff #245245)

Oh, %clang tests should be moved to test/Driver/mfentry.c

[nickdesaulniers]

• move test from clang/test/CodeGen/fentry to clang/test/Driver/mfentry

[MaskRay]

clang/test/Driver/mfentry.c

5

Change -emit-llvm to -### and delete --implicit-check-not. And add -target x86_64, because some targets don't support -mfentry.

For IR output, there can be multiple "frame-pointer"="all" function attributes. --implicit-check-not helps detect undesired attributes.

For cc1 options, there can't be multiple "frame-pointer" options. So --implicit-check-not is not useful.

14

// FP

[nickdesaulniers]

clang/test/Driver/mfentry.c

5

-target x86_64 seems like a good idea, but -### produces:

clang-10: warning: argument unused during compilation: '-pg' [-Wunused-command-line-argument]
clang-10: warning: argument unused during compilation: '-O2' [-Wunused-command-line-argument]
clang-10: warning: argument unused during compilation: '-mfentry' [-Wunused-command-line-argument]

so I don't think we want that. Or am I "holding it wrong?"

[MaskRay]

clang/test/Driver/mfentry.c

5

It works fine here.

Did you add some other unrelated options during testing? You can drop -o - by the way.

[nickdesaulniers]

• add explicit target and spacing

[nickdesaulniers]

• use -### -E and check for -mframe-pointer={all|none}

[nickdesaulniers]

• drop -o -

[nickdesaulniers]

• s/gnu-linux/linux-gnu/

[MaskRay]

The title should be changed from CodeGen to Driver

And fn attr is no longer appropriate.

[nickdesaulniers]

clang/test/Driver/mfentry.c

6

The only issue left is that -target x86_64 still produces frame pointers since the above useFramePointerForTargetByDefault() just returns true when no target triple OS is specified. Do we care about that case?

[nickdesaulniers]

• drop implicit-check-nots, add check for no os target

[nickdesaulniers]

clang/lib/Driver/ToolChains/Clang.cpp

592

This hunk does nothing and can be removed. I'll retitle the diff with a better explanation.

[nickdesaulniers]

• respect target specific frame pointer optimizations

[nickdesaulniers]

Ok, sorry for the excessive churn. I think this now accurately describes the logic of how this was expected to work, from the stance of GCC compatibility.

clang/lib/Driver/ToolChains/Clang.cpp

case llvm::Triple::thumb:
     // ARM Darwin targets require a frame pointer to be always present to aid
     // offline debugging via backtraces.
     return Triple.isOSDarwin();
   }
 }
 
 static bool useFramePointerForTargetByDefault(const ArgList &Args,
                                               const llvm::Triple &Triple) {
-  if (Args.hasArg(options::OPT_pg))
+  if (Args.hasArg(options::OPT_pg) && !Args.hasArg(options::OPT_mfentry))
     return true;
 
   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.
default:
       // All other supported Windows ISAs use xdata unwind information, so frame
       // pointers are not generally useful.
       return false;
     }
   }
 
   return true;
 }
 
 static CodeGenOptions::FramePointerKind
 getFramePointerKind(const ArgList &Args, const llvm::Triple &Triple) {
   // We have 4 states:
   //
   //  00) leaf retained, non-leaf retained
   //  01) leaf retained, non-leaf omitted (this is invalid)
   //  10) leaf omitted, non-leaf retained
   //      (what -momit-leaf-frame-pointer was designed for)

   // Make the compile command echo its inputs for /showFilenames.
   if (Output.getType() == types::TY_Object &&
       Args.hasFlag(options::OPT__SLASH_showFilenames,
                    options::OPT__SLASH_showFilenames_, false)) {
     C.getJobs().getJobs().back()->PrintInputFilenames = true;
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_pg))
-    if (FPKeepKind == CodeGenOptions::FramePointerKind::None)
+    if (FPKeepKind == CodeGenOptions::FramePointerKind::None &&
+        !Args.hasArg(options::OPT_mfentry))
       D.Diag(diag::err_drv_argument_not_allowed_with) << "-fomit-frame-pointer"
                                                       << A->getAsString(Args);
 
   // Claim some arguments which clang supports automatically.
 
   // -fpch-preprocess is used with gcc to add a special marker in the output to
   // include the PCH file.
   Args.ClaimAllArgs(options::OPT_fpch_preprocess);

clang/test/Driver/mfentry.c

 // RUN: %clang -target s390x -c -### %s -mfentry 2>&1 | FileCheck %s
 // RUN: %clang -target i386 -c -### %s -mfentry 2>&1 | FileCheck %s
 // RUN: %clang -target x86_64 -c -### %s -mfentry 2>&1 | FileCheck %s
+// RUN: %clang -pg -mfentry -O0 -emit-llvm -S -o - %s | FileCheck -check-prefix=FP --implicit-check-not='"frame-pointer"="none"' %s
+// RUN: %clang -pg -mfentry -O2 -fno-omit-frame-pointer -emit-llvm -S -o - %s | FileCheck -check-prefix=FP --implicit-check-not='"frame-pointer"="none"' %s
+// RUN: %clang -pg -mfentry -O2 -emit-llvm -S -o - %s | FileCheck -check-prefix=NOFP --implicit-check-not='"frame-pointer"="all"' %s
 
 // CHECK: "-mfentry"
 
 // RUN: %clang -target powerpc64le -c -### %s -mfentry 2>&1 | FileCheck --check-prefix=ERR %s
 
 // ERR: error: unsupported option '-mfentry' for target 'powerpc64le'
+
+//FP: "frame-pointer"="all"
+//NOFP: "frame-pointer"="none"
+void foo(void) {}