[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: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
----------------
grimar wrote:
> 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.
Maybe keep the names though, to self-document and make sure the right symbols match up :)


================
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)) + ":";
----------------
grimar wrote:
> 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.
Okay. Does the changed format match up with GNU readelf?


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

https://reviews.llvm.org/D67094