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

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

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

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

lib/Driver/Tools.cpp

5394

Nit: please use .empty()

tools/driver/cc1_main.cpp

171

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

Can this be a StringRef instead?

232

Nit: Please prefer CmdLineOpts.empty()

tools/driver/driver.cpp

450

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

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

2725 ↗

(On Diff #92410)

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

2726 ↗

(On Diff #92410)

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

2728 ↗

(On Diff #92410)

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 ↗

(On Diff #92885)

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

include/clang/Basic/Diagnostic.h

private:
 
   /// \brief Optional flag value.
   ///
   /// Some flags accept values, for instance: -Wframe-larger-than=<value> and
   /// -Rpass=<value>. The content of this string is emitted after the flag name
   /// and '='.
   std::string FlagValue;
 
+  /// \brief Command line options user entered.
+  std::string CmdLineOpts;
+
 public:
   explicit DiagnosticsEngine(IntrusiveRefCntPtr<DiagnosticIDs> Diags,
                              DiagnosticOptions *DiagOpts,
                              DiagnosticConsumer *client = nullptr,
                              bool ShouldOwnClient = true);
   DiagnosticsEngine(const DiagnosticsEngine &) = delete;
   DiagnosticsEngine &operator=(const DiagnosticsEngine &) = delete;
   ~DiagnosticsEngine();
void SetDelayedDiagnostic(unsigned DiagID, StringRef Arg1 = "",
                             StringRef Arg2 = "");
   
   /// \brief Clear out the current diagnostic.
   void Clear() { CurDiagID = ~0U; }
 
   /// \brief Return the value associated with this diagnostic flag.
   StringRef getFlagValue() const { return FlagValue; }
 
+  /// \brief Set and get command line options as a string.
+  void setCmdLineOpts(std::string &opts) { CmdLineOpts = opts; }
+
+  std::string getCmdLineOpts() const { return CmdLineOpts; }
+
 private:
   /// \brief Report the delayed diagnostic.
   void ReportDelayed();
 
   // This is private state used by DiagnosticBuilder.  We put it here instead of
   // in DiagnosticBuilder in order to keep DiagnosticBuilder a small lightweight
   // object.  This implementation choice means that we can only have one
   // diagnostic "in flight" at a time, but this seems to be a reasonable

lib/CodeGen/CGDebugInfo.cpp

if (LO.CPlusPlus) {
     LangTag = llvm::dwarf::DW_LANG_GOOGLE_RenderScript;
   } else if (LO.C99) {
     LangTag = llvm::dwarf::DW_LANG_C99;
   } else {
     LangTag = llvm::dwarf::DW_LANG_C89;
   }
 
   std::string Producer = getClangFullVersion();
+  Producer += CGM.getDiags().getCmdLineOpts();
 
   // Figure out which version of the ObjC runtime we have.
   unsigned RuntimeVers = 0;
   if (LO.ObjC1)
     RuntimeVers = LO.ObjCRuntime.isNonFragile() ? 2 : 1;
 
   llvm::DICompileUnit::DebugEmissionKind EmissionKind;
   switch (DebugKind) {

lib/Driver/Tools.cpp

#endif
   if (WholeProgramVTables) {
     if (!D.isUsingLTO())
       D.Diag(diag::err_drv_argument_only_allowed_with)
           << "-fwhole-program-vtables"
           << "-flto";
     CmdArgs.push_back("-fwhole-program-vtables");
   }
 
+  if (D.getDiags().getCmdLineOpts() != "") {
+    CmdArgs.push_back("-record-cmd-opts");
+    CmdArgs.push_back(D.getDiags().getCmdLineOpts().c_str());
+  }
+
   // Finally add the compile command to the compilation.
   if (Args.hasArg(options::OPT__SLASH_fallback) &&
       Output.getType() == types::TY_Object &&
       (InputType == types::TY_C || InputType == types::TY_CXX)) {
     auto CLCommand =
         getCLFallback()->GetCommand(C, JA, Output, Inputs, Args, LinkingOutput);
     C.addCommand(llvm::make_unique<FallbackCommand>(
         JA, *this, Exec, CmdArgs, Inputs, std::move(CLCommand)));

tools/driver/cc1_main.cpp

static void ensureSufficientStack() {
   // that we can actually use that much, if necessary.
   ensureStackAddressSpace();
 }
 #else
 static void ensureSufficientStack() {}
 #endif
 
 int cc1_main(ArrayRef<const char *> Argv, const char *Argv0, void *MainAddr) {
+  // Pop out -record-cmd-opts from Argv.
+  // If the Argv is generated from driver, it will contain command line options
+  // as the last two arguments, record and remove them from current Argv.
+  std::string CmdLineOpts = "";
+  if (Argv.size() > 2 &&
+      strcmp(Argv[Argv.size() - 2], "-record-cmd-opts") == 0) {
+    CmdLineOpts = Argv[Argv.size() - 1];
+    Argv = Argv.drop_back(2);
+  }
+
   ensureSufficientStack();
 
   std::unique_ptr<CompilerInstance> Clang(new CompilerInstance());
   IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
 
   // Register the support for object-file-wrapped Clang modules.
   auto PCHOps = Clang->getPCHContainerOperations();
   PCHOps->registerWriter(llvm::make_unique<ObjectFilePCHContainerWriter>());
#endif
   // error handler.
   llvm::install_fatal_error_handler(LLVMErrorHandler,
                                   static_cast<void*>(&Clang->getDiagnostics()));
 
   DiagsBuffer->FlushDiagnostics(Clang->getDiagnostics());
   if (!Success)
     return 1;
 
+  // Set CmdLineOpts in Diagnostics Enginer, so that it can be used by
+  // DW_AT_producer in CGDebugInfo.
+  if (CmdLineOpts != "")
+    Clang->getDiagnostics().setCmdLineOpts(CmdLineOpts);
+
   // Execute the frontend actions.
   Success = ExecuteCompilerInvocation(Clang.get());
 
   // If any timers were active but haven't been destroyed yet, print their
   // results now.  This happens in -disable-free mode.
   llvm::TimerGroup::printAll(llvm::errs());
 
   // Our error handler depends on the Diagnostics object, which we're

tools/driver/driver.cpp

auto SerializedConsumer =
         clang::serialized_diags::create(DiagOpts->DiagnosticSerializationFile,
                                         &*DiagOpts, /*MergeChildRecords=*/true);
     Diags.setClient(new ChainedDiagnosticConsumer(
         Diags.takeClient(), std::move(SerializedConsumer)));
   }
 
   ProcessWarningOptions(Diags, *DiagOpts, /*ReportDiags=*/false);
 
+  // Set command line options before building the Driver.
+  std::string CmdLineOpts = "";
+  for (const auto arg : argv) {
+    CmdLineOpts += " " + std::string(arg);
+  }
+  Diags.setCmdLineOpts(CmdLineOpts);
+
   Driver TheDriver(Path, llvm::sys::getDefaultTargetTriple(), Diags);
   SetInstallDir(argv, TheDriver, CanonicalPrefixes);
 
   insertTargetAndModeArgs(TargetAndMode.first, TargetAndMode.second, argv,
                           SavedStrings);
 
   SetBackdoorDriverOutputsFromEnvVars(TheDriver);