[PATCH] D78628: [obj2yaml] - Program headers: simplify the computation of p_filesz.

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-size-offset.yaml:33
 # CHECK:    Offset: 0x2004
-# CHECK:    FileSize: 4
+# CHECK:    FileSize: 5
 # CHECK:    MemSize: 6
----------------
jhenderson wrote:
> grimar wrote:
> > grimar wrote:
> > > jhenderson wrote:
> > > > MaskRay wrote:
> > > > > grimar wrote:
> > > > > > MaskRay wrote:
> > > > > > > Why is this changed?
> > > > > > This patch fixes a bug I beliece: it's 
> > > > > > 
> > > > > > ```
> > > > > >   # Program header with 2 SHT_NOBITS sections.
> > > > > >   - Type:     0x6abcdef0
> > > > > >     Offset:   0x2004
> > > > > >     Sections:
> > > > > >       - Section: .data
> > > > > >       - Section: .nobits1
> > > > > >       - Section: .nobits2
> > > > > > ```
> > > > > > 
> > > > > > The layout is:
> > > > > > 
> > > > > > ```
> > > > > > Section Headers:
> > > > > >   [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
> > > > > >   [ 0]                   NULL            0000000000000000 000000 000000 00     0   0  0
> > > > > >   [ 1] .text             PROGBITS        0000000000000000 001000 000004 00     0   0 4096
> > > > > >   [ 2] .rodata           PROGBITS        0000000000000000 002000 000004 00     0   0 4096
> > > > > >   [ 3] .data             PROGBITS        0000000000000000 002004 000004 00     0   0  0
> > > > > >   [ 4] .nobits1          NOBITS          0000000000000000 002008 000001 00     0   0  0
> > > > > >   [ 5] .nobits2          NOBITS          0000000000000000 002009 000001 00     0   0  0
> > > > > >   [ 6] .strtab           STRTAB          0000000000000000 002008 000001 00     0   0  1
> > > > > >   [ 7] .shstrtab         STRTAB          0000000000000000 002009 000039 00     0   0  1
> > > > > > ```
> > > > > > 
> > > > > > `0x2009 - 0x2004 == 0x5`, not `0x4`
> > > > > I think it is hard to say that this is a bug. Conceptually sh_offset of a SHT_NOBITS section can be ignored. Usually, the ELF writer should set the sh_offset field of `.nobits2` to 0x2008 because there is no need to leave a one-byte gap.
> > > > > 
> > > > > I don't think this trivia matter much though, handling it either way is ok. If doing it one way helps simplify our overall logic, let's choose that way.
> > > > Is there a risk that leaving the FileSize of the segment too high might result in it going outside the range of the file? In other words, does yaml2obj pay any attention to the segment sizes when it lays things out? In other words, in this example, if .nobits1 was say size 0xFFFF0000, would it cause the program header to be referencing data outside the file?
> > > > 
> > > > Knowing that llvm-objcopy reads segment data based on the segment file size property, I think we need to be careful about what the FileSize is for segments. It cannot go beyond the file's end.
> > > > if .nobits1 was say size 0xFFFF0000, would it cause the program header to be referencing data outside the file?
> > > 
> > > It is possible with the use of `ShOffset`. But since `ShOffset` is itself a thing for overriding the offset and used mostly for creating invalid object, this probably not a problem?
> > See.
> > 
> > > Is there a risk that leaving the FileSize of the segment too high might result in it going outside the range of the file? 
> > The way to achieve it is to use `ShOffset`, what is probably not an issue. yaml2obj places all sections in order and can'y assign an offset that is outside of the file by itself (without using `ShOffset`).
> > 
> > > In other words, does yaml2obj pay any attention to the segment sizes when it lays things out?
> > The layout of section is unrelated to segments. First we do layout for sections and then create headers independently,
> > basing on the sections layout.
> > 
> > > In other words, in this example, if .nobits1 was say size 0xFFFF0000, would it cause the program header to be referencing data outside the file?
> > 
> > Let me answer this again. I've read `size` as `offset` previously. The size of SHT-NOBITS is not taken into account.
> > It might be possible to have a regular section with a broken size (with the use of `ShSize`) and then the segment file size will be broken. But that is about broken objects again.
> Oh, I think I see. I agree that Sh* values can be ignored for this in general (and indeed, I think we agreed elsewhere that they shouldn't really impact the layout of the segments anyway).
> 
> Just to confirm my understanding, the FileSize increase is because ShOffset of .nobits2 is affecting the size of the segment, and not because the size of .nobits1 is 1?
> Just to confirm my understanding, the FileSize increase is because ShOffset of .nobits2 is affecting the size of the segment, and not because the size of .nobits1 is 1?

Right.


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

https://reviews.llvm.org/D78628