Page MenuHomePhabricator

[Bug 21681] - Fixed: Memory leak in FileArchive::find()
ClosedPublic

Authored by grimar on Sep 22 2015, 11:06 AM.

Details

Reviewers
ruiu
Summary

There were 5 subclasses of ArchiveLibraryFile in total and two of them required the fix imo.

Diff Detail

Event Timeline

grimar updated this revision to Diff 35396.Sep 22 2015, 11:06 AM
grimar retitled this revision from to [Bug 21681] - Fixed: Memory leak in FileArchive::find().
grimar updated this object.
grimar added a reviewer: ruiu.
grimar added a project: lld.
grimar added subscribers: llvm-commits, grimar.
ruiu edited edge metadata.Sep 22 2015, 12:36 PM
ruiu added a subscriber: ruiu.

File object ownership is complicated, and I'm not sure if this fix is
correct. Leaking objects is a bug, but is this safe? Who keeps the
ownership of archive files until all linking is complete?

In D13061#250927, @ruiu wrote:

File object ownership is complicated, and I'm not sure if this fix is
correct. Leaking objects is a bug, but is this safe? Who keeps the
ownership of archive files until all linking is complete?

All archive files are ownered by LinkingContext. Files are added as nodes. (like that: ctx->getNodes().push_back(std::move(node));)
LinkingContext looks to be alive all time until link complete.

ruiu accepted this revision.Sep 23 2015, 2:42 PM
ruiu edited edge metadata.

LGTM

ReaderWriter/ELF/OutputELFWriter.cpp
49–56

Let's return early.

if (_file->hasAtom())
  return nullptr;
...
This revision is now accepted and ready to land.Sep 23 2015, 2:42 PM
grimar updated this revision to Diff 35598.Sep 24 2015, 3:06 AM
grimar edited edge metadata.

Review comments addressed

If everything is ok now - could you please commit ? I dont have write permissions yet.

ReaderWriter/ELF/OutputELFWriter.cpp
49–56

Fixed.
Just please note that we should return nullptr if file does not have atoms.

grimar closed this revision.Sep 25 2015, 2:58 AM

r248525