[llvm-libtool-darwin] Refactor ArchiveWriter

[sameerarora101]

Summary

Refactoring function writeArchive in ArchiveWriter. Added a new
function writeArchiveBuffer that returns the archive in a memory
buffer instead of writing it out to the disk. This refactor is necessary
so as to allow llvm-libtool-darwin to write universal files containing
archives.

[sameerarora101]

llvm/lib/Object/ArchiveWriter.cpp

672–673

My current approach involves factoring out the common code of writing the archive to a temporary FD. Now, in case of writeArchive, I simply write the temporary file to disk by invoking Temp->keep(ArcName). However, for writeArchiveBuffer, I read the archive again in a MemoryBuffer using

ErrorOr<std::unique_ptr<MemoryBuffer>> Ret =
    MemoryBuffer::getOpenFile(Temp->FD, ArcName, -1);

This memory buffer is then put in ReturnBuffer (which is the buffer allocated by the caller to receive the archive buffer).

Please let me know if there is a better approach to get the archive in a buffer than this current roundtrip method(writing to a FD and then reading back into a MemBuf). Thanks!

[smeenai]

To add a bunch of context, one functionality we're adding to llvm-libtool-darwin is to produce universal binary outputs. For those who aren't familiar, universal binaries (also known as fat binaries) allow a single binary to have slices for multiple architectures. In libtool's case, if you pass it object files of different architectures as input, it'll group each architecture's object files together into a single-architecture static library, and then group those single-architecture static libraries into a universal static library. The functionality to create the universal static library exists in llvm-lipo, and we're refactoring that out into libObject in D84662 to allow llvm-libtool-darwin to use it as well.

What we'd ideally want is to be able to construct an object::Archive in memory (in libtool), and then pass that Archive along to writeUniversalBinary. Unfortunately, the libObject facilities for reading and writing archives seem to be completely independent. Reading uses Archive, Archive::Child, etc., whereas writing uses NewArchiveMember and writeArchive. There's no way, as far as I can see, to construct an Archive by specifying its members; you can only construct one from a block of memory. Similarly, there's no way to serialize an Archive out to a file.

It'd perhaps be nice to unify the archive reading/writing interfaces, but there might be good reasons for the current split, and doing so would also be an extensive change. Instead, what we're doing here is adding the ability for writeArchive to give us back a MemoryBuffer (instead of writing to a temp file), and then parsing that MemoryBuffer as an Archive. That's a bit wasteful (less so if we eliminate the temp file and just keep things in memory), but given the completely separate reading/writing interfaces, it was the best solution we could think of. We're definitely open to better ideas if people have them!

llvm/lib/Object/ArchiveWriter.cpp

672–673

We might be able to do something with raw_svector_ostream. I'll experiment a bit.

[smeenai]

llvm/lib/Object/ArchiveWriter.cpp

556–559

This should be static.

672–673

I played with this a bit and got something working, but I'm not convinced it's better. My idea was to make writeArchiveInFD take a raw_ostream &Out parameter. All the temp file handling shifts into writeArchive, which opens the temp file, creates a raw_fd_ostream from it, and passes that to writeArchiveInFD. writeArchiveBuffer takes a SmallString<0> & paramater, creates a raw_svector_ostream from that, and passes it to writeArchiveInFD. It then uses MemoryBuffer::getMemBuffer to create a MemoryBuffer from the SmallString and returns that.

The problem is that the MemoryBuffer then doesn't own the memory it's pointing to, which doesn't fit very well with libObject's OwningBinary model. You have to manage the lifetime of the SmallString (which actually owns the data) separately, and make sure the MemoryBuffer doesn't outlive it. (You also have to potentially manage multiple such SmallStrings, once for each slice.) We could use MemoryBuffer::getMemBufferCopy to have the MemoryBuffer own the contents, but then it's doing a copy, which isn't a whole lot better than what you have here. (It's at least avoiding the temp file, but filesystem caches are also a thing, so I'm not sure how much that ends up mattering in practice.)

[sameerarora101]

llvm/lib/Object/ArchiveWriter.cpp

672–673

Ooh makes sense, thanks a lot for explaining in such detail.

[sameerarora101]

Making writeArchiveInFD static

[MaskRay]

llvm/lib/Object/ArchiveWriter.cpp

667

I can't find writeArchiveInFD. Did you forget to add a dependent patch?

[smeenai]

llvm/lib/Object/ArchiveWriter.cpp

667

The old writeArchive is changed to it, in line 556 above.

[MaskRay]

llvm/lib/Object/ArchiveWriter.cpp

557

FD might be a bit of misnomer (to me). I'd expect FD to refer to a file descriptor...

659

What about changing ReturnBuffer to be the return type Expected<std::unique_ptr<MemoryBuffer>> ?

663

Delete the empty line.

665

Delete the empty line.

[jhenderson]

llvm/lib/Object/ArchiveWriter.cpp

557

Perhaps writeTemporaryArchive (since it's an archive in a temp file)?

659

+1. Also, consider naming this writeArchiveToBuffer.

672–673

Honestly, I was a bit surprised to see writeArchiveBuffer messing about with temp files. The streaming approach would have been much closer to what I expected. That being said, if it's complicated to implement, I'm willing to let it through.

[sameerarora101]

Change name and signature of writeArchiveBuffer

[smeenai]

llvm/lib/Object/ArchiveWriter.cpp

672–673

I just learned about the existence of SmallVectorMemoryBuffer, which seems perfect for our use case :)

@sameerarora101, we can have writeTemporaryArchive take a raw_ostream &Out parameter. The temporary file creation and handling shifts into writeArchive. writeArchiveBuffer creates a SmallVector<char, 0> and a raw_svector_ostream from it, and then returns a SmallVectorMemoryBuffer created from its SmallVector. That should allow us to be efficient and also avoid any memory management issues :)

[sameerarora101]

llvm/lib/Object/ArchiveWriter.cpp

672–673

oh wow, I'll try this out. Thanks a lot!

[sameerarora101]

Implement writeArchiveBuffer using SmallVectorMemoryBuffer.

[MaskRay]

llvm/include/llvm/Object/ArchiveWriter.h

47

If OldArchiveBuf is not used (for now), delete it. It is a good idea adding a comment that this function is similar to writeArchive but has a different return type.

[jhenderson]

llvm/lib/Object/ArchiveWriter.cpp

557

The names need redoing again, since the logic has moved around a bit. I recommend this becomes writeArchive or writeARchiveToStream and the function that writes to a file becomes writeArchiveToFile or simply leave as writeArchive since the signatures are different.

Also, ArcName doesn't appear to be used any more?

644–654

I don't think this needs doing - the MemoryBuffer will be destroyed once it goes out-of-scope (I'm assuming that reset effectively just clears the buffer). In that case, there's no need for it to even be passed in. In fact, looking up the stack, there's no need for it to be passed into any of these functions as far as I can see?

[sameerarora101]

llvm/include/llvm/Object/ArchiveWriter.h

47

Removed OldArchiveBuf from signature, thanks.

llvm/lib/Object/ArchiveWriter.cpp

557

Yup, removed ArcName from the signature.

644–654

Isn't it the case that we would need OldArchiveBuf.reset(); in writeArchive and not in writeARchiveToStream? This is because, as per the comment, we need to close all handles to the archive before calling Temp->keep(name), right? I have shifted this to writeArchive now. Please let me know if this is a wrong approach.

[sameerarora101]

Removing OldArchiveBuf from writeArchiveToBuffer signature.

[MaskRay]

Looks good, but I think it'll help if you can upload the dependent patch (draft) so that people can see that the interface is suitable.

[jhenderson]

LGTM! But please wait for others.

llvm/lib/Object/ArchiveWriter.cpp

644–654

Makes sense. Apparently I blanked half the comment...!

[sameerarora101]

In D84858#2195417, @MaskRay wrote:

Looks good, but I think it'll help if you can upload the dependent patch (draft) so that people can see that the interface is suitable.

You mean the next diff (dependent on this one) where I'll be using writeArchiveToBuffer? Yup, I plan to upload it soon. Thanks.

[smeenai]

Looks great!

llvm/lib/Object/ArchiveWriter.cpp

639

I believe you need to explicitly call Temp->discard() in this path, otherwise you'll trip this assert. Note that discard() itself can return an error, in which case you probably want to do joinErrors?

[MaskRay]

LGTM, with @smeenai's sys::fs::TempFile::discard comment addressed.

-DCMAKE_BUILD_TYPE=Debug builds enable assertions by default. For Release builds, you may want -DLLVM_ENABLE_ASSERTIONS=on

[sameerarora101]

llvm/lib/Object/ArchiveWriter.cpp

639

Oh makes sense, thanks. Added it now.

[sameerarora101]

Add Temp->dicard() error.

[sameerarora101]

Rebase to resolve previous build errors.

llvm/include/llvm/Object/ArchiveWriter.h

 };
 
 Expected<std::string> computeArchiveRelativePath(StringRef From, StringRef To);
 
 Error writeArchive(StringRef ArcName, ArrayRef<NewArchiveMember> NewMembers,
                    bool WriteSymtab, object::Archive::Kind Kind,
                    bool Deterministic, bool Thin,
                    std::unique_ptr<MemoryBuffer> OldArchiveBuf = nullptr);
+
+// writeArchiveToBuffer is similar to writeArchive but returns the Archive in a
+// buffer instead of writing it out to a file.
+Expected<std::unique_ptr<MemoryBuffer>>
+writeArchiveToBuffer(ArrayRef<NewArchiveMember> NewMembers, bool WriteSymtab,
+                     object::Archive::Kind Kind, bool Deterministic, bool Thin);
 }
 
 #endif

