[PATCH] D91028: [llvm-objcopy][NFC] replace class Buffer/MemBuffer/FileBuffer with streams.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Hopefully some other folks with more investment in dsymutil chime in here, but at least:

> This patch uses raw_ostream instead of Buffers. Generally, using streams could allow us to reduce memory usages. No need to load all data into the memory - the data could be streamed through a smaller buffer.

Aren't many MemoryBuffers backed by memory mapped files, which aren't necessarily causing real memory usage? Could that approach be used here?

That said, pretty sure LLVM's integrated assembler uses a pwrite stream to write out its object files, so if we can/if this patch does align with that method of output that seems OK.

I'm confused by the tenses in some of the notes in the patch description:

> Note 2. It would be better if Writers would be implemented in a such way that data could be streamed without seeking/updating. If that would be inconvenient then raw_ostream could be replaced with raw_pwrite_stream to have a possibility to seek back and update file headers. This is assumed to be done later if necessary.

So this patch as proposed is not using a pwrite stream, it seems? "if Writers could be implemented in such a way that data could be streamed without seeking/updating" - how does that work today, without the patch, and how does the patch address that need to seek? I don't believe it's possible to write an ELF file without either knowing at least the sizes of all the sections up-front (you might be able to know these sizes without actually buffering the entire contents of the file) or seeking. Because the header needs to either contain an offset to the section table stored at the end of the file, where it knows the offsets/lengths of all the sections, or you write the section table at the start (so the header knows the offset up-front) and the table then needs to know the offsets of all the sections up-front.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91028