[Clang FE, SystemZ] Recognize -mrecord-mcount CL option.

[jonpa]

Summary

Recognize -mrecord-mcount from the command line and add a function attribute "mrecord-mcount when passed.

Only valid on SystemZ when used with -mfentry.

[jonpa]

Probably better to only have one hard-coded space character in mrecord-mcount.c CHECK line.

[uweigand]

LGTM.

[jonpa]

2520bef

[nickdesaulniers]

This is regressing linux kernel builds for multiple architectures. The linux kernel uses feature detection for compiler flags, basically invoking:

$ clang -mrecord-mcount -target aarch64-linux-gnu -c -x c /dev/null

Because the target info isn't validated until code gen, the above returns 0, which seems like the flag is supported, so it is used for further compilations which then do fail.

[jonpa]

In D71627#1793006, @nickdesaulniers wrote:

This is regressing linux kernel builds for multiple architectures. The linux kernel uses feature detection for compiler flags, basically invoking:

$ clang -mrecord-mcount -target aarch64-linux-gnu -c -x c /dev/null

Because the target info isn't validated until code gen, the above returns 0, which seems like the flag is supported, so it is used for further compilations which then do fail.

I am not sure how this should be handled... Maybe you could open an issue on Bugzilla and add the right people on the CC list (I am not sure exactly who they would be...)?

[uweigand]

Can we simply move all the validity checks (arg only valid on SystemZ, or arg only valid in combination with another arg) into ParseCodeGenArgs in CompilerInvocation.cpp, where the args are initially looked at?

[nathanchance]

In D71627#1793142, @jonpa wrote:

In D71627#1793006, @nickdesaulniers wrote:

This is regressing linux kernel builds for multiple architectures. The linux kernel uses feature detection for compiler flags, basically invoking:

$ clang -mrecord-mcount -target aarch64-linux-gnu -c -x c /dev/null

Because the target info isn't validated until code gen, the above returns 0, which seems like the flag is supported, so it is used for further compilations which then do fail.

I am not sure how this should be handled... Maybe you could open an issue on Bugzilla and add the right people on the CC list (I am not sure exactly who they would be...)?

While that is discussed, I think this commit should be reverted. All working Linux kernel allyesconfig builds with Clang are now broken with this commit because of what Nick pointed out (-mrecord-mcount gets added to the list of CFLAGS then the build fails when actual code generation starts).

[MaskRay]

In D71627#1794089, @nathanchance wrote:

In D71627#1793142, @jonpa wrote:

In D71627#1793006, @nickdesaulniers wrote:

This is regressing linux kernel builds for multiple architectures. The linux kernel uses feature detection for compiler flags, basically invoking:

$ clang -mrecord-mcount -target aarch64-linux-gnu -c -x c /dev/null

Because the target info isn't validated until code gen, the above returns 0, which seems like the flag is supported, so it is used for further compilations which then do fail.

I am not sure how this should be handled... Maybe you could open an issue on Bugzilla and add the right people on the CC list (I am not sure exactly who they would be...)?

While that is discussed, I think this commit should be reverted. All working Linux kernel allyesconfig builds with Clang are now broken with this commit because of what Nick pointed out (-mrecord-mcount gets added to the list of CFLAGS then the build fails when actual code generation starts).

I noticed this problem as well. I will try fixing it..

[MaskRay]

Fixed by 0792ef72564071f21b727d0de3a14d565950290f. My Linux powerpc64le build is good now.

make ARCH=powerpc CROSS_COMPILE=powerpc64le-linux-gnu- CC=~/llvm/Release/bin/clang LD=~/llvm/Release/bin/ld.lld O=out/powerpc64le powernv_defconfig zImage.epapr -j 30

[uweigand]

In D71627#1794108, @MaskRay wrote:

Fixed by 0792ef72564071f21b727d0de3a14d565950290f. My Linux powerpc64le build is good now.

make ARCH=powerpc CROSS_COMPILE=powerpc64le-linux-gnu- CC=~/llvm/Release/bin/clang LD=~/llvm/Release/bin/ld.lld O=out/powerpc64le powernv_defconfig zImage.epapr -j 30

Thank for you the fix, and sorry for the disruption!

There is another limitation of the current implementation: -mrecord-mcount / -mnop-mcount are only supported with -mfentry, not with the "regular" mcount profiling. This is just a limitation of the current implementation (but in any case, the Linux kernel build will only use it in this combination anyway).

We currently have checks that this combination is specified, but those are also in the code generator. Should they be moved to the driver as well?

[MaskRay]

In D71627#1794127, @uweigand wrote:

In D71627#1794108, @MaskRay wrote:

Fixed by 0792ef72564071f21b727d0de3a14d565950290f. My Linux powerpc64le build is good now.

