Avoid conflicts between debug-info and pseudo-probe profiling

[probinson]

Summary

After D93264, using both -fdebug-info-for-profiling and -fpseudo-probe-for-profiling will cause the compiler to crash. Diagnose these conflicting options in the driver.

Also, the driver had a redundant pass-through of the option, and the existing CodeGen test was using the driver when it should be running cc1. Fixed up those as well.

[hoy]

Thanks for fixing these issues! LGTM.

[probinson]

the driver had a redundant pass-through of the option

I could've sworn it worked to remove that, but it didn't when I rebased, so that's gone from the final patch (i.e, the explicit pass-through in the driver is still there). It's a functionally separate topic anyway.

[dblaikie]

@hoy Could you explain a bit further why these two features are incompatible/what the crash looks like? At first glance I wouldn't expect any debug info mode to be incompatible with any non-debug-info mode (maybe less useful, but not crashy levels of incompatible).

[hoy]

In D96354#2554129, @probinson wrote:

the driver had a redundant pass-through of the option

I could've sworn it worked to remove that, but it didn't when I rebased, so that's gone from the final patch (i.e, the explicit pass-through in the driver is still there). It's a functionally separate topic anyway.

Soung

In D96354#2566273, @dblaikie wrote:

@hoy Could you explain a bit further why these two features are incompatible/what the crash looks like? At first glance I wouldn't expect any debug info mode to be incompatible with any non-debug-info mode (maybe less useful, but not crashy levels of incompatible).

Both debug-info mode and the pseudo-probe mode use the Dwarf discriminators but for different purposes. Therefore the passes that populate the Dwarf discriminators should not be scheduled at the same time. The crash was like compiler fatal error before this change. It

[dblaikie]

In D96354#2566502, @hoy wrote:

In D96354#2554129, @probinson wrote:

the driver had a redundant pass-through of the option

I could've sworn it worked to remove that, but it didn't when I rebased, so that's gone from the final patch (i.e, the explicit pass-through in the driver is still there). It's a functionally separate topic anyway.

Soung

In D96354#2566273, @dblaikie wrote:

@hoy Could you explain a bit further why these two features are incompatible/what the crash looks like? At first glance I wouldn't expect any debug info mode to be incompatible with any non-debug-info mode (maybe less useful, but not crashy levels of incompatible).

Both debug-info mode and the pseudo-probe mode use the Dwarf discriminators but for different purposes. Therefore the passes that populate the Dwarf discriminators should not be scheduled at the same time. The crash was like compiler fatal error before this change. It

Oh, that's subtle and seems somewhat unfortunate. Is that documented somewhere I could read more about?

[hoy]

In D96354#2566511, @dblaikie wrote:

In D96354#2566502, @hoy wrote:

In D96354#2554129, @probinson wrote:

the driver had a redundant pass-through of the option

I could've sworn it worked to remove that, but it didn't when I rebased, so that's gone from the final patch (i.e, the explicit pass-through in the driver is still there). It's a functionally separate topic anyway.

Soung

In D96354#2566273, @dblaikie wrote:

@hoy Could you explain a bit further why these two features are incompatible/what the crash looks like? At first glance I wouldn't expect any debug info mode to be incompatible with any non-debug-info mode (maybe less useful, but not crashy levels of incompatible).

Both debug-info mode and the pseudo-probe mode use the Dwarf discriminators but for different purposes. Therefore the passes that populate the Dwarf discriminators should not be scheduled at the same time. The crash was like compiler fatal error before this change. It

Oh, that's subtle and seems somewhat unfortunate. Is that documented somewhere I could read more about?

The summary of this diff has some information https://reviews.llvm.org/D91756 . Unfortunately we haven't rolled out a formal document yet.

