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

[zatrazz]

Summary

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

[MaskRay]

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 patch based on previous comments.

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/dynamic-tags-machine-specific.test

355–359 ↗

(On Diff #311177)

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]

llvm/test/tools/llvm-readobj/ELF/dynamic-tags-machine-specific.test

355–359 ↗

(On Diff #311177)

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 patch based on previous comments.

[grimar]

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]

llvm/test/tools/llvm-readobj/ELF/AArch64/aarch64-variant_pcs.s

1 ↗

(On Diff #311294)

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]

llvm/test/tools/llvm-readobj/ELF/AArch64/aarch64-variant_pcs.s

1 ↗

(On Diff #311294)

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 patch based on previous comment.

[grimar]

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]

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 patch based on previous comments. It depends on https://reviews.llvm.org/D93235 for YAML STO_AARCH64_VARIANT_PCS support.

[grimar]

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]

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 patch based on previous comments:

• Added LLVMStyle support.
• Add symbol visibility tests.

[grimar]

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

[jhenderson]

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]

llvm/test/tools/llvm-readobj/ELF/aarch64-symbols-stother.test

43–46 ↗

(On Diff #311884)

Ack, I will change the tests with your suggestions.

llvm/include/llvm/BinaryFormat/DynamicTags.def

 DYNAMIC_TAG(VERDEFNUM, 0X6FFFFFFD)  // The number of entries in DT_VERDEF.
 DYNAMIC_TAG(VERNEED, 0X6FFFFFFE)    // The address of the version dependency
                                     // table.
 DYNAMIC_TAG(VERNEEDNUM, 0X6FFFFFFF) // The number of entries in DT_VERNEED.
 
 // AArch64 specific dynamic table entries
 AARCH64_DYNAMIC_TAG(AARCH64_BTI_PLT, 0x70000001)
 AARCH64_DYNAMIC_TAG(AARCH64_PAC_PLT, 0x70000003)
+AARCH64_DYNAMIC_TAG(AARCH64_VARIANT_PCS, 0x70000005)
 
 // Hexagon specific dynamic table entries
 HEXAGON_DYNAMIC_TAG(HEXAGON_SYMSZ, 0x70000000)
 HEXAGON_DYNAMIC_TAG(HEXAGON_VER, 0x70000001)
 HEXAGON_DYNAMIC_TAG(HEXAGON_PLT, 0x70000002)
 
 // Mips specific dynamic table entry tags.
 

llvm/tools/llvm-readobj/ELFDumper.cpp

 //===- ELFDumper.cpp - ELF-specific dumper --------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 ///
std::string ELFDumper<ELFT>::getDynamicEntry(uint64_t Type,
   };
 
   // Handle custom printing of architecture specific tags
   switch (Obj.getHeader().e_machine) {
   case EM_AARCH64:
     switch (Type) {
     case DT_AARCH64_BTI_PLT:
     case DT_AARCH64_PAC_PLT:
+    case DT_AARCH64_VARIANT_PCS:
       return std::to_string(Value);
     default:
       break;
     }
     break;
   case EM_HEXAGON:
     switch (Type) {
     case DT_HEXAGON_VER:
if (this->Obj.getHeader().e_machine == ELF::EM_AMDGPU &&
     Fields[3].Str = printEnum(SymbolType, makeArrayRef(AMDGPUSymbolTypes));
   else
     Fields[3].Str = printEnum(SymbolType, makeArrayRef(ElfSymbolTypes));
 
   Fields[4].Str =
       printEnum(Symbol.getBinding(), makeArrayRef(ElfSymbolBindings));
   Fields[5].Str =
       printEnum(Symbol.getVisibility(), makeArrayRef(ElfSymbolVisibilities));
-  if (Symbol.st_other & ~0x3)
+
+  if (Symbol.st_other & ~0x3) {
+    if (this->Obj.getHeader().e_machine == ELF::EM_AARCH64) {
+      uint8_t other = Symbol.st_other & ~0x3;
+      if (other & STO_AARCH64_VARIANT_PCS) {
+        other &= ~STO_AARCH64_VARIANT_PCS;
+        Fields[5].Str += " [VARIANT_PCS";
+        if (other != 0)
+          Fields[5].Str.append(" | " + to_hexString(other, false));
+        Fields[5].Str.append("]");
+      }
+    } else {
       Fields[5].Str +=
           " [<other: " + to_string(format_hex(Symbol.st_other, 2)) + ">]";
+    }
+  }
 
   Fields[6].Column += NonVisibilityBitsUsed ? 13 : 0;
   Fields[6].Str = getSymbolSectionNdx(Symbol, SymIndex);
 
   Fields[7].Str =
       this->dumper().getFullSymbolName(Symbol, SymIndex, StrTable, IsDynamic);
   for (const Field &Entry : Fields)
     printField(Entry);