make ARCH=powerpc CROSS_COMPILE=powerpc64le-linux-gnu- CC=~/llvm/Release/bin/clang LD=~/llvm/Release/bin/ld.lld O=out/powerpc64le powernv_defconfig zImage.epapr -j 30

Thank for you the fix, and sorry for the disruption!

There is another limitation of the current implementation: -mrecord-mcount / -mnop-mcount are only supported with -mfentry, not with the "regular" mcount profiling. This is just a limitation of the current implementation (but in any case, the Linux kernel build will only use it in this combination anyway).

We currently have checks that this combination is specified, but those are also in the code generator. Should they be moved to the driver as well?

gcc/config/s390/s390.c requires -mnop-mcount to be used together with -mfentry, but gcc/config/i386/i386-options.c doesn't.

% clang -target s390x -pg -mnop-mcount -c a.c      
error: option '-mnop-mcount' cannot be specified without '-mfentry'
1 error generated.
% gcc -pg -mnop-mcount -fno-pic -c a.c

(GCC performs some other checks. I noted some in commit 527b0f8c7448707aa6b8159e6e4707a49f1dcb87, but I am not sure whether we should document all... These options are only used by Linux kernel, and more specifically, ftrace.)

clang/include/clang/Basic/CodeGenOptions.def

 
 ///< Set the minimum number of instructions in a function to determine selective
 ///< XRay instrumentation.
 VALUE_CODEGENOPT(XRayInstructionThreshold , 32, 200)
 
 CODEGENOPT(InstrumentForProfiling , 1, 0) ///< Set when -pg is enabled.
 CODEGENOPT(CallFEntry , 1, 0) ///< Set when -mfentry is enabled.
 CODEGENOPT(MNopMCount , 1, 0) ///< Set when -mnop-mcount is enabled.
+CODEGENOPT(RecordMCount , 1, 0) ///< Set when -mrecord-mcount is enabled.
 CODEGENOPT(PackedStack , 1, 0) ///< Set when -mpacked-stack is enabled.
 CODEGENOPT(LessPreciseFPMAD  , 1, 0) ///< Enable less precise MAD instructions to
                                      ///< be generated.
 CODEGENOPT(PrepareForLTO     , 1, 0) ///< Set when -flto is enabled on the
                                      ///< compile step.
 CODEGENOPT(PrepareForThinLTO , 1, 0) ///< Set when -flto=thin is enabled on the
                                      ///< compile step.
 CODEGENOPT(LTOUnit, 1, 0) ///< Emit IR to support LTO unit features (CFI, whole

clang/include/clang/Driver/Options.td

 def mpie_copy_relocations : Flag<["-"], "mpie-copy-relocations">, Group<m_Group>,
   Flags<[CC1Option]>,
   HelpText<"Use copy relocations support for PIE builds">;
 def mno_pie_copy_relocations : Flag<["-"], "mno-pie-copy-relocations">, Group<m_Group>;
 def mfentry : Flag<["-"], "mfentry">, HelpText<"Insert calls to fentry at function entry (x86/SystemZ only)">,
   Flags<[CC1Option]>, Group<m_Group>;
 def mnop_mcount : Flag<["-"], "mnop-mcount">, HelpText<"Generate mcount/__fentry__ calls as nops. To activate they need to be patched in.">,
   Flags<[CC1Option]>, Group<m_Group>;
+def mrecord_mcount : Flag<["-"], "mrecord-mcount">, HelpText<"Generate a __mcount_loc section entry for each __fentry__ call.">,
+  Flags<[CC1Option]>, Group<m_Group>;
 def mpacked_stack : Flag<["-"], "mpacked-stack">, HelpText<"Use packed stack layout (SystemZ only).">,
   Flags<[CC1Option]>, Group<m_Group>;
 def mips16 : Flag<["-"], "mips16">, Group<m_mips_Features_Group>;
 def mno_mips16 : Flag<["-"], "mno-mips16">, Group<m_mips_Features_Group>;
 def mmicromips : Flag<["-"], "mmicromips">, Group<m_mips_Features_Group>;
 def mno_micromips : Flag<["-"], "mno-micromips">, Group<m_mips_Features_Group>;
 def mxgot : Flag<["-"], "mxgot">, Group<m_mips_Features_Group>;
 def mno_xgot : Flag<["-"], "mno-xgot">, Group<m_mips_Features_Group>;

clang/lib/CodeGen/CodeGenFunction.cpp

if (!CurFuncDecl || !CurFuncDecl->hasAttr<NoInstrumentFunctionAttr>()) {
             llvm::Triple::systemz)
           CGM.getDiags().Report(diag::err_opt_not_valid_on_target)
             << "-mnop-mcount";
         if (!CGM.getCodeGenOpts().CallFEntry)
           CGM.getDiags().Report(diag::err_opt_not_valid_without_opt)
             << "-mnop-mcount" << "-mfentry";
         Fn->addFnAttr("mnop-mcount");
       }
