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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:556
+static Expected<sys::fs::TempFile>
+writeArchiveInFD(StringRef ArcName, ArrayRef<NewArchiveMember> NewMembers,
+                 bool WriteSymtab, object::Archive::Kind Kind,
----------------
MaskRay wrote:
> FD might be a bit of misnomer (to me). I'd expect FD to refer to a file descriptor...
Perhaps `writeTemporaryArchive` (since it's an archive in a temp file)?


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:660
 
+Error writeArchiveBuffer(StringRef ArcName,
+                         ArrayRef<NewArchiveMember> NewMembers,
----------------
MaskRay wrote:
> What about changing ReturnBuffer to be the return type `Expected<std::unique_ptr<MemoryBuffer>> `?
+1. Also, consider naming this `writeArchiveToBuffer`.


================
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())
----------------
sameerarora101 wrote:
> smeenai wrote:
> > 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.)
> Ooh makes sense, thanks a lot for explaining in such detail. 
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84858