[PATCH] D39449: Rewrite FileOutputBuffer as two separate classes.

[Zachary Turner via Phabricator via llvm-commits]

zturner accepted this revision.
zturner added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Support/FileOutputBuffer.cpp:84-85
+    std::error_code EC;
+    MemoryBlock MB = Memory::allocateMappedMemory(
+        Size, nullptr, sys::Memory::MF_READ | sys::Memory::MF_WRITE, EC);
+    if (EC)
----------------
ruiu wrote:
> zturner wrote:
> > ruiu wrote:
> > > zturner wrote:
> > > > Why can't we just use `new` here?
> > > I didn't design `Memory` class, but it looks like that class can be instantiated only via allocateMappedMemory.
> > Right, but why do we even need to use the `Memory` class at all?  It seems like overkill.  The comments say:
> > 
> > ```
> >     /// This method allocates a block of memory that is suitable for loading
> >     /// dynamically generated code (e.g. JIT). 
> > ```
> > 
> > Since we don't care about the protection (RWX) of the temporary storage, and only about the protection of the final file on disk, it seems like this is unnecessary.
> > 
> > Instead of storing `OwningMemoryBlock` as a member of the class, you could just store `std::vector<uint8_t>`.
> > 
> > There are fewer failure paths this way, as it's literally just one call to `new`, whereas `allocateMappedMemory` has several ways it can return errors.
> How can you catch memory exhaustion error if you use `new`?
Hmm, yea.  I guess that's a problem especially since file can be really large.

Honestly I've always had a strong dislike for how in LLVM we write to files by mapping the entire contents into memory and then writing to it.

There was actually a bug just yesterday because of this exact reason, where someone was trying to write a TAR file > 4GB, and so a bunch of hacks have to be added to lit to deal with the address space of the process.  If something is a file, you should just be able to write to it.  Especially in this case, where "writing directly to the underlying file" (as opposed to writing to a separate file first and then renaming) is actually the desired behavior.

Anyway, I guess for now since we need to handle OOM error this is fine.


https://reviews.llvm.org/D39449