This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readelf] - Print unknown st_other value if present in GNU output.
ClosedPublic

Authored by grimar on Sep 3 2019, 5:42 AM.

Details

Summary

This is a fix for https://bugs.llvm.org/show_bug.cgi?id=40785.

llvm-readelf does not print the st_value of the symbol when
st_value has any non-visibility bits set.

This patch:

  • Aligns "Ndx" row for the default and a new cases.

(it was 1 space character off for the case when "PROTECTED" visibility was printed)

  • Prints "[<other>: 0x??]" for symbols which has an additional st_other bits set.

In compare with GNU, this logic is a bit simpler and seems to be more consistent.

For MIPS GNU can print named flags, though can't print a mix of them:
0: 00000000 0 NOTYPE LOCAL DEFAULT UND
1: 00000000 0 NOTYPE GLOBAL DEFAULT [OPTIONAL] UND a1
2: 00000000 0 NOTYPE GLOBAL DEFAULT [MIPS PLT] UND a2
3: 00000000 0 NOTYPE GLOBAL DEFAULT [MIPS PIC] UND a3
4: 00000000 0 NOTYPE GLOBAL DEFAULT [MICROMIPS] UND a4
5: 00000000 0 NOTYPE GLOBAL DEFAULT [MIPS16] UND a5
6: 00000000 0 NOTYPE GLOBAL DEFAULT [<other>: c] UND b1
7: 00000000 0 NOTYPE GLOBAL DEFAULT [<other>: 28] UND b2

On PPC64 it can print a localentry value that is encoded in the high bits of st_other
63: 0000000000000850 208 FUNC GLOBAL DEFAULT [<localentry>: 8] 12

We chose to print the raw st_other field, prefixed with '0x'.

Diff Detail

Event Timeline

grimar created this revision.Sep 3 2019, 5:42 AM
MaskRay added inline comments.Sep 3 2019, 10:57 PM
test/tools/llvm-readobj/elf-symbol-visibility.test
51–85

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.

tools/llvm-readobj/ELFDumper.cpp
3276

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)

In compare with GNU, this logic is simpler and seems more consistent. The details about the known differences were listed in the comments on a bug page.

We should summarize what GNU readelf does. IIRC it interprets certain bits for some archs

0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND 
1: 00000000     0 NOTYPE  GLOBAL DEFAULT [OPTIONAL]   UND a1
2: 00000000     0 NOTYPE  GLOBAL DEFAULT [MIPS PLT]   UND a2
3: 00000000     0 NOTYPE  GLOBAL DEFAULT [MIPS PIC]   UND a3
4: 00000000     0 NOTYPE  GLOBAL DEFAULT [MICROMIPS]   UND a4
5: 00000000     0 NOTYPE  GLOBAL DEFAULT [MIPS16]   UND a5
6: 00000000     0 NOTYPE  GLOBAL DEFAULT [<other>: c]   UND b1
7: 00000000     0 NOTYPE  GLOBAL DEFAULT [<other>: 28]   UND b2

On PPC64, it just leaves the field uninterpreted. We choose to print the raw st_other field, prefixed with 0x.

grimar marked 2 inline comments as done.Sep 4 2019, 1:23 AM
grimar added inline comments.
test/tools/llvm-readobj/elf-symbol-visibility.test
51–85

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.

tools/llvm-readobj/ELFDumper.cpp
3276

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.

grimar edited the summary of this revision. (Show Details)Sep 4 2019, 1:27 AM
MaskRay added inline comments.Sep 4 2019, 2:28 AM
test/tools/llvm-readobj/elf-symbol-visibility.test
51–85

Sorry, I didn't notice that the headers are different. I agree that we should have 2 tests.

tools/llvm-readobj/ELFDumper.cpp
351

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

llvm::is_contained

361

OtherBitsUsed may be confusing. I am not sure what the best is. One suggestion is NonVisibilityBitsUsed. Let's see what others think.

grimar edited the summary of this revision. (Show Details)Sep 4 2019, 3:10 AM
jhenderson added inline comments.Sep 4 2019, 9:38 AM
test/tools/llvm-readobj/elf-symbol-visibility.test
5

a -> the

27–32

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.

tools/llvm-readobj/ELFDumper.cpp
351

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.

361

NonVisibilityBitsUsed sounds good to me.

594–595

Perhaps worth naming this last field: bool /*OtherBitsUsed*/

3257

What's this change about?

3276

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?).

5320–5321

Same as earlier comment.

grimar updated this revision to Diff 218881.Sep 5 2019, 3:42 AM
grimar marked 18 inline comments as done.
  • Addressed review comments.
test/tools/llvm-readobj/elf-symbol-visibility.test
27–32

OK.

tools/llvm-readobj/ELFDumper.cpp
355

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.

3257

Before this patch "Ndx" row was 1 space off for the case when the "PROTECTED" visibility (has the longest spelling) was printed.

3276

2:1, I renamed to "other".

MaskRay accepted this revision.Sep 5 2019, 4:01 AM
This revision is now accepted and ready to land.Sep 5 2019, 4:01 AM
jhenderson added inline comments.Sep 5 2019, 5:56 AM
test/tools/llvm-readobj/elf-symbol-visibility.test
27–32

Maybe keep the names though, to self-document and make sure the right symbols match up :)

tools/llvm-readobj/ELFDumper.cpp
3257

Okay. Does the changed format match up with GNU readelf?

grimar updated this revision to Diff 219052.Sep 6 2019, 2:44 AM
grimar marked 2 inline comments as done.
  • Addressed review comments.
tools/llvm-readobj/ELFDumper.cpp
3257

Nope. And wasn't before. GNU output is off for Ndx and Name:

Symbol table '.symtab' contains 5 entries:
   Num:    Value  Size Type    Bind   Vis      Ndx Name
     0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND 
     1: 00000000     0 NOTYPE  GLOBAL DEFAULT  UND default
     2: 00000000     0 NOTYPE  GLOBAL INTERNAL  UND internal
     3: 00000000     0 NOTYPE  GLOBAL HIDDEN   UND hidden
     4: 00000000     0 NOTYPE  GLOBAL PROTECTED  UND protected

We do not need to try be bug compatible here it seems, since we improve the alignment in favor of readability.

Our output is perfect now:

Symbol table '.symtab' contains 5 entries:
   Num:    Value  Size Type    Bind   Vis       Ndx Name
     0: 00000000     0 NOTYPE  LOCAL  DEFAULT   UND
     1: 00000000     0 NOTYPE  GLOBAL DEFAULT   UND default
     2: 00000000     0 NOTYPE  GLOBAL INTERNAL  UND internal
     3: 00000000     0 NOTYPE  GLOBAL HIDDEN    UND hidden
     4: 00000000     0 NOTYPE  GLOBAL PROTECTED UND protected
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2019, 6:06 AM