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

2020-02-18 Thread Nick Desaulniers via Phabricator via cfe-commits
nickdesaulniers updated this revision to Diff 245241.
nickdesaulniers added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74698

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/fentry.c


Index: clang/test/CodeGen/fentry.c
===
--- clang/test/CodeGen/fentry.c
+++ clang/test/CodeGen/fentry.c
@@ -2,6 +2,8 @@
 // RUN: %clang_cc1 -pg -mfentry -triple x86_64-unknown-linux-gnu -emit-llvm -o 
- %s | FileCheck %s
 // RUN: %clang_cc1 -mfentry -triple i386-unknown-unknown -emit-llvm -o - %s | 
FileCheck -check-prefix=NOPG %s
 // RUN: %clang_cc1 -mfentry -triple x86_64-unknown-linux-gnu -emit-llvm -o - 
%s | FileCheck -check-prefix=NOPG %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 -emit-llvm -S -o - %s | FileCheck 
-check-prefix=NOFP --implicit-check-not='"frame-pointer"="all"' %s
 
 int foo(void) {
   return 0;
@@ -16,3 +18,5 @@
 //CHECK-NOT: attributes #1 = { {{.*}}"fentry-call"="true"{{.*}} }
 //NOPG-NOT: attributes #0 = { {{.*}}"fentry-call"{{.*}} }
 //NOPG-NOT: attributes #1 = { {{.*}}"fentry-call"{{.*}} }
+//FP: "frame-pointer"="all"
+//NOFP: "frame-pointer"="none"
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -526,7 +526,7 @@
 
 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()) {
@@ -6150,7 +6150,8 @@
   }
 
   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);
 


Index: clang/test/CodeGen/fentry.c
===
--- clang/test/CodeGen/fentry.c
+++ clang/test/CodeGen/fentry.c
@@ -2,6 +2,8 @@
 // RUN: %clang_cc1 -pg -mfentry -triple x86_64-unknown-linux-gnu -emit-llvm -o - %s | FileCheck %s
 // RUN: %clang_cc1 -mfentry -triple i386-unknown-unknown -emit-llvm -o - %s | FileCheck -check-prefix=NOPG %s
 // RUN: %clang_cc1 -mfentry -triple x86_64-unknown-linux-gnu -emit-llvm -o - %s | FileCheck -check-prefix=NOPG %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 -emit-llvm -S -o - %s | FileCheck -check-prefix=NOFP --implicit-check-not='"frame-pointer"="all"' %s
 
 int foo(void) {
   return 0;
@@ -16,3 +18,5 @@
 //CHECK-NOT: attributes #1 = { {{.*}}"fentry-call"="true"{{.*}} }
 //NOPG-NOT: attributes #0 = { {{.*}}"fentry-call"{{.*}} }
 //NOPG-NOT: attributes #1 = { {{.*}}"fentry-call"{{.*}} }
+//FP: "frame-pointer"="all"
+//NOFP: "frame-pointer"="none"
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -526,7 +526,7 @@
 
 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()) {
@@ -6150,7 +6150,8 @@
   }
 
   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);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2020-02-18 Thread 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#188 , @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.  */
movq8(%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;
  }




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


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

2020-02-18 Thread Nick Desaulniers via Phabricator via cfe-commits
nickdesaulniers added a comment.

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


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


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

2020-02-18 Thread Fangrui Song via Phabricator via cfe-commits
MaskRay added a comment.

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...)


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


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

2020-02-16 Thread Nick Desaulniers via Phabricator via cfe-commits
nickdesaulniers updated this revision to Diff 244916.
nickdesaulniers added a comment.

- add test line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74698

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/fentry.c


Index: clang/test/CodeGen/fentry.c
===
--- clang/test/CodeGen/fentry.c
+++ clang/test/CodeGen/fentry.c
@@ -2,6 +2,7 @@
 // RUN: %clang_cc1 -pg -mfentry -triple x86_64-unknown-linux-gnu -emit-llvm -o 
- %s | FileCheck %s
 // RUN: %clang_cc1 -mfentry -triple i386-unknown-unknown -emit-llvm -o - %s | 
