[PATCH] D59186: [llvm-readobj] Separate `Symbol Version` dumpers into `LLVM style` and `GNU style`

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-readobj/elf-versioninfo.test:11
+LLVM-VERDEF: Version symbols {
+LLVM-VERDEF-NEXT:   Section Name: .gnu.version (20)
+LLVM-VERDEF-NEXT:   Address: 0x24C
----------------
Maybe you should make one of the symbols in this section have a version > 0x7fff, e.g. 0xffff?


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:4495
+        Dumper->getFullSymbolName(&Sym, StrTable, true /* IsDynamic */);
+    W.printNumber("Version", Versym->vs_index & VERSYM_VERSION);
+    W.printString("Name", FullSymbolName);
----------------
Higuoxing wrote:
> jhenderson wrote:
> > Higuoxing wrote:
> > > The version of a symbol in `.gnu.version` should be calculated using `Versym->vs_index & VERSYM_VERSION (0x7fff)`. Before this patch, we use `*VersymBuf` as its version. I think it's not a right way, though it does the right thing in most time.
> > I wasn't able to find any documentation for this outside of code, but I see that it is already used elsewhere in this file in a similar manner, and I found other examples of it in code elsewhere, so this looks fine to me.
> > 
> > I'm not quite sure what you mean by:
> > > I think it's not a right way, though it does the right thing in most time.
> > Which way don't you think is right? What conditions does it do the right thin most of the time?
> > Which way don't you think is right? What conditions does it do the right thin most of the time?
> I think using `*VersymBuf` is not a good way to indicate the symbol version, and we should use `Versym->vs_index & VERSYM_VERSION`. Usually, the version of a symbol should be a number that not so big. So, `*VersymBuf (1 byte)` should be equal to `Versym->vs_index & VERSYM_VERSION (2 bytes)`. 
Right, gotcha. LGTM. In fact, it is wise to not do *VersymBuf, because that won't take account of any Endian issues.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59186