[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

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

4050

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

[zatrazz]

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

353–359

Ack, I will fix it.

llvm/tools/llvm-readobj/ELFDumper.cpp

4050

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

4053

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

4057

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

4051

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

13

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.

17

Please align.

[zatrazz]

llvm/lib/ObjectYAML/ELFYAML.cpp

1045 ↗

(On Diff #311590)

Right.

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

17

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

13

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

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

44–47

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

44–47

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/test/tools/llvm-readobj/ELF/aarch64-symbols-stother.test

+## Check that we are able to dump AArch64 STO_* flags correctly when dumping symbols.
+
+# RUN: yaml2obj %s -o %t.o
+# RUN: llvm-readobj --symbols %t.o | FileCheck %s --check-prefix=LLVM
+# RUN: llvm-readelf --symbols %t.o | FileCheck %s --check-prefix=GNU
+
+# LLVM:      Name: foo1
+# LLVM:      Other [ (0x80)
+# LLVM-NEXT: ]
+# LLVM:      Name: foo2
+# LLVM:      Other [ (0xC0)
+# LLVM-NEXT: ]
+
+# GNU:      Symbol table '.symtab' contains 3 entries:
+# GNU:      1: 0000000000000000 0 NOTYPE LOCAL DEFAULT [VARIANT_PCS]      UND foo1
+# GNU-NEXT: 2: 0000000000000000 0 NOTYPE LOCAL DEFAULT [VARIANT_PCS | 40] UND foo2
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_AARCH64
+Symbols:
+  - Name: foo1
+    Other: [ STO_AARCH64_VARIANT_PCS ]
+  - Name: foo2
+    Other: [ STO_AARCH64_VARIANT_PCS, 0x40 ]

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

ProgramHeaders:
   - Type:     PT_DYNAMIC
     FirstSec: .dynamic
     LastSec:  .dynamic
 
 ## Case 5: Test that AARCH64 machine-specific tags can be dumped.
 # RUN: yaml2obj --docnum=5 %s -o %t.aarch64
 # RUN: llvm-readobj --dynamic-table %t.aarch64 | FileCheck %s --check-prefix=LLVM-AARCH64
 # RUN: llvm-readelf --dynamic-table %t.aarch64 | FileCheck %s --check-prefix=GNU-AARCH64
 
-# LLVM-AARCH64:     DynamicSection [ (3 entries)
-# LLVM-AARCH64-NEXT:  Tag                Type            Name/Value
-# LLVM-AARCH64-NEXT:  0x0000000070000001 AARCH64_BTI_PLT 1
-# LLVM-AARCH64-NEXT:  0x0000000070000003 AARCH64_PAC_PLT 2
-# LLVM-AARCH64-NEXT:  0x0000000000000000 NULL            0x0
+# LLVM-AARCH64:     DynamicSection [ (4 entries)
+# LLVM-AARCH64-NEXT:  Tag                Type                Name/Value
+# LLVM-AARCH64-NEXT:  0x0000000070000001 AARCH64_BTI_PLT     1
+# LLVM-AARCH64-NEXT:  0x0000000070000003 AARCH64_PAC_PLT     2
+# LLVM-AARCH64-NEXT:  0x0000000070000005 AARCH64_VARIANT_PCS 3
+# LLVM-AARCH64-NEXT:  0x0000000000000000 NULL                0x0
 # LLVM-AARCH64-NEXT:]
 
-# GNU-AARCH64:      Dynamic section at offset {{.*}} contains 3 entries:
-# GNU-AARCH64-NEXT:  Tag                Type              Name/Value
-# GNU-AARCH64-NEXT:  0x0000000070000001 (AARCH64_BTI_PLT) 1
-# GNU-AARCH64-NEXT:  0x0000000070000003 (AARCH64_PAC_PLT) 2
-# GNU-AARCH64-NEXT:  0x0000000000000000 (NULL)            0x0
+# GNU-AARCH64:      Dynamic section at offset {{.*}} contains 4 entries:
+# GNU-AARCH64-NEXT:  Tag                Type                  Name/Value
+# GNU-AARCH64-NEXT:  0x0000000070000001 (AARCH64_BTI_PLT)     1
+# GNU-AARCH64-NEXT:  0x0000000070000003 (AARCH64_PAC_PLT)     2
+# GNU-AARCH64-NEXT:  0x0000000070000005 (AARCH64_VARIANT_PCS) 3
+# GNU-AARCH64-NEXT:  0x0000000000000000 (NULL)                0x0
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_AARCH64
 Sections:
   - Name: .dynamic
     Type: SHT_DYNAMIC
     Entries:
       - Tag:   DT_AARCH64_BTI_PLT
         Value: 1
       - Tag:   DT_AARCH64_PAC_PLT
         Value: 2
+      - Tag:   DT_AARCH64_VARIANT_PCS
+        Value: 3
       - Tag:   DT_NULL
         Value: 0
 ProgramHeaders:
   - Type:     PT_LOAD
     FirstSec: .dynamic
     LastSec:  .dynamic
   - Type:     PT_DYNAMIC
     FirstSec: .dynamic
     LastSec:  .dynamic

llvm/tools/llvm-readobj/ELFDumper.cpp

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);