[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")
----------------
grimar wrote:
> 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?
> 
> I think it's not easy to change the interface. MachOEmitter doesn't use the ContiguousBlobAccumulator.

Yeah. I'd do it later. We can try to change all non-ELF emitters to take `ContiguousBlobAccumulator` to make the new limit option work for them too.


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

https://reviews.llvm.org/D81258