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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73484