clang/lib/Driver/ToolChains/Clang.cpp

 }
 
 static void renderDebugOptions(const ToolChain &TC, const Driver &D,
                                const llvm::Triple &T, const ArgList &Args,
                                bool EmitCodeView, bool IRInput,
                                ArgStringList &CmdArgs,
                                codegenoptions::DebugInfoKind &DebugInfoKind,
                                DwarfFissionKind &DwarfFission) {
+  // These two forms of profiling info can't be used together.
+  if (const Arg *A1 = Args.getLastArg(options::OPT_fpseudo_probe_for_profiling))
+    if (const Arg *A2 = Args.getLastArg(options::OPT_fdebug_info_for_profiling))
+      D.Diag(diag::err_drv_argument_not_allowed_with)
+          << A1->getAsString(Args) << A2->getAsString(Args);
+
   if (Args.hasFlag(options::OPT_fdebug_info_for_profiling,
                    options::OPT_fno_debug_info_for_profiling, false) &&
       checkDebugInfoOption(
           Args.getLastArg(options::OPT_fdebug_info_for_profiling), Args, D, TC))
     CmdArgs.push_back("-fdebug-info-for-profiling");
 
   // The 'g' groups options involve a somewhat intricate sequence of decisions
   // about what to pass from the driver to the frontend, but by the time they
if (Arg *A = getLastProfileSampleUseArg(Args)) {
     StringRef fname = A->getValue();
     if (!llvm::sys::fs::exists(fname))
       D.Diag(diag::err_drv_no_such_file) << fname;
     else
       A->render(Args, CmdArgs);
   }
   Args.AddLastArg(CmdArgs, options::OPT_fprofile_remapping_file_EQ);
 
-  if (Args.hasFlag(options::OPT_fpseudo_probe_for_profiling,
-                   options::OPT_fno_pseudo_probe_for_profiling, false))
-    CmdArgs.push_back("-fpseudo-probe-for-profiling");
-
   RenderBuiltinOptions(TC, RawTriple, Args, CmdArgs);
 
   if (!Args.hasFlag(options::OPT_fassume_sane_operator_new,
                     options::OPT_fno_assume_sane_operator_new))
     CmdArgs.push_back("-fno-assume-sane-operator-new");
 
   // -fblocks=0 is default.
   if (Args.hasFlag(options::OPT_fblocks, options::OPT_fno_blocks,

clang/test/CodeGen/pseudo-probe-emit.c

-// RUN: %clang -O2  -fexperimental-new-pass-manager -fpseudo-probe-for-profiling -g -emit-llvm -S -o - %s | FileCheck %s
+// RUN: %clang_cc1 -O2 -fno-legacy-pass-manager -fpseudo-probe-for-profiling -debug-info-kind=limited -emit-llvm -o - %s | FileCheck %s
 
 // Check the generation of pseudoprobe intrinsic call
 
 void bar();
 void go();
 
 void foo(int x) {
   // CHECK: call void @llvm.pseudoprobe(i64 [[#GUID:]], i64 1, i32 0, i64 -1)
   if (x == 0)
     // CHECK: call void @llvm.pseudoprobe(i64 [[#GUID]], i64 2, i32 0, i64 -1)
     bar();
   else
     // CHECK: call void @llvm.pseudoprobe(i64 [[#GUID]], i64 3, i32 0, i64 -1)
     go();
   // CHECK: call void @llvm.pseudoprobe(i64 [[#GUID]], i64 4, i32 0, i64 -1)
 }

clang/test/Driver/pseudo-probe.c

+// RUN: %clang -### -fpseudo-probe-for-profiling 2>&1 | FileCheck %s --check-prefix=YESPROBE
+// RUN: %clang -### -fno-pseudo-probe-for-profiling 2>&1 | FileCheck %s --check-prefix=NOPROBE
+// RUN: %clang -### -fpseudo-probe-for-profiling -fdebug-info-for-profiling 2>&1 | FileCheck %s --check-prefix=CONFLICT
+
+// YESPROBE: -fpseudo-probe-for-profiling
+// NOPROBE-NOT: -fpseudo-probe-for-profiling
+// CONFLICT: invalid argument