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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/elf-versym-invalid.test:256
+    Type:    SHT_GNU_versym
+    Entries: [ 0xFF, 0xFE ]
+DynamicSymbols:
----------------
grimar wrote:
> jhenderson wrote:
> > Would it be worth an extra 0xFF entry to show that the warning gets uniqued?
> Done.
Will you need an --implicit-check-not="warning:" or equivalent somewhere to actually detect that the warning has actually been uniqued?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4136
+        this->dumper()->getSymbolVersionByIndex(Ndx, IsDefault);
+    if (!NameOrErr || NameOrErr->empty()) {
+      if (!NameOrErr) {
----------------
grimar wrote:
> 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).
Sounds reasonable.


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

https://reviews.llvm.org/D71118