Page MenuHomePhabricator

Record command lines in objects built by clang, Clang part
ClosedPublic

Authored by zhizhouy on Mar 8 2017, 2:48 PM.

Details

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.

Diff Detail

Event Timeline

zhizhouy created this revision.Mar 8 2017, 2:48 PM
zhizhouy edited reviewers, added: george.burgess.iv; removed: gbiv.Mar 8 2017, 2:54 PM
zhizhouy edited the summary of this revision. (Show Details)Mar 8 2017, 2:57 PM
zhizhouy removed rL LLVM as the repository for this revision.Mar 8 2017, 3:39 PM

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 updated this revision to Diff 92410.Mar 20 2017, 6:08 PM

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.

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 updated this revision to Diff 92546.Mar 21 2017, 2:31 PM
zhizhouy marked 3 inline comments as done.
zhizhouy edited the summary of this revision. (Show Details)
zhizhouy added a reviewer: dblaikie.

Fixed George's comments.

Still need to create a test for this diff.

echristo requested changes to this revision.Mar 21 2017, 2:35 PM

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

This revision now requires changes to proceed.Mar 21 2017, 2:35 PM

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?

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

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 updated this revision to Diff 92885.Mar 23 2017, 5:43 PM
zhizhouy edited edge metadata.
zhizhouy retitled this revision from Record command lines in objects built by clang to Record command lines in objects built by clang, Clang part.
zhizhouy edited the summary of this revision. (Show Details)
zhizhouy added a reviewer: aprantl.

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

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?

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

echristo requested changes to this revision.Mar 23 2017, 6:49 PM
This revision now requires changes to proceed.Mar 23 2017, 6:49 PM
aprantl added inline comments.Mar 24 2017, 9:25 AM
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 updated this revision to Diff 93003.Mar 24 2017, 1:57 PM
zhizhouy edited edge metadata.
zhizhouy marked 3 inline comments as done.

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

Modified testcases for it.

zhizhouy updated this revision to Diff 93011.Mar 24 2017, 2:25 PM

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

echristo added inline comments.Mar 24 2017, 2:54 PM
test/Driver/debug-options.c
207–208

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

test/Driver/debug-options.c
207

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 updated this revision to Diff 93401.Mar 29 2017, 1:05 PM
zhizhouy marked 2 inline comments as done.
zhizhouy edited the summary of this revision. (Show Details)

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 added inline comments.Mar 29 2017, 1:07 PM
test/Driver/debug-options.c
207–208

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

211

Typo.

zhizhouy updated this revision to Diff 93403.Mar 29 2017, 1:14 PM
zhizhouy marked 2 inline comments as done.

Added testcase for recording other useful options.

echristo accepted this revision.Mar 29 2017, 1:16 PM

Sounds good. Do you have commit access?

This revision is now accepted and ready to land.Mar 29 2017, 1:16 PM

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

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