[aarch64/mac] Correctly disassemble @TLVPPAGE(OFF) relocs

[thakis]

Summary

llvm-otool -tV foo.o and llvm-objdump --macho -d foo.o would
previously fail on object files containing @TLVPPAGE or @TLVPPAGEOFF relocs.

Move llvm-objdump-specific test from
llvm/test/MC/AArch64/arm64-tls-modifiers-darwin.s to new
llvm/test/tools/llvm-objdump/MachO/disassemble-arm64-tlv-modifers.test
and put test for this fix to that new file.

Fixes PR52356.

[thakis]

maskray, would you be comfortable reviewing this?

[MaskRay]

llvm-objdump --macho -d can reproduce the issue as well. Perhaps perfer llvm-objdump which is used more frequently in tests.

The disassembly logic appears to be llvm/tools/llvm-objdump/MachODump.cpp specific.
Perhaps a better place is llvm/test/tools/llvm-objdump/MachO/ (may need to duplicate the test a bit)

llvm/test/MC/AArch64/arm64-tls-modifiers-darwin.s

11

If you switch to llvm-objdump --macho -d --no-show-raw-insn, you can omit the leading addresses and use CHECK-DIS-NEXT: instead.

[thakis]

move test

[thakis]

Thanks! I moved the test over.

llvm-otool -tV is basically the same as llvm-objdump --macho -d --no-show-raw-insn and shorter (cf parseOtoolOptions() in llvm/tools/llvm-objdump/llvm-objdump.cpp -- absence of -j is --no-show-raw-insn), so I kept the otool invocation. If you feel strongly about changing it to llvm-objdump, I'll change it.

[MaskRay]

In D112843#3118529, @thakis wrote:

Thanks! I moved the test over.

llvm-otool -tV is basically the same as llvm-objdump --macho -d --no-show-raw-insn and shorter (cf parseOtoolOptions() in llvm/tools/llvm-objdump/llvm-objdump.cpp -- absence of -j is --no-show-raw-insn), so I kept the otool invocation. If you feel strongly about changing it to llvm-objdump, I'll change it.

Not strong. I don't know which of llvm-otool and llvm-objdump should be canonical now...

llvm/test/tools/llvm-objdump/MachO/disassemble-arm64-tlv-modifers.test

1 ↗

