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.
Details
Diff Detail
- Build Status
Buildable 1932 Build 1932: arc lint + arc unit
Event Timeline
Looks good generally, but a few questions/comments.
llvm/include/llvm-c/lto.h | ||
---|---|---|
651 | Doc needs update - returns path to ... | |
658 | Since version should be 21? | |
llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h | ||
87 | 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 | Ditto | |
294 | 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 | Document return value | |
746 | What is the point of this line? | |
762 | 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? | |
950 | Remind we why we do this instead of just using the existing OutputBuffer? | |
llvm/test/ThinLTO/X86/save_objects.ll | ||
7 | Nit: why use ".o" extension for save objects which is a directory? |
Thanks for the review! See inline.
llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h | ||
---|---|---|
87 | 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 | This ensure that the SmallString is null terminated before calling sys::fs::exists (added a comment) | |
950 | 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). | |
llvm/test/ThinLTO/X86/save_objects.ll | ||
7 | This is just what Xcode is doing, because originally it was implemented for Monolithic LTO where there is a single output file ;) |
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 | 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 | Ok, thanks | |
950 | I see, because reloading will attempt to mmap the file. Nit: typo in new comment "pressuree" |
llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h | ||
---|---|---|
87 | 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). |
llvm/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h | ||
---|---|---|
87 | 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... |
Doc needs update - returns path to ...