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

[George Rimar via Phabricator via llvm-commits]

grimar marked 2 inline comments as done.
grimar added inline comments.


================
Comment at: test/tools/llvm-readobj/elf-symbol-visibility.test:53
     Other: [ STV_PROTECTED ]
     Binding: STB_GLOBAL
+
----------------
MaskRay wrote:
> Can the `Other: [ 4 ]` test be added here? I think using one test is fine. The additional YAML test does not make it easier to read.
Idea to have 2 tests is to check that we add the additional padding added when we have st_other with non-visibility flags.
I.e. to check the output with --strict-whitespace --match-full-lines before and after adding the padding.
I think it is useful.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3275
+    Fields[5].Str +=
+        " [<st_other: " + to_string(format_hex(Symbol->st_other, 2)) + ">]";
+
----------------
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.


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

https://reviews.llvm.org/D67094