[PATCH] D136474: [CodeView][clang] Disable emitting command line into CodeView by default and add flag to enable

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks updated this revision to Diff 472351.
aeubanks added a comment.

on by default


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136474

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/debug-info-codeview-buildinfo.c
  clang/test/Driver/gcodeview-command-line.c

Index: clang/test/Driver/gcodeview-command-line.c
===
--- /dev/null
+++ clang/test/Driver/gcodeview-command-line.c
@@ -0,0 +1,19 @@
+// 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
Index: clang/test/CodeGen/debug-info-codeview-buildinfo.c
===
--- clang/test/CodeGen/debug-info-codeview-buildinfo.c
+++ clang/test/CodeGen/debug-info-codeview-buildinfo.c
@@ -1,8 +1,12 @@
 // 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; }
 
@@ -14,13 +18,21 @@
 // CHECK: 0x[[TOOL:.+]] | LF_STRING_ID [size = {{.+}}] ID: , String: [[TOOLVAL:.+[\\/]clang.*]]
 // CHECK: 0x[[CMDLINE:.+]] | LF_STRING_ID [size = {{.+}}] ID: , 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:  : ``
+// DISABLE-NEXT:  0x{{.+}}: `{{.*}}`
+// DISABLE-NEXT:  0x{{.+}}: ``
+// DISABLE-NEXT:  : ``
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -4546,8 +4546,10 @@
   }
 
   // Store the command-line for using in the CodeView backend.
-  Res.getCodeGenOpts().Argv0 = Argv0;
-  append_range(Res.getCodeGenOpts().CommandLineArgs, CommandLineArgs);
+  if (Res.getCodeGenOpts().CodeViewCommandLine) {
+Res.getCodeGenOpts().Argv0 = Argv0;
+append_range(Res.getCodeGenOpts().CommandLineArgs, CommandLineArgs);
+  }
 
   FixupInvocation(Res, Diags, Args, DashX);
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4315,9 +4315,14 @@
 
   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,
+   

[PATCH] D136474: [CodeView][clang] Disable emitting command line into CodeView by default and add flag to enable

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136474

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136474: [CodeView][clang] Disable emitting command line into CodeView by default and add flag to enable

[Tobias Hieta via Phabricator via cfe-commits]

thieta added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136474

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136474: [CodeView][clang] Disable emitting command line into CodeView by default and add flag to enable

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

> 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?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136474

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136474: [CodeView][clang] Disable emitting command line into CodeView by default and add flag to enable

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136474

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136474: [CodeView][clang] Disable emitting command line into CodeView by default and add flag to enable

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136474

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136474: [CodeView][clang] Disable emitting command line into CodeView by default and add flag to enable

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136474

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136474: [CodeView][clang] Disable emitting command line into CodeView by default and add flag to enable

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136474

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136474: [CodeView][clang] Disable emitting command line into CodeView by default and add flag to enable

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136474

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136474: [CodeView][clang] Disable emitting command line into CodeView by default and add flag to enable

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136474

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136474: [CodeView][clang] Disable emitting command line into CodeView by default and add flag to enable

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

> 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136474

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136474: [CodeView][clang] Disable emitting command line into CodeView by default and add flag to enable

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136474

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136474: [CodeView][clang] Disable emitting command line into CodeView by default and add flag to enable

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136474

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136474: [CodeView][clang] Disable emitting command line into CodeView by default and add flag to enable

[Tobias Hieta via Phabricator via cfe-commits]

thieta added a comment.

Also this should have a release note.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136474

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136474: [CodeView][clang] Disable emitting command line into CodeView by default and add flag to enable

[Tobias Hieta via Phabricator via cfe-commits]

thieta added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136474

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136474: [CodeView][clang] Disable emitting command line into CodeView by default and add flag to enable

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added subscribers: thieta, saudi.
aganea added a comment.

+@saudi @thieta


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136474

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits