[PATCH] D84858: [llvm-libtool-darwin] Refactor ArchiveWriter

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:555
 
-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>
+writeArchiveInFD(StringRef ArcName, ArrayRef<NewArchiveMember> NewMembers,
----------------
This should be `static`.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:673-674
+
+  ErrorOr<std::unique_ptr<MemoryBuffer>> Ret =
+      MemoryBuffer::getOpenFile(Temp->FD, ArcName, -1);
+  if (std::error_code EC = Ret.getError())
----------------
smeenai wrote:
> sameerarora101 wrote:
> > 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!
> We might be able to do something with raw_svector_ostream. I'll experiment a bit.
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.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84858/new/

https://reviews.llvm.org/D84858