This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj/elf] - AArch64: Handle AARCH64_VARIANT_PCS
ClosedPublic

Authored by zatrazz on Dec 10 2020, 8:36 AM.

Details

Summary

It mimics the GNU readelf where it prints a [VARIANT_PCS] for symbols
with st_other with STO_AARCH64_VARIANT_PCS.

Diff Detail

Event Timeline

zatrazz created this revision.Dec 10 2020, 8:36 AM
zatrazz requested review of this revision.Dec 10 2020, 8:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2020, 8:36 AM
MaskRay added a subscriber: nsz.Dec 10 2020, 11:59 AM

You may want to update obj2yaml and yaml2obj as well since they need no additional code (see D62596 for an example).

The binutils commit 2301ed1c9af1316b4bad3747d2b03f7d44940f87 does not have a test for [VARIANT_PCS | %x] (@nsz).
I believe currently there is no way to set the other st_other bits with MC so the lack of testing may be fine.
It seems that objdump does not understand STO_AARCH64_VARIANT_PCS.

zatrazz updated this revision to Diff 311177.Dec 11 2020, 5:11 AM

Updated patch based on previous comments.

jhenderson added inline comments.
llvm/test/tools/llvm-readobj/ELF/dynamic-tags-machine-specific.test
353–359

It would be nice if the whitespace were adjusted so that the name/value column all continues to line up. Same goes below.

llvm/tools/llvm-readobj/ELFDumper.cpp
4038

It looks like this new block of code is completely untested?

zatrazz added inline comments.Dec 11 2020, 10:37 AM
llvm/test/tools/llvm-readobj/ELF/dynamic-tags-machine-specific.test
353–359

Ack, I will fix it.

llvm/tools/llvm-readobj/ELFDumper.cpp
4038

It is, but on a subsequent patch (https://reviews.llvm.org/D93045). I will add a standalone patch as well.

zatrazz updated this revision to Diff 311294.Dec 11 2020, 11:55 AM

Updated patch based on previous comments.

grimar added inline comments.Dec 13 2020, 11:46 PM
llvm/tools/llvm-readobj/ELFDumper.cpp
4041

other->Other (llvm-readobj uses upper case naming).

4045

Doesn't seem that this condition is covered by a test?

jhenderson added inline comments.Dec 14 2020, 12:47 AM
llvm/test/tools/llvm-readobj/ELF/AArch64/aarch64-variant_pcs.s
1

I'm interested to hear what @grimar thinks, but I feel like this test would be better written with a YAML input. That will a) allow the test to be tested without AArch64 target support enabled, and b) I feel like yaml2obj gives more precise control over teh symbol details, and c) it is clearer what the inputs actually are.

llvm/tools/llvm-readobj/ELFDumper.cpp
4039

Maybe we should have a test case that shows for non AARCH64 cases, this code doesn't trigger?

grimar added inline comments.Dec 14 2020, 12:54 AM
llvm/test/tools/llvm-readobj/ELF/AArch64/aarch64-variant_pcs.s
1

I agree with all mentioned points.
Using of llvm-mc is almost always excessive for llvm-readobj/elf tests I think.

I guess that with use of YAML + macros this test also could be slightly shorter.

zatrazz updated this revision to Diff 311590.Dec 14 2020, 7:28 AM

Updated patch based on previous comment.

grimar added inline comments.Dec 14 2020, 7:43 AM
llvm/lib/ObjectYAML/ELFYAML.cpp
1045 ↗(On Diff #311590)

This belongs to yaml2obj functionality, not to llvm-readelf/obj and can be fixed separatelly.

I'd suggest either
a) split this to a separate yaml2obj patch and then rebase this one on top
b) just remove this for now and use a numeric unnamed constants (0x80) in the YAML (with a TODO/FIXME note about yaml2obj).

llvm/test/tools/llvm-readobj/ELF/aarch64-symbols-stother.test
12 ↗(On Diff #311590)

This output looks a bit wrong.
See the output from mips-symbols-stother.test:

#      MIPS-LLVM:Name: foo
#      MIPS-LLVM:Other [
# MIPS-LLVM-NEXT:  STO_MIPS_MICROMIPS (0x80)
# MIPS-LLVM-NEXT:  STO_MIPS_OPTIONAL (0x4)
# MIPS-LLVM-NEXT:  STO_MIPS_PIC (0x20)
# MIPS-LLVM-NEXT:  STO_MIPS_PLT (0x8)
# MIPS-LLVM-NEXT:]

#      MIPS-LLVM:Name: bar
#      MIPS-LLVM:Other [
# MIPS-LLVM-NEXT:  STO_MIPS_MIPS16 (0xF0)
# MIPS-LLVM-NEXT:]

I think you need to update LLVMStyle<ELFT>::printSymbol too.

16 ↗(On Diff #311590)

Please align.

zatrazz added inline comments.Dec 14 2020, 9:02 AM
llvm/lib/ObjectYAML/ELFYAML.cpp
1045 ↗(On Diff #311590)

Right.

llvm/test/tools/llvm-readobj/ELF/aarch64-symbols-stother.test
16 ↗(On Diff #311590)

Sigh... sorry about that. I will fix it.

zatrazz updated this revision to Diff 311643.Dec 14 2020, 10:05 AM

Updated patch based on previous comments. It depends on https://reviews.llvm.org/D93235 for YAML STO_AARCH64_VARIANT_PCS support.

grimar added inline comments.Dec 14 2020, 11:48 PM
llvm/test/tools/llvm-readobj/ELF/aarch64-symbols-stother.test
12 ↗(On Diff #311590)

This wasn't addressed?
You did the change in GNUStyle<ELFT>::printSymbol, it is responsible for llvm-readelf output.
If you want to change the llvm-readobj, then you need to update the LLVMStyle<ELFT>::printSymbol too.

jhenderson added inline comments.Dec 15 2020, 1:17 AM
llvm/test/tools/llvm-readobj/ELF/aarch64-symbols-stother.test
28 ↗(On Diff #311643)

Perhaps also worth a case with the visibility bits set too (e.g. STV_HIDDEN)?

zatrazz updated this revision to Diff 311884.Dec 15 2020, 6:25 AM

Updated patch based on previous comments:

  • Added LLVMStyle support.
  • Add symbol visibility tests.
grimar accepted this revision.Dec 15 2020, 6:34 AM

I have no more comments. This LG. Please wait for @jhenderson too.

This revision is now accepted and ready to land.Dec 15 2020, 6:34 AM
MaskRay accepted this revision.Dec 15 2020, 3:03 PM
MaskRay retitled this revision from [llvm-readobj/elf] - AArch64: Handle AARCH64_VARIANT_PCS for GNUStyle to [llvm-readobj/elf] - AArch64: Handle AARCH64_VARIANT_PCS.

One test suggestion. Otherwise, this looks fine.

llvm/test/tools/llvm-readobj/ELF/aarch64-symbols-stother.test
43–46 ↗(On Diff #311884)

The visibility bits aren't flags, they're values, so they can't be combined like you've done here (hence why you end up with STV_PROTECTED in the final output). I think as long as you've got the max visibility value (iSTV_PROTECTED) defined in one of these cases, I think that's all you need (you can then drop the other case).

You might also want a negative case, using STV_PROTECTED, to make sure the STO_AARCH_VARIANT_PCS value isn't used in this case.

Examples suggested with the inline edit.

zatrazz added inline comments.Dec 16 2020, 4:13 AM
llvm/test/tools/llvm-readobj/ELF/aarch64-symbols-stother.test
43–46 ↗(On Diff #311884)

Ack, I will change the tests with your suggestions.