[PATCH] D85242: [AArch64] [Windows] Error out on some ELF style GOT relative relocations

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64WinCOFFObjectWriter.cpp:96
         return COFF::IMAGE_REL_ARM64_SECREL_LOW12L;
+      if (RefKind & AArch64MCExpr::VK_GOT)
+        Ctx.reportError(Fixup.getLoc(), "unknown AArch64 symbol kind!");
----------------
efriedma wrote:
> 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?
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85242/new/

https://reviews.llvm.org/D85242