Page MenuHomePhabricator

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

Authored by nickdesaulniers on Feb 16 2020, 9:35 PM.

Details

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

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2020, 9:35 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
  • add test line
MaskRay added a comment.EditedFeb 18 2020, 10:41 AM

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

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 accepted this revision.EditedFeb 18 2020, 11:45 AM

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

This revision is now accepted and ready to land.Feb 18 2020, 11:45 AM
  • prefer implicit-check-not, add test for -O2 vs -O0
nickdesaulniers marked an inline comment as done.Feb 18 2020, 1:07 PM
nickdesaulniers retitled this revision from [CodeGen] -pg shouldn't add "frame-pointer"="all" fn attr w/ -mfentry to [CodeGen] -pg shouldn't unconditionally add "frame-pointer"="all" fn attr w/ -mfentry.Feb 18 2020, 1:13 PM
nickdesaulniers edited the summary of this revision. (Show Details)
  • add test for explicitly re-enabling -fno-omit-frame-pointer at -O2
nickdesaulniers edited the summary of this revision. (Show Details)Feb 18 2020, 1:15 PM
MaskRay added inline comments.Feb 18 2020, 1:17 PM
clang/test/CodeGen/fentry.c
6 ↗(On Diff #245245)

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

  • move test from clang/test/CodeGen/fentry to clang/test/Driver/mfentry
nickdesaulniers edited the summary of this revision. (Show Details)Feb 18 2020, 1:29 PM
nickdesaulniers edited the summary of this revision. (Show Details)
MaskRay added inline comments.Feb 18 2020, 1:48 PM
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.

17

// FP

nickdesaulniers marked an inline comment as done.Feb 18 2020, 1:55 PM
nickdesaulniers added inline comments.
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 added inline comments.Feb 18 2020, 2:02 PM
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.

  • add explicit target and spacing
nickdesaulniers marked an inline comment as done.Feb 18 2020, 2:05 PM
  • use -### -E and check for -mframe-pointer={all|none}
nickdesaulniers marked 2 inline comments as done.Feb 18 2020, 2:12 PM
  • s/gnu-linux/linux-gnu/
Harbormaster completed remote builds in B46756: Diff 245264.
Harbormaster completed remote builds in B46757: Diff 245265.
MaskRay accepted this revision.Feb 18 2020, 2:18 PM

The title should be changed from CodeGen to Driver

And fn attr is no longer appropriate.

nickdesaulniers retitled this revision from [CodeGen] -pg shouldn't unconditionally add "frame-pointer"="all" fn attr w/ -mfentry to [Driver] -pg shouldn't unconditionally add "frame-pointer"="all" fn attr w/ -mfentry.Feb 18 2020, 2:27 PM
nickdesaulniers retitled this revision from [Driver] -pg shouldn't unconditionally add "frame-pointer"="all" fn attr w/ -mfentry to [Driver] -pg shouldn't unconditionally add -mframe-pointer=all w/ -mfentry.
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?

  • drop implicit-check-nots, add check for no os target
nickdesaulniers marked an inline comment as done.Feb 18 2020, 2:42 PM
nickdesaulniers added inline comments.
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 retitled this revision from [Driver] -pg shouldn't unconditionally add -mframe-pointer=all w/ -mfentry to [Driver] -pg -mfentry should respect target specific decisions for -mframe-pointer=all.Feb 18 2020, 2:43 PM
  • respect target specific frame pointer optimizations

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.

MaskRay accepted this revision.Feb 18 2020, 2:52 PM
This revision was automatically updated to reflect the committed changes.