(On Diff #385816)

For newer tests in many test/tools directories (and lld/test), comments start with two comment markers, i.e. ;;

7 ↗

(On Diff #385816)

Prefer using llvm-mc to create the object file. It does duplicate a bit of work which is tested in test/MC, but it makes the test much easier to update. For test/tools directories we try very hard to avoid prebuilt binaries.

12 ↗

(On Diff #385816)

May be better checking the assembly as well to show that the relocation is attached to the correct instruction.
Then you'll need -NEXT:

[lhames]

Late to the party, sorry.

MaskRay's feedback covers all my questions, so as long as that's addressed -- LGTM. Thanks for working on this Nico! (and for the review MaskRay)

[thakis]

(will update in a bit)

llvm/test/tools/llvm-objdump/MachO/disassemble-arm64-tlv-modifers.test

7 ↗

(On Diff #385816)

Will do in a bit. _All_ tests in llvm/test/tools/llvm-objdump/MachO currently use checked in binaries as far as I can tell, that's why I went with this.

12 ↗

(On Diff #385816)

Will do, but this part is just moved over from the other test.

[thakis]

tweak test

[MaskRay]

LGTM. @jhenderson usually wants to review llvm-objdump changes.

[jhenderson]

I've got nothing else really to add. LGTM, with or without my test comment addressed.

llvm/test/tools/llvm-objdump/MachO/disassemble-arm64-tlv-modifers.s

8–10

Optional, but you could slightly simplify the prefixes by dropping "CHECK-". Also, I have a personal preference for the inline edit for continuing RUN lines. The logic is that the indentation makes it clear that the second line is a continuation, whilst the trailing | makes it clear that the first line is continued - again, entirely optional though.

[thakis]

llvm/test/tools/llvm-objdump/MachO/disassemble-arm64-tlv-modifers.s

8–10

Dropped the CHECK prefix, kept the indentation :) Thanks!

llvm/lib/Target/AArch64/Disassembler/AArch64ExternalSymbolizer.cpp

case LLVMDisassembler_VariantKind_ARM64_PAGE:
     return MCSymbolRefExpr::VK_PAGE;
   case LLVMDisassembler_VariantKind_ARM64_PAGEOFF:
     return MCSymbolRefExpr::VK_PAGEOFF;
   case LLVMDisassembler_VariantKind_ARM64_GOTPAGE:
     return MCSymbolRefExpr::VK_GOTPAGE;
   case LLVMDisassembler_VariantKind_ARM64_GOTPAGEOFF:
     return MCSymbolRefExpr::VK_GOTPAGEOFF;
   case LLVMDisassembler_VariantKind_ARM64_TLVP:
+    return MCSymbolRefExpr::VK_TLVPPAGE;
   case LLVMDisassembler_VariantKind_ARM64_TLVOFF:
+    return MCSymbolRefExpr::VK_TLVPPAGEOFF;
   default:
     llvm_unreachable("bad LLVMDisassembler_VariantKind");
   }
 }
 
 /// tryAddingSymbolicOperand - tryAddingSymbolicOperand trys to add a symbolic
 /// operand in place of the immediate Value in the MCInst.  The immediate
 /// Value has not had any PC adjustment made by the caller. If the instruction

llvm/test/MC/AArch64/arm64-tls-modifiers-darwin.s

 ; RUN: llvm-mc -triple=arm64-apple-ios7.0 %s -o - | FileCheck %s
-; RUN: llvm-mc -triple=arm64-apple-ios7.0 -filetype=obj %s -o - | llvm-objdump -r - | FileCheck %s --check-prefix=CHECK-OBJ
 
         adrp x2, _var@TLVPPAGE
         ldr x0, [x15, _var@TLVPPAGEOFF]
         add x30, x0, _var@TLVPPAGEOFF
 ; CHECK: adrp x2, _var@TLVPPAG
 ; CHECK: ldr x0, [x15, _var@TLVPPAGEOFF]
 ; CHECK: add x30, x0, _var@TLVPPAGEOFF
-
-; CHECK-OBJ: 8 ARM64_RELOC_TLVP_LOAD_PAGEOFF12 _var
-; CHECK-OBJ: 4 ARM64_RELOC_TLVP_LOAD_PAGEOFF12 _var
-; CHECK-OBJ: 0 ARM64_RELOC_TLVP_LOAD_PAGE21 _var

llvm/test/tools/llvm-objdump/MachO/disassemble-arm64-tlv-modifers.s

+; REQUIRES: aarch64-registered-target
+; RUN: llvm-mc -triple=arm64-apple-ios7.0 -filetype=obj %s -o %t.o
+
+adrp x2, _var@TLVPPAGE
+ldr x0, [x15, _var@TLVPPAGEOFF]
+add x30, x0, _var@TLVPPAGEOFF
+
+; RUN: llvm-objdump -rd %t.o | FileCheck %s --check-prefix=OBJDUMP
+; RUN: llvm-objdump --macho -d --full-leading-addr --no-show-raw-insn %t.o \
+; RUN:     | FileCheck %s --check-prefix=MACHO
+
+; OBJDUMP:      adrp x2, 0x0
+; OBJDUMP-NEXT:     0: ARM64_RELOC_TLVP_LOAD_PAGE21 _var
+; OBJDUMP-NEXT: ldr x0, [x15]
+; OBJDUMP-NEXT:     4: ARM64_RELOC_TLVP_LOAD_PAGEOFF12 _var
+; OBJDUMP-NEXT: add x30, x0, #0
+; OBJDUMP-NEXT:     8: ARM64_RELOC_TLVP_LOAD_PAGEOFF12 _var
+
+; MACHO:      0000000000000000 adrp x2, _var@TLVPPAGE
+; MACHO-NEXT: 0000000000000004 ldr x0, [x15, _var@TLVPPAGEOFF]
+; MACHO-NEXT: 0000000000000008 add x30, x0, _var@TLVPPAGEOFF