[lld] r221552 - [mach-o] Fix MachOFileNode to own archives same as ELFFileNode

[David Blaikie]

On Fri, Nov 7, 2014 at 2:00 PM, Nick Kledzik <kledzik at apple.com> wrote:

> Author: kledzik
> Date: Fri Nov  7 16:00:23 2014
> New Revision: 221552
>
> URL: http://llvm.org/viewvc/llvm-project?rev=221552&view=rev
> Log:
> [mach-o] Fix MachOFileNode to own archives same as ELFFileNode
>
> My previous fix to have FileArchive own the member MemoryBuffers was not a
> complete solution for darwin because nothing owned the FileArchive object.
> Fixed MachOFileNode to be like ELFFileNode and have the graph node own the
> archive object.
>

Any test coverage for this/the previous change?


>
> Modified:
>     lld/trunk/include/lld/Driver/DarwinInputGraph.h
>     lld/trunk/lib/Driver/DarwinInputGraph.cpp
>
> Modified: lld/trunk/include/lld/Driver/DarwinInputGraph.h
> URL:
> http://llvm.org/viewvc/llvm-project/lld/trunk/include/lld/Driver/DarwinInputGraph.h?rev=221552&r1=221551&r2=221552&view=diff
>
> ==============================================================================
> --- lld/trunk/include/lld/Driver/DarwinInputGraph.h (original)
> +++ lld/trunk/include/lld/Driver/DarwinInputGraph.h Fri Nov  7 16:00:23
> 2014
> @@ -17,6 +17,7 @@
>  #ifndef LLD_DRIVER_DARWIN_INPUT_GRAPH_H
>  #define LLD_DRIVER_DARWIN_INPUT_GRAPH_H
>
> +#include "lld/Core/ArchiveLibraryFile.h"
>  #include "lld/Core/InputGraph.h"
>  #include "lld/ReaderWriter/MachOLinkingContext.h"
>
> @@ -67,6 +68,7 @@ private:
>   void narrowFatBuffer(StringRef filePath);
>
>    MachOLinkingContext &_context;
> +  std::unique_ptr<const ArchiveLibraryFile> _archiveFile;
>    bool _isWholeArchive;
>    bool _upwardDylib;
>  };
>
> Modified: lld/trunk/lib/Driver/DarwinInputGraph.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/Driver/DarwinInputGraph.cpp?rev=221552&r1=221551&r2=221552&view=diff
>
> ==============================================================================
> --- lld/trunk/lib/Driver/DarwinInputGraph.cpp (original)
> +++ lld/trunk/lib/Driver/DarwinInputGraph.cpp Fri Nov  7 16:00:23 2014
> @@ -88,7 +88,10 @@ std::error_code MachOFileNode::parse(con
>      // If file is an archive and -all_load, then add all members.
>      if (ArchiveLibraryFile *archive =
> dyn_cast<ArchiveLibraryFile>(pf.get())) {
>

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").

But it could be a bit closer if you switched the conditions around:

  if (_isWholeArchive) {
    if (ArchiveLibraryFile *archive =
dyn_cast<ArchiveLibraryFile>(pf.get())) {
      pf.release();
      _archiveFile.reset(archive);
    }
  }

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:

  if (_isWholeArchive)
    if (auto archive = dyn_cast<ArchiveLibraryFile>(pf))
      archiveFile = std::move(archive);

(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)

I'm not saying you need to make any changes - just talking it
through/raising some ideas in case any appeal to you.

- David


>        if (_isWholeArchive) {
> -        // Note: the members are added to _files, but the archive is not.
> +        // Have this node own the FileArchive object.
> +        _archiveFile.reset(archive);
> +        pf.release();
> +        // Add all members to _files vector
>          return archive->parseAllMembers(_files);
>        }
>      }
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141111/f6b2d9a0/attachment.html>