[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/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:
> 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.


================
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
----------------
jhenderson wrote:
> 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).
I had it below as a part of --docnum=2:

```
## Another possible case is when an alignment gap inserted
## is too large because of overaligning. Check it is also handled properly.
## Also check that we can drop the limit with the use of --max-size=0.
# RUN: not yaml2obj %s --docnum=2 -o /dev/null 2>&1 | FileCheck %s --check-prefix=ERROR
# RUN: yaml2obj --max-size=0 %s --docnum=2 -o /dev/null
```

I've splitted it out to a separate case.


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

https://reviews.llvm.org/D81258