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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
