[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 accepted this revision.
jhenderson added a comment.

LGTM.



================
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:
> > grimar wrote:
> > > jhenderson wrote:
> > > > 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?
> > > If you mean just:
> > > 
> > > ```
> > > --- !ELF
> > > FileHeader:
> > >   Class:   ELFCLASS64
> > >   Data:    ELFDATA2LSB
> > >   Type:    ET_EXEC
> > >   Machine: EM_X86_64
> > > Sections: []
> > > ```
> > > 
> > > Then - no, the output will contain 2 implicit sections (.strtab and .shstrtab) + the SHF_UNDEF section at index 0.
> > > 
> > > Sections headers can be omitted with:
> > > 
> > > ```
> > > SectionHeaderTable:
> > >   Sections: []
> > > ```
> > > 
> > > But because of the bug I've described, this only sets the `e_shoff` and `e_shnum` to 0,
> > > but still writes the section header table which consumes the file size.
> > I was thinking of the former. Sorry, I thought that did the job (why are we emitting the string tables in that situation...?). Thanks for the clarification.
> > why are we emitting the string tables in that situation...?
> 
> So, for the first case, which is
> 
> ```
> --- !ELF
> FileHeader:
>   Class:   ELFCLASS64
>   Data:    ELFDATA2LSB
>   Type:    ET_EXEC
>   Machine: EM_X86_64
> Sections: []
> ```
> 
> The output is:
> 
> ```
> Section Headers:
>   [Nr] Name              Type             Address           Offset
>        Size              EntSize          Flags  Link  Info  Align
>   [ 0]                   NULL             0000000000000000  00000000
>        0000000000000000  0000000000000000           0     0     0
>   [ 1] .strtab           STRTAB           0000000000000000  00000040
>        0000000000000001  0000000000000000           0     0     1
>   [ 2] .shstrtab         STRTAB           0000000000000000  00000041
>        0000000000000013  0000000000000000           0     0     1
> ```
> 
> I.e. we emit a ` .strtab` with a one null byte (a minimal valid string table) and a `.shstrtab` with their names.
> It happens because we add those implicit sections unconditionally, we never tried to stop adding them
> when the `Sections` key is explicitly set as empty I think.
> 
> 
> For the second case, which is
> 
> ```
> SectionHeaderTable:
>   Sections: []
> ```
> 
> Perhaps we also could stop emitting the `shstrtab`. Currently we do not add excluded section names to it:
> 
> ```
>     if (!ExcludedSectionHeaders.count(S->Name))
>       DotShStrtab.add(ELFYAML::dropUniqueSuffix(S->Name));
> ```
> 
> But I think that even when all section headers are excluded, we probably still emit the 1 byte length `.shstrtab` ('\0'),
> what is probably not a big problem though.
Thinking about it further, it could be argued that the section header string table is required for the index zero section header, since that has an sh_name of 0. By including a section header string table containing at least a null byte, it is possible to avoi special handling of that section header's name. 


================
Comment at: llvm/test/tools/yaml2obj/ELF/output-limit.yaml:55
+## Check that we can drop the limit with the use of --max-size=0.
+# RUN: yaml2obj --max-size=0 %s --docnum=2 -o /dev/null
----------------
Thanks. I don't know how I managed to miss that before.


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

https://reviews.llvm.org/D81258