+
+      if (CGM.getCodeGenOpts().RecordMCount) {
+        if (getContext().getTargetInfo().getTriple().getArch() !=
+            llvm::Triple::systemz)
+          CGM.getDiags().Report(diag::err_opt_not_valid_on_target)
+            << "-mrecord-mcount";
+        if (!CGM.getCodeGenOpts().CallFEntry)
+          CGM.getDiags().Report(diag::err_opt_not_valid_without_opt)
+            << "-mrecord-mcount" << "-mfentry";
+        Fn->addFnAttr("mrecord-mcount");
+      }
     }
   }
 
   if (CGM.getCodeGenOpts().PackedStack) {
     if (getContext().getTargetInfo().getTriple().getArch() !=
         llvm::Triple::systemz)
       CGM.getDiags().Report(diag::err_opt_not_valid_on_target)
         << "-mpacked-stack";

clang/lib/Driver/ToolChains/Clang.cpp

if (TC.SupportsProfiling())
     Args.AddLastArg(CmdArgs, options::OPT_pg);
 
   if (TC.SupportsProfiling())
     Args.AddLastArg(CmdArgs, options::OPT_mfentry);
 
   if (TC.SupportsProfiling())
     Args.AddLastArg(CmdArgs, options::OPT_mnop_mcount);
 
+  if (TC.SupportsProfiling())
+    Args.AddLastArg(CmdArgs, options::OPT_mrecord_mcount);
+
   Args.AddLastArg(CmdArgs, options::OPT_mpacked_stack);
 
   if (Args.getLastArg(options::OPT_fapple_kext) ||
       (Args.hasArg(options::OPT_mkernel) && types::isCXX(InputType)))
     CmdArgs.push_back("-fapple-kext");
 
   Args.AddLastArg(CmdArgs, options::OPT_flax_vector_conversions_EQ);
   Args.AddLastArg(CmdArgs, options::OPT_fobjc_sender_dependent_dispatch);

clang/lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   else
     for (const auto &A : XRayInstrBundles)
       parseXRayInstrumentationBundle("-fxray-instrumentation-bundle=", A, Args,
                                      Diags, Opts.XRayInstrumentationBundle);
 
   Opts.InstrumentForProfiling = Args.hasArg(OPT_pg);
   Opts.CallFEntry = Args.hasArg(OPT_mfentry);
   Opts.MNopMCount = Args.hasArg(OPT_mnop_mcount);
+  Opts.RecordMCount = Args.hasArg(OPT_mrecord_mcount);
   Opts.PackedStack = Args.hasArg(OPT_mpacked_stack);
   Opts.EmitOpenCLArgMetadata = Args.hasArg(OPT_cl_kernel_arg_info);
 
   if (const Arg *A = Args.getLastArg(OPT_fcf_protection_EQ)) {
     StringRef Name = A->getValue();
     if (Name == "full") {
       Opts.CFProtectionReturn = 1;
       Opts.CFProtectionBranch = 1;

clang/test/CodeGen/mrecord-mcount.c

+// RUN: %clang_cc1 -pg -mfentry -mrecord-mcount -triple s390x-ibm-linux -emit-llvm \
+// RUN:   -o - %s 2>&1 | FileCheck  %s
+// RUN: not %clang_cc1 -pg -mrecord-mcount -triple s390x-ibm-linux -emit-llvm -o - \
+// RUN:   %s 2>&1 | FileCheck -check-prefix=NOMFENTRY %s
+// RUN: %clang_cc1 -mfentry -mrecord-mcount -triple s390x-ibm-linux -emit-llvm -o - \
+// RUN:   %s 2>&1 | FileCheck -check-prefix=NOPG %s
+// RUN: %clang_cc1 -mrecord-mcount -triple s390x-ibm-linux -emit-llvm -o - %s \
+// RUN:   2>&1 | FileCheck -check-prefix=NOPG %s
+// RUN: not %clang_cc1 -pg -mfentry -mrecord-mcount -triple x86_64-linux-gnu \
+// RUN:   -emit-llvm -o - %s 2>&1 | FileCheck -check-prefix=X86 %s
+
+int foo(void) {
+  return 0;
+}
+
+int __attribute__((no_instrument_function)) no_instrument(void) {
+  return foo();
+}
+
+//CHECK: attributes #0 = { {{.*}}"mrecord-mcount"{{.*}} }
+//CHECK: attributes #1 = { {{.*}} }
+//CHECK-NOT: attributes #1 = { {{.*}}"mrecord-mcount"{{.*}} }
+//NOMFENTRY: error: option '-mrecord-mcount' cannot be specified without '-mfentry'
+//NOPG-NOT: attributes #0 = { {{.*}}"mrecord-mcount"{{.*}} }
+//NOPG-NOT: attributes #1 = { {{.*}}"mrecord-mcount"{{.*}} }
+//X86: error: option '-mrecord-mcount' cannot be specified on this target