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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:924
+  // a corresponding internal error state if the limit was reached.
+  CBA.getRawOS(0);
+
----------------
jhenderson wrote:
> Could you maybe change `hasReachedLimit` to actually check against the limit? I think that would be a more natural function to use based on names. Same probably goes above for the initial call to `getRawOS` in this function.
> Could you maybe change hasReachedLimit to actually check against the limit?

Done. It is called `takeLimitError` now.

> Same probably goes above for the initial call to getRawOS in this function.

I think I still need to call `getRawOS(0)` because it returns the `raw_ostream` and while the needed size is
unknown, there is probably no better value than `0` for such case.


================
Comment at: llvm/test/tools/yaml2obj/ELF/output-limit.yaml:10
+# RUN: yaml2obj %s -DSIZE=0x9FFEC0 --docnum=1 -o /dev/null 2>&1
+# RUN: not yaml2obj %s -DSIZE=0x9FFEC1 --docnum=1 -o /dev/null 2>&1 | \
+# RUN:  FileCheck %s --check-prefix=ERROR
----------------
jhenderson wrote:
> I think it would be worthwhile testing a case where there is no section header table at all. This is because the `CBA` is not used to write the elf header and program header table.
I'd leave it for a follow-up:

we have an issue currently. Even when all headers are ommitted, we still write them:

```
  State.writeELFHeader(OS, SHOff);
  writeArrayData(OS, makeArrayRef(PHeaders));
  CBA.writeBlobToStream(OS);
  writeArrayData(OS, makeArrayRef(SHeaders));
  return true;
```

Instead we should write the number of headers specified in  `e_shnum`. Actually the logic should be even a bit more complex.
E.g. when the `e_shnum` (e.g 3) is overriden with the use of `SHNum` (e.g. 1), we should write the number of headers that would be written normally, i.e. 3, I think.

For now the condition I`ve used is intentionally simple and matches the current logic:

```
SHOff + arrayDataSize(makeArrayRef(SHeaders)) > MaxSize
```

This patch should allow to fix this issue nicely (will simplify writing a test).


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

https://reviews.llvm.org/D81258