[AArch64] Attempt to parse expressions as adr/adrp operands

[dmgreen]

Summary

This tries to make use of evaluateAsRelocatable in AArch64AsmParser::classifySymbolRef to
parse more complex expressions as relocatable operands to ADR/ADRP's. It is hopefully
better than the existing code which only handles Symbol +- Constant.

Fixes PR38831

[efriedma]

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

5289

classifySymbolRef is used for other instructions, including movw/movk, add/sub, and ldr. Please make sure we have appropriate test coverage.

It looks like a lot of instructions try to do some sort of fallback if classifySymbolRef fails. I'm not sure that actually make sense, but please make sure we have test coverage, at least.

5313

It looks like there isn't any test coverage for the !Res.getSymA() || Res.getSymB() part? In particular, I'm not sure why you'd want to reject an absolute value, in general.

There should probably also be test coverage that someone doesn't write something silly like adr x0, :lo12:f.

test/MC/AArch64/adr-badsyms.s

36

Newline?

[jakehehrlich]

cc @mcgrathr @phosek

Awesome! I'm looking at this now.

[peter.smith]

I don't have much to add over Eli's existing comments. I think the general form of (symA - symB) + constant may not be handled well for all instructions though.

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

5313

In principle I agree that something like

 .text
lab1: nop
lab2: nop
 adr x0, lab2 - lab1 + 0x1000

which would be rejected by the code above, but is equivalent to adr x0, 0x1004 which is.

From a brief experiment I don't think that the backend can handle expressions like that for all relocations though. I ended up with a RELA relocation with addend 0x1004 to an undefined symbol, which unsurprisingly didn't get past the linker, we'd need an absolute symbol with value 0. May be if the backend were fixed first we could let that one through.

[dmgreen]

I seem to have stepped into a bit of a rabbit hole here.. and a lot of things seem to be a bit broken.

As far as I understand, something like (lab2-lab1)*2 + Sym should be accepted (if the difference between lab2 and lab1 is know, Sym can be relocated against). This is handled by gcc, but I don't see how it would be easily handled by clang atm. We are trying to classify these operands quite early.

I haven't managed to fix that here, but have added a number of tests around the place for things I couldn't see otherwise tested. Let me know if I should add any more.

