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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-readobj/elf-symbol-visibility.test:5
+## Check how we dump symbols when they have only STV_* bits set for st_other.
+## (This is a most common case).
+
----------------
a -> the


================
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
----------------
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.


================
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
----------------
MaskRay wrote:
> 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.
> ```
I do have some suggestions:

"One holds visibility (STV_* bits)" etc -> "The first two bits hold the symbol visibility (STV_*)" and the remainder hold other platform-specific values."

I'd delete the entire STO_* brackets bit, because the elf gABI doesn't officially support anything defined in those bits (it certainly doesn't have an STO_* enum), so the meaning is entirely platform dependent.


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


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:595
   void printSymbol(const ELFO *Obj, const Elf_Sym *Symbol, const Elf_Sym *First,
-                   StringRef StrTable, bool IsDynamic) override;
+                   StringRef StrTable, bool IsDynamic, bool) override;
   void printProgramHeaders(const ELFO *Obj);
----------------
Perhaps worth naming this last field: `bool /*OtherBitsUsed*/`


================
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)) + ":";
----------------
What's this change about?


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


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:5319
                                   const Elf_Sym *First, StringRef StrTable,
-                                  bool IsDynamic) {
+                                  bool IsDynamic, bool) {
   unsigned SectionIndex = 0;
----------------
Same as earlier comment.


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

https://reviews.llvm.org/D67094