This is an archive of the discontinued LLVM Phabricator instance.

Add an option to support debug fission on implicit ThinLTO.
ClosedPublic

Authored by yunlian on Mar 22 2018, 9:41 AM.

Details

Summary

This adds an option -gsplit-dwarf=<arg>. LLVM can create .dwo files in the given directory
during the implicit ThinLTO link stage.

Diff Detail

Event Timeline

yunlian created this revision.Mar 22 2018, 9:41 AM
pcc added a comment.Mar 22 2018, 10:23 AM

Does this depend on another patch?

I have another one D44792 on LLVM side.

I don't think requiring a new option is a great user interface. In existing use cases the location of the .dwo file matches the location of the output file. Why is this one different?

In implicit ThinLTO, the object files are only temporary.

Sort of similar to using -gsplit-dwarf when compiling straight to an
executable (without using -c): "clang++ x.cpp y.cpp -o a.out" - where
should the .dwo files go then? If they go where the .o files go, then
they'll be in /tmp/ and get deleted either when the ocmpiler ends after it
runs the linker, or perhaps at some uncertain point in the future when the
temp space is reclaimed.
(granted, I'm not suggesting we support that actual case - it's not
terribly common for anyone who'd actually need -gsplit-dwarf - but the
implicit ThinLTO case looks quite similar for demonstration purposes)

In implicit ThinLTO, the object files are only temporary.

Sort of similar to using -gsplit-dwarf when compiling straight to an
executable (without using -c): "clang++ x.cpp y.cpp -o a.out" - where
should the .dwo files go then? If they go where the .o files go, then
they'll be in /tmp/ and get deleted either when the ocmpiler ends after it
runs the linker, or perhaps at some uncertain point in the future when the
temp space is reclaimed.

I think that the .dwo files generally go where the user-specified final output goes. So in your example they would go where a.out goes, not where the intermediate/temporary .o files go.
Being able to override that is fine, but being required to specify a directory in order to get fission in the first place is not.

The only data we have is that where .o files go, .dwo files go beside them.
How to generalize this to other situations isn't really obvious to me -
even for a.out (do you put all the .dwo files next to a.out? in the same
directory? if the names collide, where then? etc).

Interestingly GCC for "g++ foo.cpp" puts the foo.dwo file right where it
would go by default (next to foo.cpp, even though there's no foo.o). Oh,
LLVM does that too. Huh. I'm not sure that's a terribly helpful default to
extrapolate to ThinLTO for, though.

yunlian updated this revision to Diff 139956.Mar 27 2018, 10:26 AM
yunlian updated this revision to Diff 140857.Apr 3 2018, 1:50 PM

I prefer to have a dedicated directory to store all the .dwo files. As dblaikie said, all the .dwo files are temporary files. In addition, in order to differentiate the .dwo files generated by different link stage with the same .o, we need to add
some suffixes to the .dwo file. So for a file a.o, we may need to generate a .dwo named a._${number}_${random_string}.dwo, which is not neat. Furthermore, we need to dealing with archive files. In this case, we may need to generate
multiple .dwo files for a single archive files.

pcc added inline comments.Apr 30 2018, 10:24 AM
lib/Driver/ToolChains/CommonArgs.cpp
444

Can we default this to a path alongside the output file as @probinson suggested?

yunlian updated this revision to Diff 146698.May 14 2018, 3:03 PM
yunlian edited the summary of this revision. (Show Details)

Use the option -gsplit-dwarf to make it more consistent.

yunlian marked an inline comment as done.Jun 5 2018, 10:34 AM

ping?

pcc added inline comments.Jun 5 2018, 11:45 AM
lib/Driver/ToolChains/CommonArgs.cpp
454

You no longer need to pass objcopy=, see D47091.

yunlian updated this revision to Diff 150028.Jun 5 2018, 12:48 PM
yunlian updated this revision to Diff 150419.Jun 7 2018, 3:35 PM

Makes the default value to /path/to/binary_dwo, when DWO_DIR is provided, make the path to DWO_DIR/path/to/binary_dwo

pcc added inline comments.Jun 18 2018, 3:54 PM
lib/Driver/ToolChains/CommonArgs.cpp
423

I think that if I pass -gsplit-dwarf=/path/to/foo I would expect the dwo directory to be named /path/to/foo, not /path/to/foo/something_dwo.

428

If you make this else if (Args.hasArg(options::OPT_gsplit_dwarf)) { you wouldn't need the if on line 429.

yunlian added inline comments.Jun 20 2018, 9:58 AM
lib/Driver/ToolChains/CommonArgs.cpp
428

Will do.

yunlian updated this revision to Diff 152120.Jun 20 2018, 11:25 AM

Removed redundant 'if' statement

yunlian marked 2 inline comments as done.Jun 25 2018, 3:18 PM

ping?

yunlian updated this revision to Diff 152794.Jun 25 2018, 3:47 PM
pcc accepted this revision.Jun 25 2018, 3:55 PM

LGTM

This revision is now accepted and ready to land.Jun 25 2018, 3:55 PM
This revision was automatically updated to reflect the committed changes.