This is an archive of the discontinued LLVM Phabricator instance.

Support option -plugin-opt=dwo_dir=
ClosedPublic

Authored by yunlian on Jun 7 2018, 1:24 PM.

Details

Summary

This adds support to option -plugin-opt=dwo_dir=${DIR}. This option is used to specify the directory to store the .dwo files when LTO and debug fission is used
at the same time.

Diff Detail

Event Timeline

yunlian created this revision.Jun 7 2018, 1:24 PM
yunlian added a reviewer: pcc.Jun 8 2018, 10:10 AM
pcc added a comment.Jun 20 2018, 11:28 AM

Can you add a test case?

ELF/Options.td
452

nit: Directory

yunlian updated this revision to Diff 152148.Jun 20 2018, 1:34 PM
pcc added a comment.Jun 25 2018, 5:15 PM

Looks like you updated this review with the wrong patch.

ruiu added inline comments.Jun 25 2018, 10:18 PM
ELF/Options.td
451

sort

yunlian updated this revision to Diff 152915.Jun 26 2018, 10:17 AM

Upload the right patch, fixed the option order in Options.td

pcc added inline comments.Jun 26 2018, 10:27 AM
ELF/Options.td
437

The new option should appear before this line I think.

447

nit: Directory

pcc added a comment.Jun 26 2018, 10:29 AM

Oh and there's still no test.

yunlian updated this revision to Diff 155025.Jul 11 2018, 9:58 AM

I will upload a test later.

yunlian updated this revision to Diff 155198.Jul 12 2018, 9:34 AM

unittest added

ruiu added inline comments.Jul 12 2018, 9:45 AM
test/ELF/lto/thinlto-debug-fission.ll
3 ↗(On Diff #155198)

Remove.

4 ↗(On Diff #155198)

Double space characters.

7 ↗(On Diff #155198)

If the specified directory doesn't exist, does --plug-opt=dwo_dir creates a new directory?

7 ↗(On Diff #155198)

-o /dev/null is preferred if you don't need an output file.

yunlian marked 3 inline comments as done.Jul 12 2018, 10:07 AM
yunlian added inline comments.
test/ELF/lto/thinlto-debug-fission.ll
7 ↗(On Diff #155198)

Yes, it will create a new directory if the specified directory does not exist.

yunlian updated this revision to Diff 155208.Jul 12 2018, 10:08 AM
pcc accepted this revision.Jul 16 2018, 10:07 AM

LGTM

test/ELF/lto/thinlto-debug-fission.ll
7 ↗(On Diff #155198)

I'd add an rm -rf %T/dwo to test that and make sure that files aren't left over from previous runs.

This revision is now accepted and ready to land.Jul 16 2018, 10:07 AM
yunlian updated this revision to Diff 155718.Jul 16 2018, 10:54 AM

removed temporary file generated by the test.

pcc added inline comments.Jul 16 2018, 10:55 AM
test/ELF/lto/thinlto-debug-fission.ll
7 ↗(On Diff #155198)

Move the rm -rf before the call to ld.lld.

This revision was automatically updated to reflect the committed changes.
pcc added inline comments.Jul 16 2018, 11:05 AM
test/ELF/lto/thinlto-debug-fission.ll
7 ↗(On Diff #155198)

Move the rm -rf before the call to ld.lld.

Looks like this wasn't resolved before committing.

I added the 'rm' after the test. Will upload another patch to fix that.