FileCheck -check-prefix=NOPG %s
 // 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
 
 int foo(void) {
   return 0;
@@ -16,3 +17,5 @@
 //CHECK-NOT: attributes #1 = { {{.*}}"fentry-call"="true"{{.*}} }
 //NOPG-NOT: attributes #0 = { {{.*}}"fentry-call"{{.*}} }
 //NOPG-NOT: attributes #1 = { {{.*}}"fentry-call"{{.*}} }
+//NOFP-NOT: "frame-pointer"="all"
+//NOFP: "frame-pointer"="none"
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -526,7 +526,7 @@
 
 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()) {
@@ -6150,7 +6150,8 @@
   }
 
   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);
 


Index: clang/test/CodeGen/fentry.c
===
--- clang/test/CodeGen/fentry.c
+++ clang/test/CodeGen/fentry.c
@@ -2,6 +2,7 @@
 // RUN: %clang_cc1 -pg -mfentry -triple x86_64-unknown-linux-gnu -emit-llvm -o - %s | FileCheck %s
 // RUN: %clang_cc1 -mfentry -triple i386-unknown-unknown -emit-llvm -o - %s | FileCheck -check-prefix=NOPG %s
 // 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
 
 int foo(void) {
   return 0;
@@ -16,3 +17,5 @@
 //CHECK-NOT: attributes #1 = { {{.*}}"fentry-call"="true"{{.*}} }
 //NOPG-NOT: attributes #0 = { {{.*}}"fentry-call"{{.*}} }
 //NOPG-NOT: attributes #1 = { {{.*}}"fentry-call"{{.*}} }
+//NOFP-NOT: "frame-pointer"="all"
+//NOFP: "frame-pointer"="none"
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -526,7 +526,7 @@
 
 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()) {
@@ -6150,7 +6150,8 @@
   }
 
   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);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2020-02-16 Thread Nick Desaulniers via Phabricator via cfe-commits
nickdesaulniers created this revision.
nickdesaulniers added reviewers: void, manojgupta, dberris.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

$ 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.

Fixes: pr/44934


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74698

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/fentry.c


Index: clang/test/CodeGen/fentry.c
===
--- clang/test/CodeGen/fentry.c
+++ clang/test/CodeGen/fentry.c
@@ -2,6 +2,7 @@
 // RUN: %clang_cc1 -pg -mfentry -triple x86_64-unknown-linux-gnu -emit-llvm -o 
- %s | FileCheck %s
 // RUN: %clang_cc1 -mfentry -triple i386-unknown-unknown -emit-llvm -o - %s | 
FileCheck -check-prefix=NOPG %s
 // 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
 
 int foo(void) {
   return 0;
@@ -16,3 +17,4 @@
 //CHECK-NOT: attributes #1 = { {{.*}}"fentry-call"="true"{{.*}} }
 //NOPG-NOT: attributes #0 = { {{.*}}"fentry-call"{{.*}} }
 //NOPG-NOT: attributes #1 = { {{.*}}"fentry-call"{{.*}} }
+//NOFP-NOT: "frame-pointer"="all"
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -526,7 +526,7 @@
 
 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()) {
@@ -6150,7 +6150,8 @@
   }
 
   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);
 


Index: clang/test/CodeGen/fentry.c
===
--- clang/test/CodeGen/fentry.c
+++ clang/test/CodeGen/fentry.c
@@ -2,6 +2,7 @@
 // RUN: %clang_cc1 -pg -mfentry -triple x86_64-unknown-linux-gnu -emit-llvm -o - %s | FileCheck %s
 // RUN: %clang_cc1 -mfentry -triple i386-unknown-unknown -emit-llvm -o - %s | FileCheck -check-prefix=NOPG %s
 // 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
 
 int foo(void) {
   return 0;
@@ -16,3 +17,4 @@
 //CHECK-NOT: attributes #1 = { {{.*}}"fentry-call"="true"{{.*}} }
 //NOPG-NOT: attributes #0 = { {{.*}}"fentry-call"{{.*}} }
 //NOPG-NOT: attributes #1 = { {{.*}}"fentry-call"{{.*}} }
+//NOFP-NOT: "frame-pointer"="all"
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -526,7 +526,7 @@
 
 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()) {
@@ -6150,7 +6150,8 @@
   }
 
   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);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits