[CodeView][clang] Add flag to disable emitting command line into CodeView

[aeubanks]

Summary

In https://reviews.llvm.org/D80833, there were concerns about
determinism emitting the commandline into CodeView. We're actually
hitting these when running clang-cl on Linux (cross compiling) versus on
Windows (e.g. -fmessage-length being inferred on terminals).

Add -g[no-]codeview-command-line to enable/disable this feature.
It's still on by default to preserve the current state of clang.

[MaskRay]

clang/lib/Driver/ToolChains/Clang.cpp

4319

This needs a test in clang/test/Driver/

Use addOptInFlag

[aganea]

Thanks for the patch @aeubanks and apologies for this oversight. I am wondering however if it wouldn’t make more sense to just strip this flag (-fmessage-length) from the emitted cmd-line? The goal is to provide a reproducer, this flag does not matter for that purpose.

[thakis]

There will be other problems like this. We don't care about having the command in the debug info and we'd rather get rid of this class of bugs Once And For All :)

[aeubanks]

add driver test

[aeubanks]

In D136474#3875442, @aganea wrote:

Thanks for the patch @aeubanks and apologies for this oversight. I am wondering however if it wouldn’t make more sense to just strip this flag (-fmessage-length) from the emitted cmd-line? The goal is to provide a reproducer, this flag does not matter for that purpose.

+1 to what @thakis said, and doing this can cause more cache hits with slightly different flags that don't affect codegen

clang/lib/Driver/ToolChains/Clang.cpp

4319

addOptIn/OutFlag doesn't work here with the semantics I want

For -cc1, -gcodeview-command-line should control whether or not we use this feature, defaulting to not performing this feature.
But on the driver level, not specifying anything should pass -gcodeview-command-line by default (assuming EmitCodeView).

But perhaps this is confusing and we should default to -gcodeview-command-line everywhere?

[aganea]

clang/lib/Driver/ToolChains/Clang.cpp

4319

Is there a reason for having divergent initial values? true below but -gcodeview-command-line being false? They should be the same. The other question is, MSVC emits LF_BUILDINFO by default (if /Z7 is on). Not sure if we should be compatible out-of-the-box or if we should favor good cache hits? I have no problem specifying the flag on the cmd-line if it isn't on by default.

+@stefan_reinalter

[aeubanks]

default to off

[aeubanks]

clang/lib/Driver/ToolChains/Clang.cpp

4319

the reason was to keep compatibility with msvc by emitting the command line by default, but I also wanted the -cc1 value to determine whether or not we actually did this

anyway, I've turned this feature off by default for now, open to reverting back to turning it on by default if people want that

[thakis]

Thanks!

Please mention the default value of the flag in the commit message.

[aganea]

+@saudi @thieta

[thieta]

We have tooling that uses this command line from the PDB. I think the conservative approach is to emit the command line by default since this is how MSVC behaves. I don't mind this flag now that I know about it, but I think we should keep the default to what MSVC does for clang-cl.

[thieta]

Also this should have a release note.

[thakis]

FWIW we do have precedent of deviating from cl.exe for determinism reasons. I think having deterministic output is a good default.

[rnk]

In D136474#3883629, @thakis wrote:

FWIW we do have precedent of deviating from cl.exe for determinism reasons. I think having deterministic output is a good default.

This is true, but the cc1 flag isn't inherently non-deterministic. I would prefer to fix the determinism bugs instead of adding flags and more divergence.

Can we use the grecord-command-line flag here instead of adding a new one?
https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-grecord-command-line
I understand it has different semantics. We'd record the command line differently depending on whether codeview or DWARF is in use, and potentially both ways when both are in use (🙄 ).

[thakis]

This is true, but the cc1 flag isn't inherently non-deterministic. I would prefer to fix the determinism bugs instead of adding flags and more divergence.

See https://reviews.llvm.org/D136474#3875546

[thakis]

(In other words, all known people who care about clang's determinism do not care about having the command recorded in the debug data -- and some (me :) ) mentioned concerns about determinism about the patch adding that feature. It seems weird to put those people on the hook to fix the bugs, then.)

[rnk]

In D136474#3884248, @thakis wrote:

This is true, but the cc1 flag isn't inherently non-deterministic. I would prefer to fix the determinism bugs instead of adding flags and more divergence.

See https://reviews.llvm.org/D136474#3875546

I read that, and I'm indicating that I don't agree. Chromium's requirements are driven by its particular usage of a cross-compiling distributed build system, which may not represent the average user's needs. Ubisoft's use case is also somewhat specialized, but IMO we really ought to write down the command line in the debug info. IMO it's useful. Sorry I don't have time to construct a more persuasive argument.

If you are still set on this course and don't want to try to address the determinism other ways, this flag seems fine to me, but I don't think it should be the default behavior. Does that provide a way forward?

[thakis]

I read that, and I'm indicating that I don't agree. Chromium's requirements are driven by its particular usage of a cross-compiling distributed build system, which may not represent the average user's needs.

Are you saying that it's a good thing if clang produces different output on different hosts?

If you are still set on this course and don't want to try to address the determinism other ways, this flag seems fine to me, but I don't think it should be the default behavior. Does that provide a way forward?

I'm fine with not toggling the default in this patch. I'm a fan of doing one thing per patch anyways.

[aganea]

In D136474#3888330, @thakis wrote:

I read that, and I'm indicating that I don't agree. Chromium's requirements are driven by its particular usage of a cross-compiling distributed build system, which may not represent the average user's needs.

Are you saying that it's a good thing if clang produces different output on different hosts?

Objectively speaking, flags that change the terminal output, or more generally don't affect the output artifact, shouldn't be part of the reproducer. It was an oversight on my end when I initially implemented the feature. Back at Ubisoft we had the same goal as you do, ie. build for a given platform and end up with the same results, regardless of the host. But that was never completed after I left. My view is that we should first add the flag to allow disabling the reproducer, and then in subsequent patches try to remove irrelevant flags from the reproducer.

[thakis]

In D136474#3888455, @aganea wrote:

In D136474#3888330, @thakis wrote:

I read that, and I'm indicating that I don't agree. Chromium's requirements are driven by its particular usage of a cross-compiling distributed build system, which may not represent the average user's needs.

Are you saying that it's a good thing if clang produces different output on different hosts?

Objectively speaking, flags that change the terminal output, or more generally don't affect the output artifact, shouldn't be part of the reproducer. It was an oversight on my end when I initially implemented the feature. Back at Ubisoft we had the same goal as you do, ie. build for a given platform and end up with the same results, regardless of the host. But that was never completed after I left. My view is that we should first add the flag to allow disabling the reproducer, and then in subsequent patches try to remove irrelevant flags from the reproducer.

That sounds good to me, assuming that you have someone concrete in mind for who "we" is who's removing the irrelevant flags :) But if nobody's in charge of that at the moment, imho it makes sense to make this default-off until it no longer regresses determinism.

[rnk]

In D136474#3888527, @thakis wrote:

That sounds good to me, assuming that you have someone concrete in mind for who "we" is who's removing the irrelevant flags :) But if nobody's in charge of that at the moment, imho it makes sense to make this default-off until it no longer regresses determinism.

That works for me, I just want to make it clear that the long term goal for the project is that we emit the command line in LF_BUILDINFO by default, rather than disabling it once and for all. If it's not ready yet, great, let's disable it and continue.

Over on Linux, this was a frequently requested feature, and you can see that we have -f and -g flags for it.

FWIW, we could probably move the terminal detection to the cc1 job. It inherits the TTY, right, so it can do the detection? I would look at how we handle fcolor-diagnostics here as reference, is that in the cc1 line or not?

[thakis]

That works for me, I just want to make it clear that the long term goal for the project is that we emit the command line in LF_BUILDINFO by default, rather than disabling it once and for all. If it's not ready yet, great, let's disable it and continue.

Fully on board with this :)

FWIW, we could probably move the terminal detection to the cc1 job. It inherits the TTY, right, so it can do the detection? I would look at how we handle fcolor-diagnostics here as reference, is that in the cc1 line or not?

[thieta]

I am fine with this plan as well. And I am willing to chip in and try to fix the issues with the current implementation since we actually use the feature.

@thakis do you mind explaining the issues you see and how to replicate them and I can have a look.

[thakis]

Just to be clear, in any case, this patch here is ready to go as long as it doesn't flip the default, yes?

@thakis do you mind explaining the issues you see and how to replicate them and I can have a look.

See above (-fmessage-length gets embedded and is machine-dependent).

[aeubanks]

on by default

[rnk]

I'm happy with this if it works for everyone else.

[thieta]

I posted https://reviews.llvm.org/D137322 to remove -fmessage-length

clang/include/clang/Basic/CodeGenOptions.def

 
 /// Whether we should emit CodeView debug information. It's possible to emit
 /// CodeView and DWARF into the same object.
 CODEGENOPT(EmitCodeView, 1, 0)
 
 /// Whether to emit the .debug$H section containing hashes of CodeView types.
 CODEGENOPT(CodeViewGHash, 1, 0)
 
+/// Whether to emit the compiler path and command line into the CodeView debug information.
+CODEGENOPT(CodeViewCommandLine, 1, 0)
+
 /// The kind of inlining to perform.
 ENUM_CODEGENOPT(Inlining, InliningMethod, 2, NormalInlining)
 
 /// The maximum stack size a function can have to be considered for inlining.
 VALUE_CODEGENOPT(InlineMaxStackSize, 32, UINT_MAX)
 
 // Vector functions library to use.
 ENUM_CODEGENOPT(VecLib, VectorLibrary, 3, NoLibrary)

clang/include/clang/Driver/Options.td

 def gcodeview : Flag<["-"], "gcodeview">,
   HelpText<"Generate CodeView debug information">,
   Flags<[CC1Option, CC1AsOption, CoreOption]>,
   MarshallingInfoFlag<CodeGenOpts<"EmitCodeView">>;
 defm codeview_ghash : BoolOption<"g", "codeview-ghash",
   CodeGenOpts<"CodeViewGHash">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Emit type record hashes in a .debug$H section">,
   NegFlag<SetFalse>, BothFlags<[CoreOption]>>;
+defm codeview_command_line : BoolOption<"g", "codeview-command-line",
+  CodeGenOpts<"CodeViewCommandLine">, DefaultTrue,
+  PosFlag<SetTrue, [], "Emit compiler path and command line into CodeView debug information">,
+  NegFlag<SetFalse, [], "Don't emit compiler path and command line into CodeView debug information">,
+  BothFlags<[CoreOption, CC1Option]>>;
 defm inline_line_tables : BoolGOption<"inline-line-tables",
   CodeGenOpts<"NoInlineLineTables">, DefaultFalse,
   NegFlag<SetTrue, [CC1Option], "Don't emit inline line tables.">,
   PosFlag<SetFalse>, BothFlags<[CoreOption]>>;
 
 def gfull : Flag<["-"], "gfull">, Group<g_Group>;
 def gused : Flag<["-"], "gused">, Group<g_Group>;
 def gstabs : Joined<["-"], "gstabs">, Group<g_Group>, Flags<[Unsupported]>;

clang/lib/Driver/ToolChains/Clang.cpp

else if (EffectiveDWARFVersion < 5)
           << A->getAsString(Args) << TC.getTripleString() << 5
           << EffectiveDWARFVersion;
     else if (checkDebugInfoOption(A, Args, D, TC))
       CmdArgs.push_back("-gembed-source");
   }
 
   if (EmitCodeView) {
     CmdArgs.push_back("-gcodeview");
+
     Args.addOptInFlag(CmdArgs, options::OPT_gcodeview_ghash,
                       options::OPT_gno_codeview_ghash);
+
+    Args.addOptOutFlag(CmdArgs, options::OPT_gcodeview_command_line,
+                       options::OPT_gno_codeview_command_line);
   }
+
   Args.addOptOutFlag(CmdArgs, options::OPT_ginline_line_tables,
                      options::OPT_gno_inline_line_tables);
 
   // When emitting remarks, we need at least debug lines in the output.
   if (willEmitRemarks(Args) &&
       DebugInfoKind <= codegenoptions::DebugDirectivesOnly)
     DebugInfoKind = codegenoptions::DebugLineTablesOnly;
 

clang/lib/Frontend/CompilerInvocation.cpp

