[PATCH] D73484: [llvm-readobj] - Improve error message reported by DynRegionInfo.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test:360
+##    a location and an entity size of the dynamic symbol table from the section header.
+##    The code uses sizeof(Elf_Sym) for an entity size, so it can't be incorrect and
+##    the message printed is a bit shorter.
----------------
grimar wrote:
> jhenderson wrote:
> > grimar wrote:
> > > jhenderson wrote:
> > > > grimar wrote:
> > > > > jhenderson wrote:
> > > > > > Aside: why is it not using the DT_SYMENT dynamic tag?
> > > > > I do not know. Perhaps we want to fix this. Also, I think this area is not well tested.
> > > > > I can investigate it in a follow-up.
> > > > Sounds good.
> > > The behavior of GNU readelf looks broken to me.
> > > See: when we have `DT_SYMENT == 0x123` and a `.dynsym` of size of `0x1`:
> > > 
> > > 
> > > ```
> > > --- !ELF
> > > FileHeader:
> > >   Class:   ELFCLASS64
> > >   Data:    ELFDATA2LSB
> > >   Type:    ET_DYN
> > >   Machine: EM_X86_64
> > > Sections:
> > >   - Name:    .dynamic
> > >     Type:    SHT_DYNAMIC
> > >     Entries:
> > >       - Tag:   DT_SYMTAB
> > >         Value: 0x100
> > >       - Tag:   DT_SYMENT
> > >         Value: 0x123
> > >       - Tag:   DT_NULL
> > >         Value: 0
> > >   - Name:    .dynsym
> > >     Type:    SHT_DYNSYM
> > >     Address: 0x100
> > >     Size:    0x1
> > > ProgramHeaders:
> > >   - Type: PT_LOAD
> > >     VAddr: 0x100
> > >     Sections:
> > >       - Section: .dynsym
> > > ```
> > > 
> > > what produce the object with following section headers:
> > > 
> > > 
> > > ```
> > > Section Headers:
> > >   [Nr] Name              Type             Address           Offset
> > >        Size              EntSize          Flags  Link  Info  Align
> > >   [ 0]                   NULL             0000000000000000  00000000
> > >        0000000000000000  0000000000000000           0     0     0
> > > readelf: Warning: [ 1]: Link field (0) should index a string section.
> > >   [ 1] .dynamic          DYNAMIC          0000000000000000  00000078
> > >        0000000000000030  0000000000000010           0     0     0
> > > readelf: Warning: [ 2]: Link field (0) should index a string section.
> > >   [ 2] .dynsym           DYNSYM           0000000000000100  000000a8
> > >        0000000000000001  0000000000000018   A       0     1     0
> > >   [ 3] .strtab           STRTAB           0000000000000000  000000a9
> > >        0000000000000001  0000000000000000           0     0     1
> > >   [ 4] .shstrtab         STRTAB           0000000000000000  000000aa
> > >        0000000000000024  0000000000000000           0     0     1
> > > ```
> > > 
> > > (Note: .dynsym has sh_entsize == 0x18. It is a correct value set by yaml2obj
> > > implicitly. The size is broken as expected and == 0x1).
> > > 
> > > I have following outputs:
> > > 
> > > **Case 1:**
> > > 
> > > ```
> > > umb at ubuntu:~/tests/121$ readelf --symbols test.o
> > > readelf: Error: Section .dynsym has an invalid sh_entsize of 0x18
> > > 
> > > Symbol table '.dynsym' contains 0 entries:
> > >    Num:    Value          Size Type    Bind   Vis      Ndx Name
> > > readelf: Error: Section .dynsym has an invalid sh_entsize of 0x18
> > > ```
> > > 
> > > **Case 2:**
> > > ```
> > > umb at ubuntu:~/tests/121$ readelf --symbols test.o -D
> > > readelf: Error: Section .dynsym has an invalid sh_entsize of 0x18
> > > 
> > > Dynamic symbol information is not available for displaying symbols.
> > > ```
> > > 
> > > Well... there are many broken things here I see:
> > > 1) ".dynsym has an invalid sh_entsize" it is not true. The `sh_size` is broken, but `0x18` is a valid value for `sh_entsize`.
> > > 2) Duplicate error messages reported in the case #1. It reminds me about plans to switch to `reportUniqueWarning`.
> > >    Perhaps it is a time to do this.
> > > 3) The different output. I.e. in the first case we see the dynamic symbols section header,
> > >    in the second we don't have it. Looks a bit inconsistent to me. In both cases we dump symbols.
> > > 4) Error messages started from the upper-case. I see we have the same thing in "llvm-readobj\ELF\broken-group.test",
> > >    but our other tests use the lower case form (as expected, I believe).
> > > 5) Error? Why not just a warning...
> > > 
> > That does seem very broken in various ways. Perhaps worth reporting to GNU? Either way, I don't think we need to follow any of that behaviour.
> Yes, but in the context of the `DT_SYMENT`:
> I think I do not understand why this value is usefull/was introduced.
> E.g. LLD has the following line `addInt(DT_SYMENT, sizeof(Elf_Sym));`.
> I'd expect other linkers to have something the same.
> So seems we have 2 possible cases: 32/64 versions of a symbol. LLD works really well as we know...
> Is it possible that something else can be valid as a value for the `DT_SYMENT`?
> I just expect that such things are baked too hard and perhaps we do not want to
> care about `DT_SYMENT`? Or may be should just check and report if it is `!= sizeof(Elf_Sym)`?
I guess a warning if the DT_SYMENT does not match the symbol size of the appropriate ELF variety is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73484