<div dir="ltr">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?<br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 22, 2015 at 11:06 AM, George Rimar <span dir="ltr"><<a href="mailto:grimar@accesssoftek.com" target="_blank">grimar@accesssoftek.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">grimar created this revision.<br>
grimar added a reviewer: ruiu.<br>
grimar added subscribers: llvm-commits, grimar.<br>
grimar added a project: lld.<br>
<br>
There were 5 subclasses of ArchiveLibraryFile in total and two of them required the fix imo.<br>
<br>
<a href="http://reviews.llvm.org/D13061" rel="noreferrer" target="_blank">http://reviews.llvm.org/D13061</a><br>
<br>
Files:<br>
  ReaderWriter/ELF/OutputELFWriter.cpp<br>
  ReaderWriter/FileArchive.cpp<br>
<br>
Index: ReaderWriter/FileArchive.cpp<br>
===================================================================<br>
--- ReaderWriter/FileArchive.cpp<br>
+++ ReaderWriter/FileArchive.cpp<br>
@@ -7,6 +7,7 @@<br>
 //<br>
 //===----------------------------------------------------------------------===//<br>
<br>
+#include "lld/Driver/Driver.h"<br>
 #include "lld/Core/ArchiveLibraryFile.h"<br>
 #include "lld/Core/LLVM.h"<br>
 #include "lld/Core/LinkingContext.h"<br>
@@ -76,8 +77,11 @@<br>
     if (instantiateMember(ci, result))<br>
       return nullptr;<br>
<br>
-    // give up the pointer so that this object no longer manages it<br>
-    return result.release();<br>
+    File *file = result.get();<br>
+    _filesReturned.push_back(std::move(result));<br>
+<br>
+    // Give up the file pointer. It was stored and will be destroyed with destruction of FileArchive<br>
+    return file;<br>
   }<br>
<br>
   // Instantiate a member file containing a given symbol name.<br>
@@ -259,6 +263,7 @@<br>
   std::vector<std::unique_ptr<MemoryBuffer>> _memberBuffers;<br>
   std::map<const char *, std::unique_ptr<Future<File *>>> _preloaded;<br>
   std::mutex _mutex;<br>
+  FileVector _filesReturned;<br>
 };<br>
<br>
 class ArchiveReader : public Reader {<br>
Index: ReaderWriter/ELF/OutputELFWriter.cpp<br>
===================================================================<br>
--- ReaderWriter/ELF/OutputELFWriter.cpp<br>
+++ ReaderWriter/ELF/OutputELFWriter.cpp<br>
@@ -45,8 +45,15 @@<br>
<br>
     assert(!_file->hasAtoms() && "The file shouldn't have atoms yet");<br>
     _resolver(sym, *_file);<br>
-    // If atoms were added - release the file to the caller.<br>
-    return _file->hasAtoms() ? _file.release() : nullptr;<br>
+<br>
+    // If atoms were added - return the file but also store it for later destruction<br>
+    if (_file->hasAtoms()) {<br>
+      File *result = _file.get();<br>
+      _returnedFiles.push_back(std::move(_file));<br>
+      return result;<br>
+    }<br>
+<br>
+    return nullptr;<br>
   }<br>
<br>
 private:<br>
@@ -57,6 +64,7 @@<br>
   // reversed destruction order.<br>
   llvm::BumpPtrAllocator _alloc;<br>
   unique_bump_ptr<SymbolFile<ELFT>> _file;<br>
+  std::vector<unique_bump_ptr<SymbolFile<ELFT>>> _returnedFiles;<br>
 };<br>
<br>
 } // end anon namespace<br>
<br>
<br>
</blockquote></div><br></div></div>