[AArch64] [Windows] Error out on unsupported symbol locations

[mstorsjo]

Summary

These might occur in seemingly generic assembly. Previously when targeting COFF, they were silently ignored, which certainly won't give the right result. Instead clearly error out, to make it clear that the assembly needs to be adjusted for this target.

Also change a preexisting report_fatal_error into a proper error message, pointing out the offending source instruction. This isn't strictly an internal error, as it can be triggered by user input.

[efriedma]

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

119

VK_GOT isn't a bitmask.

If you need to mask, use the helpers AArch64MCExpr::getSymbolLoc() etc.

We probably also want to exclude other variants that don't make sense, like VK_GOTTPREL?

"unknown AArch64 symbol kind" isn't a great error message; can we explicitly say that we don't expect this kind of symbol on COFF targets?

[mstorsjo]

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

119

VK_GOT isn't a bitmask.

If you need to mask, use the helpers AArch64MCExpr::getSymbolLoc() etc.

Ah, thanks, will do.

We probably also want to exclude other variants that don't make sense, like VK_GOTTPREL?

Yeah, that's probably safest. I initially went with just disallowing certain ones, but in the end it looks like only VK_ABS and VK_SECREL are supported at all, and this can be checked at the top outside of this switch, so instead of forbidding individual ones, just allow the few supported ones and error out on anything else.

"unknown AArch64 symbol kind" isn't a great error message; can we explicitly say that we don't expect this kind of symbol on COFF targets?

True, and we can also mention the explicit variant kind, for an even more understandable error message.

[mstorsjo]

Using getSymbolLoc(), moved the check to the top of the function, doing the check of symbol location regardless of relocation type (as only VK_ABS and VK_SECREL are supported), changed a preexisting report_fatal_error for unsupported relocation types into an error with a source location, as it can be triggered with custom assembly input.

I've managed to a rather large corpus of code (VLC, including around 100 third party library dependencies) with this patch, so it looks fairly safe that no other symbol locations than VK_ABS and VK_SECREL are being used in successfully built code so far.

[compnerd]

This looks fantastic! I'm really liking the better diagnostics. The only thing I'd change is the "symbol kind %0 unsupported on COFF targets". I think that "relocation variant" is more precise than "symbol kind". Thanks for improving this.

[mstorsjo]

@hans - WDYT about backporting this one? It's a little bit risky if the check would turn out to be too strict, but there's been no false positives in my test build corpus (which includes a fair bit of assembly source); the upside is for anybody working on porting (mainly assembly) code that isn't originally written for arm64/windows - as it previously wouldn't error out at build time on constructs that don't exist on windows and won't work as intended at runtime.

[hans]

In D85242#2198707, @mstorsjo wrote:

@hans - WDYT about backporting this one? It's a little bit risky if the check would turn out to be too strict, but there's been no false positives in my test build corpus (which includes a fair bit of assembly source); the upside is for anybody working on porting (mainly assembly) code that isn't originally written for arm64/windows - as it previously wouldn't error out at build time on constructs that don't exist on windows and won't work as intended at runtime.

I think the risk/reward ratio is decent here. Pushed as 3aec1c6a493f543d41caf99e5df9056776856941, please shout if you see any problems.

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

 //= AArch64WinCOFFObjectWriter.cpp - AArch64 Windows COFF Object Writer C++ =//
 //
 // 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
 //
 //===---------------------------------------------------------------------===//
 
 #include "MCTargetDesc/AArch64FixupKinds.h"
 #include "MCTargetDesc/AArch64MCExpr.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/BinaryFormat/COFF.h"
 #include "llvm/MC/MCAsmBackend.h"
+#include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCFixup.h"
 #include "llvm/MC/MCFixupKindInfo.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCValue.h"
 #include "llvm/MC/MCWinCOFFObjectWriter.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"

 
 unsigned AArch64WinCOFFObjectWriter::getRelocType(
     MCContext &Ctx, const MCValue &Target, const MCFixup &Fixup,
     bool IsCrossSection, const MCAsmBackend &MAB) const {
   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())) {
   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());
