This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Add an API to trigger file-based API for returning objects to the linker
ClosedPublic

Authored by mehdi_amini on Dec 7 2016, 1:27 AM.

Details

Summary

The motivation is to support better the -object_path_lto option on
Darwin. The linker needs to write down the generate object files on
disk for later use by lldb or dsymutil (debug info are not present
in the final binary). We're moving this into libLTO so that we can
be smarter when a cache is enabled and hard-link when possible
instead of duplicating the files.

Diff Detail

Repository
rL LLVM

Event Timeline

mehdi_amini updated this revision to Diff 80552.Dec 7 2016, 1:27 AM
mehdi_amini retitled this revision from to [ThinLTO] Add an API to trigger file-based API for returning objects to the linker.
mehdi_amini updated this object.
mehdi_amini added reviewers: tejohnson, pcc.
mehdi_amini added subscribers: llvm-commits, dexonsmith.
mehdi_amini updated this revision to Diff 80557.Dec 7 2016, 2:36 AM
mehdi_amini edited edge metadata.

Fix typo in the C API

tejohnson edited edge metadata.Dec 12 2016, 10:02 AM

Looks good generally, but a few questions/comments.

llvm/include/llvm-c/lto.h
651 ↗(On Diff #80557)

Doc needs update - returns path to ...

658 ↗(On Diff #80557)

Since version should be 21?

llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h
87 ↗(On Diff #80557)

Give an error here if the wrong routine called? I.e. if getProducedBinaries called and setGeneratedObjectsDirectory was called. I see that the C api will assert if the wrong interface is called because the index will be out of range, but perhaps a more meaningful error could be given.

97 ↗(On Diff #80557)

Ditto

294 ↗(On Diff #80557)

This variable name was confusing to me - can you change to something like SavedObjectsDirectoryPath or something like that (and change corresponding parameter names where this is passed in)?

llvm/lib/LTO/ThinLTOCodeGenerator.cpp
740 ↗(On Diff #80557)

Document return value

746 ↗(On Diff #80557)

What is the point of this line?

762 ↗(On Diff #80557)

I guess this is the case where the cache entry was somehow pruned between the time the OutputBuffer was loaded and when the copy/hardlink was attempted? Can you add a comment to that effect?

953 ↗(On Diff #80557)

Remind we why we do this instead of just using the existing OutputBuffer?

llvm/test/ThinLTO/X86/save_objects.ll
6 ↗(On Diff #80557)

Nit: why use ".o" extension for save objects which is a directory?

mehdi_amini marked 8 inline comments as done.
mehdi_amini edited edge metadata.

Address Teresa's comments

Thanks for the review! See inline.

llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h
87 ↗(On Diff #80557)

There shouldn't be an assert in the C API if correctly used:

The client is supposed to iterate over the range given by thinlto_module_get_num_object_files() and call thinlto_module_get_object_file() (similarly thinlto_module_get_object/thinlto_module_get_num_objects).

The way I implemented it in ld64 is basically:

for (int BufId = 0; BufID < thinlto_module_get_num_objects; ++BufID) {
   process(thinlto_module_get_object(BufID));
}
for (int FileID = 0; FileID < thinlto_module_get_num_object_files; ++FileID) {
   process(thinlto_module_get_object_file(FileID));
}

Does it make sense?

llvm/lib/LTO/ThinLTOCodeGenerator.cpp
746 ↗(On Diff #80557)

This ensure that the SmallString is null terminated before calling sys::fs::exists (added a comment)

953 ↗(On Diff #80557)

Lower memory consumption: the buffer is on the heap and releasing this memory can be used for the next file. The final binary link will reload from disk (or from the VFS cache if the memory pressure wasn't too high).
(Added comment to this effect)

llvm/test/ThinLTO/X86/save_objects.ll
6 ↗(On Diff #80557)

This is just what Xcode is doing, because originally it was implemented for Monolithic LTO where there is a single output file ;)
Changed it to %t.thin.out

tejohnson accepted this revision.Dec 12 2016, 12:14 PM
tejohnson edited edge metadata.

LGTM, see small comments below. Error message I was suggesting is not a blocker, up to you if you agree.

llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h
87 ↗(On Diff #80557)

Sure, it won't assert if invoked properly, I was suggesting a more meaningful error in case it is not.

llvm/lib/LTO/ThinLTOCodeGenerator.cpp
746 ↗(On Diff #80557)

Ok, thanks

953 ↗(On Diff #80557)

I see, because reloading will attempt to mmap the file. Nit: typo in new comment "pressuree"

This revision is now accepted and ready to land.Dec 12 2016, 12:14 PM
mehdi_amini marked 3 inline comments as done.Dec 12 2016, 1:16 PM
mehdi_amini added inline comments.
llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h
87 ↗(On Diff #80557)

I just don't understand what error you're suggesting here, can you clarify?

Right now this API is always OK to be called, and both the thinlto_module_get_num_object_files and thinlto_module_get_num_objects are invoking it during the normal use I mentioned above (loop bound check).
That's why the C API asserts only when you access out-of-bound and not here when you access the vector itself (to check for size for instance).

tejohnson added inline comments.Dec 12 2016, 1:22 PM
llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h
87 ↗(On Diff #80557)

Oh I see, you need to access the maps to get the size in order to guard/index the calls appropriately. So what I had in mind won't work. I was just trying to see if there was a more meaningful error other than the ones in thinlto_module_get_object_file() and the like than "Index overflow" when the wrong map was accessed. Maybe not worth it...

This revision was automatically updated to reflect the committed changes.