[PATCH] D81258: [yaml2obj] - Introduce a ~10 Mb limit of the output by default and a --nolimit option.

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:908-909
+  // --nolimit yaml2obj command line option.
+  raw_ostream &OS = *CBA.getRawOS(0x100000 * 8); // 8Mb.
+  uint64_t BeginOffset = CBA.tell();
   if (Name == ".debug_str")
----------------
Higuoxing wrote:
> jhenderson wrote:
> > This feels like a bit of a hack to get this to work with the DWARFYAML interface. @Higuoxing, how straightforward do you think it would be to modify the DWARFYAML interface to take a `ContiguousBlobAccumulator`? I'm guessing not particularly, since we'd need to move it into a shared header etc, but thought I'd ask.
> > 
> > @grimar, as an alternative, maybe you could create a new CBA here, with a size limit equal to the remaining size available in the original CBA. You'd then append its buffer to the original, once you'd checked the size limit hadn't been breached.
> I think it's not easy to change the interface. `MachOEmitter` doesn't use the `ContiguousBlobAccumulator`.
> since we'd need to move it into a shared header etc

Sharing `ContiguousBlobAccumulator` is a reasonable move I think. I thought about it when wrote this, as doing this should allow to reuse it for non-ELF emitters.

> as an alternative, maybe you could create a new CBA here, with a size limit equal to the remaining size available in the original CBA.

Ok, I'll try. But it is still not ideal. While we use `raw_ostream` in the `DWARFYAML` directly, it is possible to break everything. Imagine I create `ContiguousBlobAccumulator`. with a limit of 10 Mb here, but `DWARFYAML` tries to write 10 Gb.
We might face a OOM inside `DWARFYAML` logic before having a chance to check that the size limit is breached here.

So the only right way is changing the DWARFYAML interface I believe. I think for now we can go with your suggestion and leave `DWARFYAML` changes for a follow-up?



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

https://reviews.llvm.org/D81258