bool CompilerInvocation::CreateFromArgsImpl(
   // If sanitizer is enabled, disable OPT_ffine_grained_bitfield_accesses.
   if (Res.getCodeGenOpts().FineGrainedBitfieldAccesses &&
       !Res.getLangOpts()->Sanitize.empty()) {
     Res.getCodeGenOpts().FineGrainedBitfieldAccesses = false;
     Diags.Report(diag::warn_drv_fine_grained_bitfield_accesses_ignored);
   }
 
   // Store the command-line for using in the CodeView backend.
+  if (Res.getCodeGenOpts().CodeViewCommandLine) {
     Res.getCodeGenOpts().Argv0 = Argv0;
     append_range(Res.getCodeGenOpts().CommandLineArgs, CommandLineArgs);
+  }
 
   FixupInvocation(Res, Diags, Args, DashX);
 
   return Diags.getNumErrors() == NumErrorsBefore;
 }
 
 bool CompilerInvocation::CreateFromArgs(CompilerInvocation &Invocation,
                                         ArrayRef<const char *> CommandLineArgs,

clang/test/CodeGen/debug-info-codeview-buildinfo.c

 // REQUIRES: x86-registered-target
 // RUN: %clang_cl --target=i686-windows-msvc /c /Z7 /Fo%t.obj -- %s
 // RUN: llvm-pdbutil dump --types %t.obj | FileCheck %s
+// RUN: %clang_cl -gcodeview-command-line --target=i686-windows-msvc /c /Z7 /Fo%t.obj -- %s
+// RUN: llvm-pdbutil dump --types %t.obj | FileCheck %s
 // RUN: %clang_cl --target=i686-windows-msvc /c /Z7 /Fo%t.obj -fdebug-compilation-dir=. -- %s
 // RUN: llvm-pdbutil dump --types %t.obj | FileCheck %s --check-prefix RELATIVE
+// RUN: %clang_cl -gno-codeview-command-line --target=i686-windows-msvc /c /Z7 /Fo%t.obj -- %s
+// RUN: llvm-pdbutil dump --types %t.obj | FileCheck %s --check-prefix DISABLE
 
 int main(void) { return 42; }
 
 // CHECK:                       Types (.debug$T)
 // CHECK: ============================================================
 // CHECK: 0x[[PWD:.+]] | LF_STRING_ID [size = {{.+}}] ID: <no type>, String: [[PWDVAL:.+]]
 // CHECK: 0x[[FILEPATH:.+]] | LF_STRING_ID [size = {{.+}}] ID: <no type>, String: [[FILEPATHVAL:.+[\\/]debug-info-codeview-buildinfo.c]]
 // CHECK: 0x[[ZIPDB:.+]] | LF_STRING_ID [size = {{.+}}] ID: <no type>, String:
 // CHECK: 0x[[TOOL:.+]] | LF_STRING_ID [size = {{.+}}] ID: <no type>, String: [[TOOLVAL:.+[\\/]clang.*]]
 // CHECK: 0x[[CMDLINE:.+]] | LF_STRING_ID [size = {{.+}}] ID: <no type>, String: "-cc1
 // CHECK: 0x{{.+}} | LF_BUILDINFO [size = {{.+}}]
-// CHECK:          0x[[PWD]]: `[[PWDVAL]]`
-// CHECK:          0x[[TOOL]]: `[[TOOLVAL]]`
-// CHECK:          0x[[FILEPATH]]: `[[FILEPATHVAL]]`
-// CHECK:          0x[[ZIPDB]]: ``
-// CHECK:          0x[[CMDLINE]]: `"-cc1
+// CHECK-NEXT:          0x[[PWD]]: `[[PWDVAL]]`
+// CHECK-NEXT:          0x[[TOOL]]: `[[TOOLVAL]]`
+// CHECK-NEXT:          0x[[FILEPATH]]: `[[FILEPATHVAL]]`
+// CHECK-NEXT:          0x[[ZIPDB]]: ``
+// CHECK-NEXT:          0x[[CMDLINE]]: `"-cc1
 
 // RELATIVE:                       Types (.debug$T)
 // RELATIVE: ============================================================
 // RELATIVE: 0x{{.+}} | LF_BUILDINFO [size = {{.+}}]
 // RELATIVE:          0x{{.+}}: `.`
+
+// DISABLE-NOT: cc1
+// DISABLE: 0x{{.+}} | LF_BUILDINFO [size = {{.+}}]
+// DISABLE-NEXT:          0x{{.+}}: `{{.*}}`
+// DISABLE-NEXT:          <no type>: ``
+// DISABLE-NEXT:          0x{{.+}}: `{{.*}}`
+// DISABLE-NEXT:          0x{{.+}}: ``
+// DISABLE-NEXT:          <no type>: ``

clang/test/Driver/gcodeview-command-line.c

+// Note: %s must be preceded by --, otherwise it may be interpreted as a
+// command-line option, e.g. on Mac where %s is commonly under /Users.
+
+// ON-NOT: "-gno-codview-commandline"
+// OFF: "-gno-codeview-command-line"
+
+// default
+// RUN: %clang_cl /Z7 -### -- %s 2>&1 | FileCheck -check-prefix=ON %s
+// enabled
+// RUN: %clang_cl /Z7 -gno-codeview-command-line -gcodeview-command-line -### -- %s 2>&1 | FileCheck -check-prefix=ON %s
+// disabled
+// RUN: %clang_cl /Z7 -gcodeview-command-line -gno-codeview-command-line -### -- %s 2>&1 | FileCheck -check-prefix=OFF %s
+
+// enabled, no /Z7
+// RUN: %clang_cl -gcodeview-command-line -### -- %s 2>&1 | FileCheck -check-prefix=ON %s
+
+// GCC-style driver
+// RUN: %clang -g -gcodeview -gno-codeview-command-line -gcodeview-command-line -### -- %s 2>&1 | FileCheck -check-prefix=ON %s
+// RUN: %clang -g -gcodeview -gcodeview-command-line -gno-codeview-command-line -### -- %s 2>&1 | FileCheck -check-prefix=OFF %s