Record command lines in objects built by clang, Clang part

[zhizhouy]

Summary

This is a chromium related bug, referred to: Link

I created this bug and updated the info on bugzilla, please refer to: Link

GCC provides -grecord-gcc-switches option, which is set on as default, to record user command line options and store them DW_AT_producer in DWARF file.
While DW_AT_producer in clang only records the full version info of current clang.

The implementation enabled the -grecord-gcc-switches option. When user turn it on, driver will record the arguments from command line and reuse CC1Option
"-dwarf-debug-flags".

Finally the CGDebugInfo could access the CodeGenOptions of "-dwarf-debug-flags" from and set Producer to clang full version plus command line options.

[george.burgess.iv]

Thanks for the patch! Since the description says this is unfinished, I'm assuming you'll add tests for this behavior in a more "finished" version.

Once things are a bit more ironed out, I'll try to grab someone who can properly review this (since I'm not familiar with clang's parser-y bits, I'm not comfortable LGTMing this).

include/clang/Basic/Diagnostic.h

394 ↗

(On Diff #91075)

I'm not incredibly familiar with these bits of clang, but if our goal is to get this information to CodeGen, should this instead be put in something more CodeGen-y?

773 ↗

(On Diff #91075)

Nit: please use a StringRef or a std::string that you move into CmdLineOpts instead of a std::string &. Variable names should be capitalized, like Opts

775 ↗

(On Diff #91075)

Nit: Please return a StringRef here. (if that's not possible, please use a const string & instead)

lib/Driver/Tools.cpp

5394 ↗

(On Diff #91075)

Nit: please use .empty()

tools/driver/cc1_main.cpp

171 ↗

(On Diff #91075)

Assuming CodeGen is the right place to try and store this string, it looks like ParseCodeGenArgs in lib/Frontend/CompilerInvocation.cpp might be a better place to try and parse this from the cc1 args.

(Also, we should probably create an option in include/clang/Driver/CC1Options.td instead of trying to do the string matching ourselves)

174 ↗

(On Diff #91075)

Can this be a StringRef instead?

232 ↗

(On Diff #91075)

Nit: Please prefer CmdLineOpts.empty()

tools/driver/driver.cpp

450 ↗

(On Diff #91075)

Style nits:

• Please use const auto * to indicate that this is a pointer (assuming it's a const char *)
• Please use Arg instead of arg
• Please remove the braces, since the body of this is a single one-line statement.

451 ↗

(On Diff #91075)

Starting an this argument with a ' ' and relying on that in CodeGen seems a bit subtle to me. Can we drop the first space here and add it in CodeGen instead, please?

[zhizhouy]

Re-implemented the the patch: reusing -grecord-gcc-switches from gcc instead of passing arguments to DiagnosticsEngine.

If user specify the -grecord-gcc-switches in command line, Clang will start to record it in Driver, and create a CC1Option of "-record-cmd-opts" and record it in CodeGenOptions.

Producer will be updated directly from the CodeGenOptions.

[george.burgess.iv]

This is looking pretty good! Aside from a few nits, I'm happy with this after we add tests.

lib/Driver/ToolChains/Clang.cpp

2733

Nit: can we use a SmallString<256> that we initialize to "-record-cmd-opts=" instead?

2734

Style nit: Loop bodies should go on a new line. If you use clang-format, it should handle all of these issues for you. :)

2736

Nit: since we're using a thing directly convertible to StringRef, can we call MakeArgStringRef here instead?

[zhizhouy]

Fixed George's comments.

Still need to create a test for this diff.

[echristo]

Why aren't we passing the flags down as a string in the IR?

Needs a testcase: you should only check for IR in clang tests. To make sure something is propagated to the other end use an IR test and then use llvm-dwarfdump to inspect as an llvm test.

-eric

[aprantl]

Sorry for being late to the party, but have you looked at CodeGenOptions::DwarfDebugFlags? It looks like it almost does what you want, but it is currently only enabled for MachO and when a specific environment variable is set. Would it make sense to generalize that mechanism instead?

[zhizhouy]

Hi aprantl, thanks for replying. I checked the usage of DwarfDebugFlags, it seems that it really did the same work of recording command line options.

And I noticed that it is set to false by default. Is it because of some concerns like the debug info size?

Is it proper to just set it to true when -grecord-gcc-switches exists on whatever platform?

Thanks,
Zhizhou

[aprantl]

In D30760#707178, @zhizhouy wrote:

Hi aprantl, thanks for replying. I checked the usage of DwarfDebugFlags, it seems that it really did the same work of recording command line options.

And I noticed that it is set to false by default. Is it because of some concerns like the debug info size?

I think it is off by default because it doesn't really help the debugger and is more useful as a informational tool (e.g., when investigating compiler bugs). It also might leak unexpected information into the build (paths, macros, etc, ...) that a user might want to have control over.

Is it proper to just set it to true when -grecord-gcc-switches exists on whatever platform?

I think so.

thanks,
adrian

[zhizhouy]

I am reusing the DwarfDebugFlags to try to solve this bug now.

DwarfDebugFlags only records the command line options for Apple Darwin and will generate DW_AT_APPLE_flags into debug info.
I decide not to touch those parts for now.

When -grecord-gcc-switches is set, I record command line options into DwarfDebugFlags no matter what ToolChain currently is.

This patch also modified LLVM side, which can be referred to: name

[george.burgess.iv]

lib/Driver/ToolChains/Clang.cpp

4328

looks like we have to consider -gno-record-gcc-switches (which already exists in Options.td), too. hasFlag should let you test them both easily.

please also add a test checking that we respect -gno-record-gcc-switches.

test/CodeGen/debug-info-grecord-gcc-switches.c

6 ↗

(On Diff #92885)

nit: since RenderAsInput args (-Xlinker, -o, ...) have cases where they're printed without their arg, can we check that -o - is printed sanely here, as well?

[echristo]

Needs more testing. Might want to make sure that you actually are recording some useful command line options and that you're looking at the cc1 command line.

This should also be a Driver test and not CodeGen. You can then use -### to inspect the command lines as passed around.

-eric

[aprantl]

test/CodeGen/debug-info-grecord-gcc-switches.c

1 ↗

(On Diff #92885)

Shouldn't this test be in the Driver/ directory? There is already a big file that exercises all -g options, I would just append it there.

[zhizhouy]

Checked both grecord-gcc-swtiches and gno-record-gcc-switches.

Modified testcases for it.

[zhizhouy]

Added tests into test/Driver/debug-options.c. Thanks.

[echristo]

test/Driver/debug-options.c

201–202 ↗

(On Diff #93011)

This seems a little light on the testing, would you mind adding some more interesting lines here? (Also, -grecord-gcc-switches seems like an option we might want to ignore for this?)

[george.burgess.iv]

test/Driver/debug-options.c

201 ↗

(On Diff #93011)

re-pasting my comment, since it looks like it got dropped when we updated diffs:

nit: since RenderAsInput args (-Xlinker, -o, ...) have cases where they're printed without their arg, can we check that at least -o - is printed sanely, as well?

[zhizhouy]

Added two more testcases, one is options with both grecord-gcc-switches and gno-record-gcc-switches; the other one is testing if "-o -" is omitted correctly.

[echristo]

test/Driver/debug-options.c

209 ↗

(On Diff #93401)

Typo.

201–202 ↗

(On Diff #93011)

How about adding things like "-O3" or "-ffunction-sections" or something? Just useful options that we'd expect to see.

[zhizhouy]

Added testcase for recording other useful options.

[echristo]

Sounds good. Do you have commit access?

[echristo]

Committed thusly:
echristo@athyra ~/s/l/t/clang> git svn dcommit
Committing to https://llvm.org/svn/llvm-project/cfe/trunk ...
M lib/Driver/ToolChains/Clang.cpp
M test/Driver/debug-options.c
Committed r299037

[george.burgess.iv]

(Committed as noted by echristo; just trying to clean my queue a bit. :) )

lib/Driver/ToolChains/Clang.cpp

#endif
   if (Args.hasArg(options::OPT_gcodeview) || EmitCodeView) {
     // DwarfVersion remains at 0 if no explicit choice was made.
     CmdArgs.push_back("-gcodeview");
   } else if (DwarfVersion == 0 &&
              DebugInfoKind != codegenoptions::NoDebugInfo) {
     DwarfVersion = getToolChain().GetDefaultDwarfVersion();
   }
 
-  // We ignore flags -gstrict-dwarf and -grecord-gcc-switches for now.
+  // We ignore flag -gstrict-dwarf for now.
+  // And we handle flag -grecord-gcc-switches later with DwarfDebugFlags.
   Args.ClaimAllArgs(options::OPT_g_flags_Group);
 
   // Column info is included by default for everything except PS4 and CodeView.
   // Clang doesn't track end columns, just starting columns, which, in theory,
   // is fine for CodeView (and PDB).  In practice, however, the Microsoft
   // debuggers don't handle missing end columns well, so it's better not to
   // include any column info.
   if (Args.hasFlag(options::OPT_gcolumn_info, options::OPT_gno_column_info,
                    /*Default=*/ !IsPS4CPU && !(IsWindowsMSVC && EmitCodeView)))
     CmdArgs.push_back("-dwarf-column-info");
 
   // FIXME: Move backend command line options to the module.
else
     Input.getInputArg().renderAsInput(Args, CmdArgs);
 
   Args.AddAllArgs(CmdArgs, options::OPT_undef);
 
   const char *Exec = getToolChain().getDriver().getClangProgramPath();
 
   // Optionally embed the -cc1 level arguments into the debug info, for build
   // analysis.
-  if (getToolChain().UseDwarfDebugFlags()) {
+  // Also record command line arguments into the debug info if
+  // -grecord-gcc-switches options is set on.
+  // By default, -gno-record-gcc-switches is set on and no recording.
+  if (getToolChain().UseDwarfDebugFlags() ||
+      Args.hasFlag(options::OPT_grecord_gcc_switches,
+                   options::OPT_gno_record_gcc_switches, false)) {
     ArgStringList OriginalArgs;
     for (const auto &Arg : Args)
       Arg->render(Args, OriginalArgs);
 
     SmallString<256> Flags;
     Flags += Exec;
     for (const char *OriginalArg : OriginalArgs) {
       SmallString<128> EscapedArg;

test/Driver/grecord-gcc-switches.c

+// RUN: %clang -### -g -c -grecord-gcc-switches %s 2>&1 | FileCheck -check-prefix=REC %s
+// RUN: %clang -### -g -c -gno-record-gcc-switches %s 2>&1 | FileCheck -check-prefix=NO_REC %s
+// RUN: %clang -### -g -c %s 2>&1 | FileCheck %s
+int main (void) {
+  return 0;
+}
+
+// REC: "-dwarf-debug-flags"
+// REC: -### -g -c -grecord-gcc-switches
+// NO_REC-NOT: "-dwarf-debug-flags"
+// NO_REC-NOT: -### -g -c -gno-record-gcc-switches
+// CHECK-NOT: "-dwarf-debug-flags"
+// CHECK-NOT: -### -g -c