[PATCH] D67094: [llvm-readelf] - Print unknown st_other value if present in GNU output.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: test/tools/llvm-readobj/elf-symbol-visibility.test:28-33
+# GNU-NEXT:   Num:    Value  Size Type    Bind   Vis       Ndx Name
+# GNU-NEXT:     0: 00000000     0 NOTYPE  LOCAL  DEFAULT   UND 
+# GNU-NEXT:     1: 00000000     0 NOTYPE  GLOBAL DEFAULT   UND default
+# GNU-NEXT:     2: 00000000     0 NOTYPE  GLOBAL INTERNAL  UND internal
+# GNU-NEXT:     3: 00000000     0 NOTYPE  GLOBAL HIDDEN    UND hidden
+# GNU-NEXT:     4: 00000000     0 NOTYPE  GLOBAL PROTECTED UND protected
----------------
jhenderson wrote:
> I don't mind too much, but maybe you could simplify this whilst still preserving the core of the test by removing `--match-full-lines`. You only really care about what's going around the Visibility columns (and the spacing to the Ndx column), so everything else is a bit superfluous (e.g. the Type and Value columns).
> 
> Same comment applies below to the second test case.
OK.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:355
+  // non-visibility bits were set for at least one symbol.
+  bool OtherBitsUsed = llvm::find_if(Syms, [](const Elf_Sym &S) {
+                         return S.st_other & ~0x3;
----------------
MaskRay wrote:
> llvm::is_contained
I do not think I can use `llvm::is_contained` here? I need to find '`Elf_Sym` which has `Sym.st_other & ~0x3 != 0`.
`llvm::is_contained` would help if I needed to find a particular `Elf_Sym`.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3256
   Field Fields[8] = {0,         8,         17 + Bias, 23 + Bias,
-                     31 + Bias, 38 + Bias, 47 + Bias, 51 + Bias};
+                     31 + Bias, 38 + Bias, 48 + Bias, 51 + Bias};
   Fields[0].Str = to_string(format_decimal(Idx++, 6)) + ":";
----------------
jhenderson wrote:
> What's this change about?
Before this patch "Ndx" row was 1 space off for the case when the "PROTECTED" visibility (has the longest spelling) was printed.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3275
+    Fields[5].Str +=
+        " [<st_other: " + to_string(format_hex(Symbol->st_other, 2)) + ">]";
+
----------------
jhenderson wrote:
> grimar wrote:
> > MaskRay wrote:
> > > The description says:
> > > 
> > > > Prints "[<other>: 0x??]" for symbols which has an additional st_other bits set.
> > > 
> > > However, the code actually uses `[<st_other: ...>]` (I would favor `[<other:...>]` because it is shorter, if you don't think that confusing)
> > We print it under a row named "Vis". Probably it is not so clear what is "other" when we print the visibility.
> > It is also probably not clear if "other" contain the whole st_other value or st_other & ~0x3.
> > 
> > So "st_other" IMO is a bit better. Lets see what others think.
> I don't have a strong feeling either way. I think it's rare enough that it won't matter. `<other:...>` is fine by me as it is a bit shorter, and I don't think it'll be that confusing (after all how would the value work if it didn't include the visibility bits?).
`2:1`, I renamed to "other".


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

https://reviews.llvm.org/D67094