[PATCH] D71118: [llvm-readelf/llvm-readobj] - Improved the error reporting in a few method related to versioning.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/elf-versym-invalid.test:256
+    Type:    SHT_GNU_versym
+    Entries: [ 0xFF, 0xFE ]
+DynamicSymbols:
----------------
jhenderson wrote:
> Would it be worth an extra 0xFF entry to show that the warning gets uniqued?
Done.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4136
+        this->dumper()->getSymbolVersionByIndex(Ndx, IsDefault);
+    if (!NameOrErr || NameOrErr->empty()) {
+      if (!NameOrErr) {
----------------
jhenderson wrote:
> grimar wrote:
> > jhenderson wrote:
> > > I'm not sure it's right to say that an empty name is corrupt?
> > That what original code did (the test was introduced in D59877) and I think it is
> > probably reasonable to assume that.
> > Normally we probably never want to have an empty version name for a symbol that has version?
> > Having such an empty version intentionally seems looks highly unlikely.
> > 
> > Spec says: "All other values are used to identify version strings located in one of the other Symbol
> > Version sections. The value itself is not the version associated with the symbol.
> > The string identified by the value defines the version of the symbol."
> > (https://refspecs.linuxfoundation.org/LSB_3.0.0/LSB-PDA/LSB-PDA.junk/symversion.html)
> > It says nothing about the case when the version string is empty, though
> > it sounds like it is assumed that a version exists, i.e is non-empty?
> > 
> > it is also just a bit strange to me to see the output for such case (below is what GNU readelf prints):
> > 
> > ```
> > Symbol table '.dynsym' contains 6 entries:
> >    Num:    Value          Size Type    Bind   Vis      Ndx Name
> >      0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND 
> > ....
> >      2: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND __libc_start_main@ (2)
> > ....
> > Version symbols section '.gnu.version' contains 6 entries:
> >  Addr: 000000000000043e  Offset: 0x00043e  Link: 5 (.dynsym)
> >   000:   0 (*local*)       0 (*local*)       2 ()              0 (*local*)    
> >   004:   0 (*local*)       2 () 
> > ```
> > 
> > It looks like something is just wrong with the symbol `__libc_start_main@` and version 2?
> But shouldn't we just allow it? It simplifies the code and brings us closer to GNU compatibility. Whilst I think it a bit odd, I don't see a strong motivation for handling the empty string differently to other strings. I don't feel strongly about this however.
I do not mind to suggest a follow-up patch to change this behavior if we do not really want it.
(I'd not change this existent behavior in this patch, as it is probably unrelated).


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

https://reviews.llvm.org/D71118