[llvm] [llvm-objcopy] Stream ELF output to reduce peak memory usage (PR #217706)

[James Henderson via llvm-commits]

https://github.com/jh7370 commented:

I've added myself and @MaskRay as reviewers, as we are probably the two most active in the LLVM binutils from a reviewing point of view.

I'm going to be the one to say it: this should be a stacked PR (whether manually stacked or using some interface to do it). We have guides in the LLVM GitHub documentation on how to do this. Please split it up so that each piece can be reviewed independently (reviewers can look at all three by viewing the final branch, if needed). This will also ensure that all three separate elements end up as independent commits in the final output, since otherwise they'll be squashed and merged, and allow for individual fix-up commits to each PR without requiring a force push (which is strongly discouraged because it trashes the ability to do incremental reviewing). These can then be merged into the later branches via a merge commit as needed.

I've done a first pass from a high-ish level point of view (specifically looking at the ObjCopy changes, not the raw_ostream changes). I've got a couple of general comments and then some smaller ones inline. I'll likely have more, but I don't think it's worth me reviewing further until we've got some of these other things answered.

I see from the numbers you've provided that this is an all-round win in both speed and memory consumption for the inputs you tried. Are similar gains visible for smaller inputs (e.g. circa 100MB or 10MB)? I'm assuming there would be, but the last thing we want to do is improve the performance for one set but significantly reduce it for another. Also, I'm guessing you've run these numbers on a Linux-based system, due to your comments about RSS. What about Windows? (To be clear, I'm supportive of these changes in general, assuming these other performance numbers aren't bad).

I assume this is done on an ELF level rather than something higher up, because the different Object formats for ELF/COFF/Mach-O etc use divergent implementations, so there's no way to share things?

I'm not convinced by the noise the error propagation is adding to this code. I wonder if we would be better off capturing the `Error` lower down and then inspecting and reporting it at the end, with the low-level writing code being a no-op if things are already in a bad state. I don't know whether that will just move the ugliness to somewhere else, but it would make the ELFObject implementation cleaner at least (and in my opinion that would make the code more readable). IIRC, we do something similar in the DataExtractor::Cursor class (for reading instead of writing).

https://github.com/llvm/llvm-project/pull/217706