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

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar 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.
----------------
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)`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73484