This is an archive of the discontinued LLVM Phabricator instance.

[gold/ThinLTO] Restore ThinLTO file management in gold plugin
ClosedPublic

Authored by tejohnson on Aug 19 2016, 11:03 AM.

Details

Summary

The gold-plugin changes added along with the new LTO API in r278338 had
the effect of removing the management of the PluginInputFile that
ensured the files weren't released back to gold until the backend
threads were complete. Add back the old file handling.

Fixes PR29020.

Diff Detail

Event Timeline

tejohnson updated this revision to Diff 68715.Aug 19 2016, 11:03 AM
tejohnson retitled this revision from to [gold/ThinLTO] Restore ThinLTO file management in gold plugin.
tejohnson updated this object.
tejohnson added a reviewer: mehdi_amini.
tejohnson added subscribers: hjl.tools, llvm-commits.
tejohnson added a subscriber: pcc.Aug 19 2016, 4:54 PM
mehdi_amini added inline comments.Aug 19 2016, 5:05 PM
tools/gold/gold-plugin.cpp
833

Fo you need a map here? Can't you just push back to a vector?

tejohnson added inline comments.Aug 19 2016, 5:09 PM
tools/gold/gold-plugin.cpp
833

This is necessary for correct file locking when we have archives. See the first part of the summary for r270814. Specifically, ensure we only attempt to lock and unlock the underlying archive file once.

mehdi_amini accepted this revision.Aug 19 2016, 5:31 PM
mehdi_amini edited edge metadata.

LGTM.

tools/gold/gold-plugin.cpp
833

For some reason I missed the HandleToInputFile.count(..) the first time I read this. It makes sense now.

This revision is now accepted and ready to land.Aug 19 2016, 5:31 PM
This revision was automatically updated to reflect the committed changes.
pcc added a comment.Sep 15 2016, 11:59 AM

This change doesn't seem right to me. If the link command has ThinLTO inputs but does not pass in the thinlto plugin option, we will release files early.

Regrettably I think we may need to move some of the memory management into the lib/LTO interface.

In D23721#543923, @pcc wrote:

This change doesn't seem right to me. If the link command has ThinLTO inputs but does not pass in the thinlto plugin option, we will release files early.

Good point (before the new API gold would only handle the file as ThinLTO with the plugin option).

Regrettably I think we may need to move some of the memory management into the lib/LTO interface.

Since this is a gold-specific issue (particularly the way it ensures that an archive is only opened once), how about we simply check whether it is a ThinLTO file here in place of using the option? I know it is conceptually nice to not have to check the IR object type, but I think the file management is better left in the client.

It seems to me that this should be part of the contract of the API: at the moment all the input buffers are supposed to stay available for the duration of the process.
This is not an issue for mapped file I believe (which I guess every clients are using right now?), but a client that have to decompress buffers in memory before calling the API may be happy to have the ability to release buffers early. We could rework the InputFile to carry the lifetime if needed.

pcc added a comment.Sep 23 2016, 11:17 AM

I agree with Mehdi here.

One possibility would be to change InputFile to store a MemoryBuffer instead of a MemoryBufferRef. Clients that require special handling of freed InputFiles can supply a custom derived class of MemoryBuffer with an overridden destructor.