-    report_fatal_error(Twine("unsupported relocation type: ") + Info.Name);
+      Ctx.reportError(Fixup.getLoc(), Twine("relocation type ") + Info.Name +
+                                          " unsupported on COFF targets");
+    }
+    return COFF::IMAGE_REL_ARM64_ABSOLUTE; // Dummy return value
   }
 
   case FK_Data_4:
     switch (Modifier) {
     default:
       return COFF::IMAGE_REL_ARM64_ADDR32;
     case MCSymbolRefExpr::VK_COFF_IMGREL32:
       return COFF::IMAGE_REL_ARM64_ADDR32NB;
unsigned AArch64WinCOFFObjectWriter::getRelocType(
   case AArch64::fixup_aarch64_ldst_imm12_scale2:
   case AArch64::fixup_aarch64_ldst_imm12_scale4:
   case AArch64::fixup_aarch64_ldst_imm12_scale8:
   case AArch64::fixup_aarch64_ldst_imm12_scale16:
     if (const AArch64MCExpr *A64E = dyn_cast<AArch64MCExpr>(Expr)) {
       AArch64MCExpr::VariantKind RefKind = A64E->getKind();
       if (RefKind == AArch64MCExpr::VK_SECREL_LO12)
         return COFF::IMAGE_REL_ARM64_SECREL_LOW12L;
     }
     return COFF::IMAGE_REL_ARM64_PAGEOFFSET_12L;
 
   case AArch64::fixup_aarch64_pcrel_adr_imm21:
     return COFF::IMAGE_REL_ARM64_REL21;
 
   case AArch64::fixup_aarch64_pcrel_adrp_imm21:
     return COFF::IMAGE_REL_ARM64_PAGEBASE_REL21;
 

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
+
+  adrp x0, :got:symbol
+  // CHECK: error: relocation variant :got: unsupported on COFF targets
+  // CHECK-NEXT: adrp x0, :got:symbol
+  // CHECK-NEXT: ^
+
+  ldr x0, [x0, :got_lo12:symbol]
+  // CHECK: error: relocation variant :got_lo12: unsupported on COFF targets
+  // CHECK-NEXT: ldr x0, [x0, :got_lo12:symbol]
+  // CHECK-NEXT: ^
+
+  adrp x0, :tlsdesc:symbol
+  // CHECK: error: relocation variant :tlsdesc: unsupported on COFF targets
+  // CHECK-NEXT: adrp x0, :tlsdesc:symbol
+  // CHECK-NEXT: ^
+  add x0, x0, :tlsdesc_lo12:symbol
+  // CHECK: error: relocation variant :tlsdesc_lo12: unsupported on COFF targets
+  // CHECK-NEXT: add x0, x0, :tlsdesc_lo12:symbol
+  // CHECK-NEXT: ^
+
+  adrp x0, :gottprel:symbol
+  // CHECK: error: relocation variant :gottprel: unsupported on COFF targets
+  // CHECK-NEXT: adrp x0, :gottprel:symbol
+  // CHECK-NEXT: ^
+  ldr x0, [x0, :gottprel_lo12:symbol]
+  // CHECK: error: relocation variant :gottprel_lo12: unsupported on COFF targets
+  // CHECK-NEXT: ldr x0, [x0, :gottprel_lo12:symbol]
+  // CHECK-NEXT: ^
+
+  add x0, x0, #:dtprel_hi12:symbol, lsl #12
+  // 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: ^
+
+  movz x0, #:abs_g0:symbol
+  // CHECK: error: relocation type :abs_g0: unsupported on COFF targets
+  // CHECK-NEXT: movz x0, #:abs_g0:symbol
+  // CHECK-NEXT: ^