This is an archive of the discontinued LLVM Phabricator instance.

Enable debug fission for thinLTO linked via gold-plugin
ClosedPublic

Authored by yunlian on Mar 22 2018, 10:29 AM.

Details

Summary

This enables debug fission on implicit ThinLTO when linked with gold. It will put the .dwo files in a directory specified by user.

Diff Detail

Event Timeline

yunlian created this revision.Mar 22 2018, 10:29 AM
pcc added a comment.Mar 22 2018, 10:46 AM

Please upload your patch with context.

include/llvm/LTO/Config.h
77

Rename to match coding conventions: DwoDir.

include/llvm/LTO/LTO.h
169 ↗(On Diff #139468)

Remove this field.

lib/LTO/LTOBackend.cpp
439

The module identifier is not enough to uniquely identify a .dwo file, as there could be multiple inputs with the same identifier. It could also contain slashes (which would require creating parent directories) or other characters which are invalid to use in file names. Probably the simplest thing to do is use the task identifier.

488

Move all of this logic into codegen; that way, you don't need to pass a file name back. If fission is enabled we should call addPassesToEmitFile on a temporary file, and then have objcopy operate on that file and finally open it and write its contents to Stream->OS.

yunlian added inline comments.Mar 22 2018, 11:41 AM
lib/LTO/LTOBackend.cpp
439

I think it should be good enough in normal cases.
Below are some example identifier when I link chrome.

./obj/device/bluetooth/public/mojom/mojom/uuid.mojom.o
./obj/device/bluetooth/libbluetooth.a.llvm.19039330.bluetooth_adapter_client.cc

For .o, it is the name (path included) of the input bitecode. For archives, it is the name of the archive appended by offset and the source file name.

The only thing that it does not work is that, the same input files are shared by different binaries and the binaries are linked in the same time.

The module name implies that we may need to create sub directories to store the .dwo files. I think that should be fine.

pcc added inline comments.Mar 22 2018, 11:46 AM
lib/LTO/LTOBackend.cpp
439

I think it should be good enough in normal cases.

Yes, but the task identifier is good enough in all cases, and it requires less code.

yunlian updated this revision to Diff 139607.Mar 23 2018, 10:18 AM
yunlian marked 2 inline comments as done.Mar 23 2018, 10:21 AM
yunlian added inline comments.
lib/LTO/LTOBackend.cpp
439

I use the module identifier without directory information and the Task as the .dwo file name.

pcc added a comment.Mar 23 2018, 12:14 PM

A reminder to add context (https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

Test cases please. Also I think you will need to add the dwo path and the task id to the cache key if split dwarf is enabled.

include/llvm/LTO/Config.h
76

Drop the "when using ThinLTO" part because this should also work with regular LTO.

lib/LTO/LTOBackend.cpp
292

if (auto EC = sys::...

305

Indentation

308

Do you need this line?

310

Remove braces and if (auto EC = sys::...

311

Include EC.message() in error.

323–324

Add error handling.

327

Add error handling.

yunlian updated this revision to Diff 139665.Mar 23 2018, 4:18 PM
yunlian marked 8 inline comments as done.
yunlian updated this revision to Diff 139669.Mar 23 2018, 4:30 PM
pcc added a comment.Mar 23 2018, 5:02 PM
In D44792#1047010, @pcc wrote:

A reminder to add context (https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

Test cases please. Also I think you will need to add the dwo path and the task id to the cache key if split dwarf is enabled.

Please address.

lib/LTO/LTOBackend.cpp
294

This can just be llvm::raw_fd_ostream OS(FD, true); because you aren't transferring the ownership anywhere.

328

I wouldn't bother with the arguments, I'd probably say something like Failed to extract dwo: exit code foo. Same below.

yunlian updated this revision to Diff 139864.Mar 26 2018, 4:31 PM
yunlian marked 2 inline comments as done.
yunlian updated this revision to Diff 140091.Mar 28 2018, 9:22 AM
In D44792#1047010, @pcc wrote:

A reminder to add context (https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

Test cases please. Also I think you will need to add the dwo path and the task id to the cache key if split dwarf is enabled.

I added the dwo dir to cache key.
About the test cases, I am not sure how to do that in my case.
Ideally, it should build a binary and I can check the output of readelf.
But this needs clang. Can we call clang in a unittest of llvm?

yunlian updated this revision to Diff 140147.Mar 28 2018, 3:29 PM
pcc added a comment.Mar 28 2018, 3:32 PM
In D44792#1047010, @pcc wrote:

A reminder to add context (https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

Test cases please. Also I think you will need to add the dwo path and the task id to the cache key if split dwarf is enabled.

I added the dwo dir to cache key.
About the test cases, I am not sure how to do that in my case.
Ideally, it should build a binary and I can check the output of readelf.
But this needs clang. Can we call clang in a unittest of llvm?

You shouldn't need clang to test ThinLTO. See for example any of the tests in test/tools/gold/X86/thinlto*.ll.

yunlian updated this revision to Diff 140270.Mar 29 2018, 9:11 AM

You shouldn't need clang to test ThinLTO. See for example any of the tests in test/tools/gold/X86/thinlto*.ll.

Thanks, It seems I can to pass -shared to gold to avoid invoking clang. A test case was uploaded.

yunlian updated this revision to Diff 140854.Apr 3 2018, 1:39 PM
pcc added inline comments.Apr 3 2018, 2:05 PM
lib/LTO/LTO.cpp
139

You don't need the if condition here.

test/tools/gold/X86/split-dwarf.ll
2

I don't think this is necessary, as long as you use LLVM tools.

4

Prefer to specify absolute paths to commands to avoid depending on the current directory.

8

We generally don't depend on GNU binutils in our tests. You probably want to pass the path to the just-built llvm-objcopy here.

10

This test should start with textual IR instead of a .bc file. Use the opt command to create a .bc file from your test input and then pass that to gold.

16

Similarly, don't depend on binutils here. Can this be tested with llvm-dwarfdump?

yunlian added inline comments.Apr 3 2018, 4:11 PM
lib/LTO/LTO.cpp
139

Will do

test/tools/gold/X86/split-dwarf.ll
2

The reason I want to exclude windows platform is I want to use 'which' command line, if I can remove that, I will remove this line too.

8

How can I get the path of the llvm-objcopy? I can run llvm-* command line inside llvm-lit without specifying the path. But this is one is a little bit tricky, it needs the full path of the objcopy.

10

If I use llvm-as or opt to create a .bc file from .ll and do the same test, the .dwo file name changes from split-dwarf.dwo to ld-temp.dwo. Is that expected?

16

llvm-dwarfdump works here.
Will do.

pcc added inline comments.Apr 3 2018, 4:21 PM
test/tools/gold/X86/split-dwarf.ll
8

Create a ToolSubst for %llvm-objcopy in llvm/test/lit.cfg.py and use it here.

10

Your .bc files don't have a summary, so they are being compiled with full LTO instead of ThinLTO. Pass -module-summary to opt to get ThinLTO object files.

Come to think of it, we probably want to test both full and ThinLTO. Can you construct a test with one full LTO input file and one ThinLTO?

yunlian added inline comments.Apr 4 2018, 9:46 AM
test/tools/gold/X86/split-dwarf.ll
4

It seems that %t is an absolute path already.

pcc added inline comments.Apr 4 2018, 10:06 AM
test/tools/gold/X86/split-dwarf.ll
4

Yes but you are passing a relative path as dwo_dir. Pass %t/dwo_dir instead and remove this line.

yunlian updated this revision to Diff 140988.Apr 4 2018, 10:17 AM
yunlian updated this revision to Diff 140999.Apr 4 2018, 11:02 AM
pcc added inline comments.Apr 4 2018, 6:14 PM
test/lit.cfg.py
137

%llvm-objcopy (and elsewhere)

Can you use FindTool here?

yunlian updated this revision to Diff 141672.Apr 9 2018, 9:37 AM
yunlian marked an inline comment as done.Apr 10 2018, 8:40 AM
pcc added inline comments.Apr 10 2018, 10:03 AM
test/lit.cfg.py
137

Looks like the %llvm-objcopy part isn't done.

yunlian added inline comments.Apr 10 2018, 10:15 AM
test/lit.cfg.py
137

I did not use %llvm-objcopy, I used %objcopy in my test instead. Do I still need that?

pcc added inline comments.Apr 10 2018, 3:41 PM
test/lit.cfg.py
137

Yes because we want to be clear that this is llvm-objcopy and not some other objcopy.

yunlian updated this revision to Diff 142201.Apr 12 2018, 9:20 AM
yunlian marked an inline comment as done.
pcc accepted this revision.Apr 12 2018, 1:31 PM

LGTM

tools/gold/gold-plugin.cpp
811–818

You should be able to set all of these without the if.

This revision is now accepted and ready to land.Apr 12 2018, 1:31 PM
yunlian updated this revision to Diff 142296.Apr 12 2018, 4:21 PM
yunlian marked an inline comment as done.
yunlian updated this revision to Diff 142323.Apr 12 2018, 8:39 PM

rebase to trunk.

yunlian updated this revision to Diff 142324.Apr 12 2018, 8:52 PM

Fix an indent error during rebase.

This revision was automatically updated to reflect the committed changes.