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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1815
+  if (ReachedLimit)
+    State.reportError("the output size produced reached the limit. Use the "
+                      "--max-size option to change the limit");
----------------
On further reflection, I think this message should be: "the desired output size is greater than permitted. Use the ..."


================
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
----------------
grimar wrote:
> 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).
Doesn't `Sections: []` result in no sections (and therefore no section headers) at all?


================
Comment at: llvm/test/tools/yaml2obj/ELF/output-limit.yaml:1
+## Check that yaml2obj limits the output size by default to 10 MB.
+## Check it is possible to change this limit using the
----------------
You need a test case for --max-size=0 (showing that it doesn't set the max size to zero, or leave it as the default).


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

https://reviews.llvm.org/D81258