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


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

https://reviews.llvm.org/D71118