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

[Xing via Phabricator via llvm-commits]

Higuoxing marked an inline comment as done.
Higuoxing added inline comments.


================
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);
----------------
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)`. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59186