llvm/lib/Object/ArchiveWriter.cpp

 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Object/SymbolicFile.h"
 #include "llvm/Support/Alignment.h"
 #include "llvm/Support/EndianStream.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/SmallVectorMemoryBuffer.h"
 #include "llvm/Support/ToolOutputFile.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include <map>
 
 #if !defined(_MSC_VER) && !defined(__MINGW32__)
 #include <unistd.h>
 #else
for (auto FromE = sys::path::end(DirFrom); FromI != FromE; ++FromI)
     sys::path::append(Relative, sys::path::Style::posix, "..");
 
   for (auto ToE = sys::path::end(PathTo); ToI != ToE; ++ToI)
     sys::path::append(Relative, sys::path::Style::posix, *ToI);
 
   return std::string(Relative.str());
 }
 
-Error writeArchive(StringRef ArcName, ArrayRef<NewArchiveMember> NewMembers,
+static Error writeArchiveToStream(raw_ostream &Out,
+                                  ArrayRef<NewArchiveMember> NewMembers,
                                   bool WriteSymtab, object::Archive::Kind Kind,
-                   bool Deterministic, bool Thin,
-                   std::unique_ptr<MemoryBuffer> OldArchiveBuf) {
+                                  bool Deterministic, bool Thin) {
   assert((!Thin || !isBSDLike(Kind)) && "Only the gnu format has a thin mode");
 
   SmallString<0> SymNamesBuf;
   raw_svector_ostream SymNames(SymNamesBuf);
   SmallString<0> StringTableBuf;
   raw_svector_ostream StringTable(StringTableBuf);
 
   Expected<std::vector<MemberData>> DataOrErr = computeMemberData(
if (WriteSymtab) {
     if (LastOffset >= (1ULL << Sym64Threshold)) {
       if (Kind == object::Archive::K_DARWIN)
         Kind = object::Archive::K_DARWIN64;
       else
         Kind = object::Archive::K_GNU64;
     }
   }
 
-  Expected<sys::fs::TempFile> Temp =
-      sys::fs::TempFile::create(ArcName + ".temp-archive-%%%%%%%.a");
-  if (!Temp)
-    return Temp.takeError();
-
-  raw_fd_ostream Out(Temp->FD, false);
   if (Thin)
     Out << "!<thin>\n";
   else
     Out << "!<arch>\n";
 
   if (WriteSymtab)
     writeSymbolTable(Out, Kind, Deterministic, Data, SymNamesBuf);
 
   for (const MemberData &M : Data)
     Out << M.Header << M.Data << M.Padding;
 
   Out.flush();
+  return Error::success();
+}
+
+Error writeArchive(StringRef ArcName, ArrayRef<NewArchiveMember> NewMembers,
+                   bool WriteSymtab, object::Archive::Kind Kind,
+                   bool Deterministic, bool Thin,
+                   std::unique_ptr<MemoryBuffer> OldArchiveBuf) {
+  Expected<sys::fs::TempFile> Temp =
+      sys::fs::TempFile::create(ArcName + ".temp-archive-%%%%%%%.a");
+  if (!Temp)
+    return Temp.takeError();
+  raw_fd_ostream Out(Temp->FD, false);
+
+  if (Error E = writeArchiveToStream(Out, NewMembers, WriteSymtab, Kind,
+                                     Deterministic, Thin)) {
+    if (Error DiscardError = Temp->discard())
+      return joinErrors(std::move(E), std::move(DiscardError));
+    return E;
+  }
 
   // At this point, we no longer need whatever backing memory
   // was used to generate the NewMembers. On Windows, this buffer
   // could be a mapped view of the file we want to replace (if
   // we're updating an existing archive, say). In that case, the
   // rename would still succeed, but it would leave behind a
   // temporary file (actually the original file renamed) because
   // a file cannot be deleted while there's a handle open on it,
   // only renamed. So by freeing this buffer, this ensures that
   // the last open handle on the destination file, if any, is
   // closed before we attempt to rename.
   OldArchiveBuf.reset();
 
   return Temp->keep(ArcName);
 }
 
+Expected<std::unique_ptr<MemoryBuffer>>
+writeArchiveToBuffer(ArrayRef<NewArchiveMember> NewMembers, bool WriteSymtab,
+                     object::Archive::Kind Kind, bool Deterministic,
+                     bool Thin) {
+  SmallVector<char, 0> ArchiveBufferVector;
+  raw_svector_ostream ArchiveStream(ArchiveBufferVector);
+
+  if (Error E = writeArchiveToStream(ArchiveStream, NewMembers, WriteSymtab,
+                                     Kind, Deterministic, Thin))
+    return std::move(E);
+
+  return std::make_unique<SmallVectorMemoryBuffer>(
+      std::move(ArchiveBufferVector));
+}
+
 } // namespace llvm