[AArch64] [COFF] Properly produce cross-section relative relocations

[mstorsjo]

Summary

This fixes breakage on Windows/ARM64 after D94355.

Modelled after the corresponding code for X86; not entirely familiar
with those aspects of that layer otherwise.

[mstorsjo]

Ping

[rnk]

lgtm, although I don't have deep expertise to do this with great confidence.

[mstorsjo]

In D99572#2686887, @rnk wrote:

lgtm, although I don't have deep expertise to do this with great confidence.

Yeah it's a bit nonobvious code, but at least it's essentially equivalent to what we have on X86 (which I'd assume has been a fair bit more battle tested than these arch/OS combos).

[mstorsjo]

In D99572#2686970, @mstorsjo wrote:

In D99572#2686887, @rnk wrote:

lgtm, although I don't have deep expertise to do this with great confidence.

Yeah it's a bit nonobvious code, but at least it's essentially equivalent to what we have on X86 (which I'd assume has been a fair bit more battle tested than these arch/OS combos).

Just for reference, these codepaths seems to originate in ed16477cb9f907bd3525ae99d787c5d448f8dc0b, and was refactored into its current place in 1da4529b15ed80e5d6c0b21d3ac46538fbcad87f and touched a bit more in 58173b97209bfc907dd0b47ecfc5ab7c1fe1f036.

[thakis]

Sounds like the commit that turned this on by default broke Edge on win/arm64: https://bugs.chromium.org/p/chromium/issues/detail?id=1204788

I'm guessing we'll get a standalone repro for that in time, but to save others time tracking down the bug from the enablement, maybe you want to undo it for now?

(Not this change here, but https://github.com/llvm/llvm-project/commit/57b259a852a6383880f5d0875d848420bb3c2945 )

[richard.townsend.arm]

Bug opened to track the Windows arm64 thing (only a full reproducer available at the moment). https://bugs.llvm.org/show_bug.cgi?id=50227

[mstorsjo]

In D99572#2738593, @thakis wrote:

Sounds like the commit that turned this on by default broke Edge on win/arm64: https://bugs.chromium.org/p/chromium/issues/detail?id=1204788

I'm guessing we'll get a standalone repro for that in time, but to save others time tracking down the bug from the enablement, maybe you want to undo it for now?

(Not this change here, but https://github.com/llvm/llvm-project/commit/57b259a852a6383880f5d0875d848420bb3c2945 )

Ok, reverted it for now. I'd be interested to hear once you're able to pin down the issue closer, whether it's an issue with the relative relocations themselves, or with the rewrite done by the pass.

[mstorsjo]

In D99572#2738626, @mstorsjo wrote:

In D99572#2738593, @thakis wrote:

Sounds like the commit that turned this on by default broke Edge on win/arm64: https://bugs.chromium.org/p/chromium/issues/detail?id=1204788

I'm guessing we'll get a standalone repro for that in time, but to save others time tracking down the bug from the enablement, maybe you want to undo it for now?

(Not this change here, but https://github.com/llvm/llvm-project/commit/57b259a852a6383880f5d0875d848420bb3c2945 )

Ok, reverted it for now. I'd be interested to hear once you're able to pin down the issue closer, whether it's an issue with the relative relocations themselves, or with the rewrite done by the pass.

Also CC @gulfem regarding the pass itself.

llvm/lib/Target/AArch64/MCTargetDesc/AArch64WinCOFFObjectWriter.cpp

public:
   bool recordRelocation(const MCFixup &) const override;
 };
 
 } // end anonymous namespace
 
 unsigned AArch64WinCOFFObjectWriter::getRelocType(
     MCContext &Ctx, const MCValue &Target, const MCFixup &Fixup,
     bool IsCrossSection, const MCAsmBackend &MAB) const {
+  unsigned FixupKind = Fixup.getKind();
+  if (IsCrossSection) {
+    if (FixupKind != FK_Data_4) {
+      Ctx.reportError(Fixup.getLoc(), "Cannot represent this expression");
+      return COFF::IMAGE_REL_ARM64_ADDR32;
+    }
+    FixupKind = FK_PCRel_4;
+  }
+
   auto Modifier = Target.isAbsolute() ? MCSymbolRefExpr::VK_None
                                       : Target.getSymA()->getKind();
   const MCExpr *Expr = Fixup.getValue();
 
   if (const AArch64MCExpr *A64E = dyn_cast<AArch64MCExpr>(Expr)) {
     AArch64MCExpr::VariantKind RefKind = A64E->getKind();
     switch (AArch64MCExpr::getSymbolLoc(RefKind)) {
     case AArch64MCExpr::VK_ABS:
     case AArch64MCExpr::VK_SECREL:
       // Supported
       break;
     default:
       Ctx.reportError(Fixup.getLoc(), "relocation variant " +
                                           A64E->getVariantKindName() +
                                           " unsupported on COFF targets");
       return COFF::IMAGE_REL_ARM64_ABSOLUTE; // Dummy return value
     }
   }
 
-  switch (static_cast<unsigned>(Fixup.getKind())) {
+  switch (FixupKind) {
   default: {
     if (const AArch64MCExpr *A64E = dyn_cast<AArch64MCExpr>(Expr)) {
       Ctx.reportError(Fixup.getLoc(), "relocation type " +
                                           A64E->getVariantKindName() +
                                           " unsupported on COFF targets");
     } else {
       const MCFixupKindInfo &Info = MAB.getFixupKindInfo(Fixup.getKind());
       Ctx.reportError(Fixup.getLoc(), Twine("relocation type ") + Info.Name +
                                           " unsupported on COFF targets");
     }
     return COFF::IMAGE_REL_ARM64_ABSOLUTE; // Dummy return value
   }
 
+  case FK_PCRel_4:
+    return COFF::IMAGE_REL_ARM64_REL32;
+
   case FK_Data_4:
     switch (Modifier) {
     default:
       return COFF::IMAGE_REL_ARM64_ADDR32;
     case MCSymbolRefExpr::VK_COFF_IMGREL32:
       return COFF::IMAGE_REL_ARM64_ADDR32NB;
     case MCSymbolRefExpr::VK_SECREL:
       return COFF::IMAGE_REL_ARM64_SECREL;

llvm/test/MC/AArch64/coff-relocations-diags.s

// RUN: not llvm-mc -triple aarch64-win32 -filetype=obj %s -o /dev/null 2>&1 | FileCheck %s
   // CHECK: error: relocation variant :dtprel_hi12: unsupported on COFF targets
   // CHECK-NEXT: add x0, x0, #:dtprel_hi12:symbol, lsl #12
   // CHECK-NEXT: ^
   add x0, x0, :dtprel_lo12:symbol
   // CHECK: error: relocation variant :dtprel_lo12: unsupported on COFF targets
   // CHECK-NEXT: add x0, x0, :dtprel_lo12:symbol
   // CHECK-NEXT: ^
 
+label:
   movz x0, #:abs_g0:symbol
   // CHECK: error: relocation type :abs_g0: unsupported on COFF targets
   // CHECK-NEXT: movz x0, #:abs_g0:symbol
   // CHECK-NEXT: ^
+
+  .section .rdata, "dr"
+table:
+  .short label - table
+  // CHECK: error: Cannot represent this expression
+  // CHECK-NEXT: .short label - table
+  // CHECK-NEXT: ^

llvm/test/MC/AArch64/coff-relocations.s

 // RUN: llvm-mc -triple aarch64-windows -filetype obj -o %t.obj %s
 // RUN: llvm-readobj -r %t.obj | FileCheck %s
 // RUN: llvm-objdump -d %t.obj | FileCheck %s --check-prefix=DISASM
+// RUN: llvm-objdump -s %t.obj | FileCheck %s --check-prefix=DATA
 
 // IMAGE_REL_ARM64_ADDR32
 .Linfo_foo:
   .asciz "foo"
   .long foo
 
 // IMAGE_REL_ARM64_ADDR32NB
 .long func@IMGREL

 adr x0, foo + 0x12345
 
 // IMAGE_REL_ARM64_BRANCH19
 bne target
 
 // IMAGE_REL_ARM64_BRANCH14
 tbz x0, #0, target
 
+.section .rdata, "dr"
+.Ltable:
+.word .Linfo_bar - .Ltable
+.word .Linfo_foo - .Ltable
+
 // CHECK: Format: COFF-ARM64
 // CHECK: Arch: aarch64
 // CHECK: AddressSize: 64bit
 // CHECK: Relocations [
 // CHECK:   Section (1) .text {
 // CHECK: 0x4 IMAGE_REL_ARM64_ADDR32 foo
 // CHECK: 0x8 IMAGE_REL_ARM64_ADDR32NB func
 // CHECK: 0xC IMAGE_REL_ARM64_ADDR64 arr

 // CHECK: 0x3C IMAGE_REL_ARM64_PAGEOFFSET_12L foo
 // CHECK: 0x40 IMAGE_REL_ARM64_SECREL_LOW12A foo
 // CHECK: 0x44 IMAGE_REL_ARM64_SECREL_HIGH12A foo
 // CHECK: 0x48 IMAGE_REL_ARM64_SECREL_LOW12L foo
 // CHECK: 0x4C IMAGE_REL_ARM64_REL21 foo
 // CHECK: 0x50 IMAGE_REL_ARM64_BRANCH19 target
 // CHECK: 0x54 IMAGE_REL_ARM64_BRANCH14 target
 // CHECK:   }
+// CHECK:   Section (4) .rdata {
+// CHECK: 0x0 IMAGE_REL_ARM64_REL32 .text
+// CHECK: 0x4 IMAGE_REL_ARM64_REL32 .text
+// CHECK:   }
 // CHECK: ]
 
 // DISASM: 30:       20 1a 09 b0     adrp    x0, 0x12345000
 // DISASM: 34:       00 14 0d 91     add     x0, x0, #837
 // DISASM: 38:       00 14 4d 39     ldrb    w0, [x0, #837]
 // DISASM: 3c:       00 a4 41 f9     ldr     x0, [x0, #840]
 // DISASM: 40:       00 00 00 91     add     x0, x0, #0
 // DISASM: 44:       00 00 40 91     add     x0, x0, #0, lsl #12
 // DISASM: 48:       00 00 40 f9     ldr     x0, [x0]
 // DISASM: 4c:       20 1a 09 30     adr     x0, #74565
+
+// DATA: Contents of section .rdata:
+// DATA-NEXT:  0000 2c000000 04000000