I've also made mov's only accept 0 addends and ld/st accept addends > 0 and not mod scale. I think this is OK (it seems to be what gcc does, you don't know the alignment of Sym afterall), but I'm not sure if it needs to work that way for darwin.

[efriedma]

LGTM, but please give Peter a couple days to see if he has any additional comments.

[peter.smith]

Yes I'm happy with this too.

[phosek]

I tested this change with Fuchsia and it's working for us. Thank you for looking into this! I'd like to see this landed as soon as possible as it unblocks the toolchain roll for us.

[dmgreen]

OK. Thanks guys.

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

 #include "llvm/MC/MCParser/MCParsedAsmOperand.h"
 #include "llvm/MC/MCParser/MCTargetAsmParser.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/MC/MCTargetOptions.h"
 #include "llvm/MC/SubtargetFeature.h"
+#include "llvm/MC/MCValue.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/SMLoc.h"
 #include "llvm/Support/TargetParser.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/raw_ostream.h"
bool AArch64AsmParser::parseDirectiveUnreq(SMLoc L) {
   RegisterReqs.erase(Parser.getTok().getIdentifier().lower());
   Parser.Lex(); // Eat the identifier.
   if (parseToken(AsmToken::EndOfStatement))
     return addErrorSuffix("in '.unreq' directive");
   return false;
 }
 
 bool
 AArch64AsmParser::classifySymbolRef(const MCExpr *Expr,
                                     AArch64MCExpr::VariantKind &ELFRefKind,
                                     MCSymbolRefExpr::VariantKind &DarwinRefKind,
                                     int64_t &Addend) {
   ELFRefKind = AArch64MCExpr::VK_INVALID;
   DarwinRefKind = MCSymbolRefExpr::VK_None;
   Addend = 0;
 
   if (const AArch64MCExpr *AE = dyn_cast<AArch64MCExpr>(Expr)) {
     ELFRefKind = AE->getKind();
     Expr = AE->getSubExpr();
   }
 
   const MCSymbolRefExpr *SE = dyn_cast<MCSymbolRefExpr>(Expr);
   if (SE) {
     // It's a simple symbol reference with no addend.
     DarwinRefKind = SE->getKind();
     return true;
   }
 
-  const MCBinaryExpr *BE = dyn_cast<MCBinaryExpr>(Expr);
-  if (!BE)
+  // Check that it looks like a symbol + an addend
+  MCValue Res;
+  bool Relocatable = Expr->evaluateAsRelocatable(Res, nullptr, nullptr);
+  if (!Relocatable || !Res.getSymA() || Res.getSymB())
     return false;
 
-  SE = dyn_cast<MCSymbolRefExpr>(BE->getLHS());
-  if (!SE)
-    return false;
-  DarwinRefKind = SE->getKind();
-
-  if (BE->getOpcode() != MCBinaryExpr::Add &&
-      BE->getOpcode() != MCBinaryExpr::Sub)
-    return false;
-
-  // See if the addend is a constant, otherwise there's more going
-  // on here than we can deal with.
-  auto AddendExpr = dyn_cast<MCConstantExpr>(BE->getRHS());
-  if (!AddendExpr)
-    return false;
-
-  Addend = AddendExpr->getValue();
-  if (BE->getOpcode() == MCBinaryExpr::Sub)
-    Addend = -Addend;
+  DarwinRefKind = Res.getSymA()->getKind();
+  Addend = Res.getConstant();
 
   // It's some symbol reference + a constant addend, but really
   // shouldn't use both Darwin and ELF syntax.
   return ELFRefKind == AArch64MCExpr::VK_INVALID ||
          DarwinRefKind == MCSymbolRefExpr::VK_None;
 }
 
 /// Force static initialization.

test/MC/AArch64/adr-badsyms.s

+// RUN: not llvm-mc -triple aarch64-none-linux-gnu %s -filetype=obj -o /dev/null 2>&1 | FileCheck %s
+
+  adr x0, -IMAGE_START
+// CHECK: error: expected relocatable expression
+// CHECK-NEXT:   adr x0, -IMAGE_START
+// CHECK-NEXT:   ^
+  adr x1, IMAGE_START * 10
+// CHECK: error: expected relocatable expression
+// CHECK-NEXT:   adr x1, IMAGE_START * 10
+// CHECK-NEXT:   ^
+  adr x2, 2 * (IMAGE_LOAD_START + 987136)
+// CHECK: error: expected relocatable expression
+// CHECK-NEXT:   adr x2, 2 * (IMAGE_LOAD_START + 987136)
+// CHECK-NEXT:   ^
+  adr x3, (IMAGE_LOAD_END + IMAGE_LOAD_START)
+// CHECK: error: expected relocatable expression
+// CHECK-NEXT:   adr x3, (IMAGE_LOAD_END + IMAGE_LOAD_START)
+// CHECK-NEXT:   ^
+
+  adrp x0, -IMAGE_START
+// CHECK: error: expected relocatable expression
+// CHECK-NEXT:   adrp x0, -IMAGE_START
+// CHECK-NEXT:   ^
+  adrp x1, IMAGE_START * 10
+// CHECK: error: expected relocatable expression
+// CHECK-NEXT:   adrp x1, IMAGE_START * 10
+// CHECK-NEXT:   ^
+  adrp x2, 2 * (IMAGE_LOAD_START + 987136)
+// CHECK: error: expected relocatable expression
+// CHECK-NEXT:   adrp x2, 2 * (IMAGE_LOAD_START + 987136)
+// CHECK-NEXT:   ^
+  adrp x3, (IMAGE_LOAD_END + IMAGE_LOAD_START)
+// CHECK: error: expected relocatable expression
+// CHECK-NEXT:   adrp x3, (IMAGE_LOAD_END + IMAGE_LOAD_START)
+// CHECK-NEXT:   ^
 No newline at end of file

test/MC/AArch64/adr.s

+// RUN: llvm-mc -triple aarch64-none-linux-gnu %s -filetype=obj -o %t.o
+// RUN: llvm-objdump -d -r %t.o | FileCheck %s
+
+  // CHECK: adr x0, #100
+  // CHECK-NEXT: adr x1, #0
+  // CHECK-NEXT: R_AARCH64_ADR_PREL_LO21  IMAGE_START
+  // CHECK-NEXT: adr x2, #0
+  // CHECK-NEXT: R_AARCH64_ADR_PREL_LO21  IMAGE_START
+  // CHECK-NEXT: adr x3, #0
+  // CHECK-NEXT: R_AARCH64_ADR_PREL_LO21  IMAGE_LOAD_START+987136
+  // CHECK-NEXT: adr x4, #0
+  // CHECK-NEXT: R_AARCH64_ADR_PREL_LO21  IMAGE_LOAD_START+987136
+  // CHECK-NEXT: adr x5, #0
+  // CHECK-NEXT: R_AARCH64_ADR_PREL_LO21  IMAGE_LOAD_START+987136
+
+  adr x0, 100
+  adr x1, IMAGE_START
+  adr x2, IMAGE_START + 0
+  adr x3, IMAGE_LOAD_START + 987136
+  adr x4, (0xffffffff000f1000 - 0xffffffff00000000 + IMAGE_LOAD_START)
+  adr x5, IMAGE_LOAD_START + (0xffffffff000f1000 - 0xffffffff00000000)
+
+  // CHECK-NEXT: adrp x1, #0
+  // CHECK-NEXT: R_AARCH64_ADR_PREL_PG_HI21 IMAGE_START
+  // CHECK-NEXT: adrp x2, #0
+  // CHECK-NEXT: R_AARCH64_ADR_PREL_PG_HI21 IMAGE_START
+  // CHECK-NEXT: adrp x3, #0
+  // CHECK-NEXT: R_AARCH64_ADR_PREL_PG_HI21 IMAGE_LOAD_START+987136
+  // CHECK-NEXT: adrp x4, #0
+  // CHECK-NEXT: R_AARCH64_ADR_PREL_PG_HI21 IMAGE_LOAD_START+987136
+  // CHECK-NEXT: adrp x5, #0
+  // CHECK-NEXT: R_AARCH64_ADR_PREL_PG_HI21 IMAGE_LOAD_START+987136
+
+  adrp x1, IMAGE_START
+  adrp x2, IMAGE_START + 0
+  adrp x3, IMAGE_LOAD_START + 987136
+  adrp x4, (0xffffffff000f1000 - 0xffffffff00000000 + IMAGE_LOAD_START)
+  adrp x5, IMAGE_LOAD_START + (0xffffffff000f1000 - 0xffffffff00000000)