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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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:
> 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.
Thanks @grimar. That sounds reasonable to me.


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

https://reviews.llvm.org/D81258