[PATCH] D74698: [CodeGen] -pg shouldn't add "frame-pointer"="all" fn attr w/ -mfentry

[Fangrui Song via Phabricator via cfe-commits]

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.

In D74698#1881111 <https://reviews.llvm.org/D74698#1881111>, @nickdesaulniers wrote:

> In D74698#1881034 <https://reviews.llvm.org/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) <https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=3c5273a96ba8dbf98c40bc6d9d0a1587b4cfedb2> 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;
  }



================
Comment at: clang/test/CodeGen/fentry.c:5
 // RUN: %clang_cc1 -mfentry -triple x86_64-unknown-linux-gnu -emit-llvm -o - %s | FileCheck -check-prefix=NOPG %s
+// RUN: %clang_cc1 -pg -mfentry -emit-llvm -o - %s | FileCheck -check-prefix=NOFP  %s
 
----------------
`--implicit-check-not='"frame-pointer"="all"'` may be better


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74698/new/

https://reviews.llvm.org/D74698