<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Nov 7, 2014 at 2:00 PM, Nick Kledzik <span dir="ltr"><<a href="mailto:kledzik@apple.com" target="_blank">kledzik@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: kledzik<br>
Date: Fri Nov  7 16:00:23 2014<br>
New Revision: 221552<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=221552&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=221552&view=rev</a><br>
Log:<br>
[mach-o] Fix MachOFileNode to own archives same as ELFFileNode<br>
<br>
My previous fix to have FileArchive own the member MemoryBuffers was not a<br>
complete solution for darwin because nothing owned the FileArchive object.<br>
Fixed MachOFileNode to be like ELFFileNode and have the graph node own the<br>
archive object.<br></blockquote><div><br></div><div>Any test coverage for this/the previous change?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Modified:<br>
    lld/trunk/include/lld/Driver/DarwinInputGraph.h<br>
    lld/trunk/lib/Driver/DarwinInputGraph.cpp<br>
<br>
Modified: lld/trunk/include/lld/Driver/DarwinInputGraph.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/include/lld/Driver/DarwinInputGraph.h?rev=221552&r1=221551&r2=221552&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/include/lld/Driver/DarwinInputGraph.h?rev=221552&r1=221551&r2=221552&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/include/lld/Driver/DarwinInputGraph.h (original)<br>
+++ lld/trunk/include/lld/Driver/DarwinInputGraph.h Fri Nov  7 16:00:23 2014<br>
@@ -17,6 +17,7 @@<br>
 #ifndef LLD_DRIVER_DARWIN_INPUT_GRAPH_H<br>
 #define LLD_DRIVER_DARWIN_INPUT_GRAPH_H<br>
<br>
+#include "lld/Core/ArchiveLibraryFile.h"<br>
 #include "lld/Core/InputGraph.h"<br>
 #include "lld/ReaderWriter/MachOLinkingContext.h"<br>
<br>
@@ -67,6 +68,7 @@ private:<br>
  void narrowFatBuffer(StringRef filePath);<br>
<br>
   MachOLinkingContext &_context;<br>
+  std::unique_ptr<const ArchiveLibraryFile> _archiveFile;<br>
   bool _isWholeArchive;<br>
   bool _upwardDylib;<br>
 };<br>
<br>
Modified: lld/trunk/lib/Driver/DarwinInputGraph.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/lib/Driver/DarwinInputGraph.cpp?rev=221552&r1=221551&r2=221552&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/lib/Driver/DarwinInputGraph.cpp?rev=221552&r1=221551&r2=221552&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/lib/Driver/DarwinInputGraph.cpp (original)<br>
+++ lld/trunk/lib/Driver/DarwinInputGraph.cpp Fri Nov  7 16:00:23 2014<br>
@@ -88,7 +88,10 @@ std::error_code MachOFileNode::parse(con<br>
     // If file is an archive and -all_load, then add all members.<br>
     if (ArchiveLibraryFile *archive = dyn_cast<ArchiveLibraryFile>(pf.get())) {<br></blockquote><div><br>Pity about this (I don't think we have any good answer for this "maybe I want to transfer ownership if this thing is this other kind of thing").<br><br>But it could be a bit closer if you switched the conditions around:<br><br>  if (_isWholeArchive) {<br>    if (ArchiveLibraryFile *archive = dyn_cast<ArchiveLibraryFile>(pf.get())) {<br>      pf.release();<br>      _archiveFile.reset(archive);<br>    }<br>  }<br><br>Keeps the change in ownership a bit closer together - at some point maybe we can add a dyn_cast for unique_ptrs that does the right thing and then the code would become:<br><br>  if (_isWholeArchive)<br>    if (auto archive = dyn_cast<ArchiveLibraryFile>(pf))<br>      archiveFile = std::move(archive);<br><br>(whereas as written today, the "if (_isWholeArchive)" nested inside makes the ownership a bit trickier - 'archive' doesn't own until you're inside the "if (_isWholeArchive)" at which point it does, and is passed to '_archiveFile)<br><br>I'm not saying you need to make any changes - just talking it through/raising some ideas in case any appeal to you.<br><br>- David<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
       if (_isWholeArchive) {<br>
-        // Note: the members are added to _files, but the archive is not.<br>
+        // Have this node own the FileArchive object.<br>
+        _archiveFile.reset(archive);<br>
+        pf.release();<br>
+        // Add all members to _files vector<br>
         return archive->parseAllMembers(_files);<br>
       }<br>
     }<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div>