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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: test/tools/llvm-readobj/elf-symbol-visibility.test:53
     Other: [ STV_PROTECTED ]
     Binding: STB_GLOBAL
+
----------------
grimar wrote:
> 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.
Sorry, I didn't notice that the headers are different. I agree that we should have 2 tests.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:351
+
+  // The st_other field has 2 logical parts. One holds visibility (STV_* bits)
+  // and another holds other values (STO_* bits usually, but also sometimes
----------------
I think James will suggest a better wording here... ELFYAML.cpp has such comments:

```
  // Symbol's Other field is a bit special. It is usually a field that
  // represents st_other and holds the symbol visibility. However, on some
  // platforms, it can contain bit fields and regular values, or even sometimes a
  // crazy mix of them (see comments for NormalizedOther). Because of this, we
  // need special handling.
```


================
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;
----------------
llvm::is_contained


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:362
+    ELFDumperStyle->printSymbol(Obj, &Sym, Syms.begin(), StrTable, IsDynamic,
+                                OtherBitsUsed);
 }
----------------
`OtherBitsUsed` may be confusing. I am not sure what the best is. One suggestion is `NonVisibilityBitsUsed`. Let's see what others think.


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

https://reviews.llvm.org/D67094