[ARM] support symbolic expressions as branch target in b.w

[jcai19]

Summary

Currently ARM backend validates the range of branch targets before the
layout of fragments is finalized. This causes build failure if symbolic
expressions are used, with the exception of a single symbolic value.
For example, "b.w ." works but "b.w . + 2" currently fails to
assemble. This fixes the issue by delaying this check (in
ARMAsmParser::validateInstruction) of b.w instructions until the symbol
expressions are resolved (in ARMAsmBackend::adjustFixupValue).

Link:
https://github.com/ClangBuiltLinux/linux/issues/1286

[jcai19]

This is created based on the discussion at https://reviews.llvm.org/D97501#inline-914358.

[nickdesaulniers]

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

7955

if you don't use the return value from dyn_cast, you should use isa.
https://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates

or rewrite this as two nested ifs:

if (auto foo = dyn_cast<...):
  if (!foo.isSignedOffset<...):
    return Error(...);

[MaskRay]

I made a very similar patch for RISC-V: D92293.

Patching isSignedOffset is fine since it fixes more instructions. It is also fine to restrict the lift on t2B currently as you don't have tests for others.

llvm/test/MC/ARM/thumb2-b.w-target.s

1

Use consistent comment marker in a test.

5

No need to say GNU objdump behavior. The specific llvm-objdump tests for the symbolization will do it.

11

start+0xfffff is undeeded detail. There should be specific llvm-objdump tests for the symbolization.

llvm/test/MC/ARM/thumb2-branch-ranges.s

101

[[@LINE]] is deprecated FileCheck syntax.

[[#@LINE+1]]:[[#]]: error: ...

[jcai19]

Skip range check of MCBinaryExpr in b.w instead of any non-constant expressions.

[jcai19]

Remove unncessary CHECK-NOT.

[MaskRay]

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

7954

Add a period after a complete sentence.

[jcai19]

Add period after a comment.

[nickdesaulniers]

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

7967

It's definitely not great seeing this problem across multiple different instructions; in particular I suspect that t2Bcc is the same operation as t2B (branch), just with a condition code (cc) and smaller immediate.

I still think the callers should defer checks for MCBinaryExpr (as this patch does), but then seeing things like isThumbBranchTarget/isARMBranchTarget makes me nervous that we can't validate MCBinaryExpr operands until after adjusting fixups (very late) for many other instructions.

If MCSymbolRefExpr is just the . operator in assembler, and MCBinaryExpr is for expressions, I wonder if isUnsignedOffset, isSignedOffset, isLEOffset, and isThumbMemPC should all return true for MCBinaryExpr?

isARMBranchTarget/isThumbBranchTarget default to true (for example for MCSymbolRefExpr and MCBinaryExpr, but the list above do not. And that's curious to me. We can't know if these immediates satisfy some range constraints until after fixup; right now these methods are a mix of what they permit when expressions involving . are used.

[nickdesaulniers]

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

7967

And part of that is that these isSignedOffset and friends all return bool, when for MCSymbolRefExpr and MCBinaryExpr operands the answer has a third possible state which is "maybe, but I won't know until adjusting fixups" which is one of the last things the assembler does, without rerunning instruction validation. isUnsignedOffset, isSignedOffset, isLEOffset, and isThumbMemPC all respond "no" conservatively; isARMBranchTarget/isThumbBranchTarget respond "sure" perhaps too eagerly. I suspect given a MCSymbolRefExpr or MCBinaryExpr, isARMBranchTarget/isThumbBranchTarget both respond true, when after fixups the answer perhaps should have been "no."

[jcai19]

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

7967

Agreed, I will take a look at the code and see if we could refactor it for a more permanent solution. Also . gets converted into a symbol like any regular label, so MCSymbolRefExpr covers any expressions with a single label or ..

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp

bool ARMAsmParser::validateInstruction(MCInst &Inst,
 
   // Final range checking for Thumb unconditional branch instructions.
   case ARM::tB:
     if (!(static_cast<ARMOperand &>(*Operands[2])).isSignedOffset<11, 1>())
       return Error(Operands[2]->getStartLoc(), "branch target out of range");
     break;
   case ARM::t2B: {
     int op = (Operands[2]->isImm()) ? 2 : 3;
-    if (!static_cast<ARMOperand &>(*Operands[op]).isSignedOffset<24, 1>())
+    ARMOperand &Operand = static_cast<ARMOperand &>(*Operands[op]);
+    // Delay the checks of symbolic expressions until they are resolved
+    if (dyn_cast<MCConstantExpr>(Operand.getImm()) &&
+        !Operand.isSignedOffset<24, 1>())
       return Error(Operands[op]->getStartLoc(), "branch target out of range");
     break;
   }
   // Final range checking for Thumb conditional branch instructions.
   case ARM::tBcc:
     if (!static_cast<ARMOperand &>(*Operands[2]).isSignedOffset<8, 1>())
       return Error(Operands[2]->getStartLoc(), "branch target out of range");
     break;
   case ARM::t2Bcc: {
     int Op = (Operands[2]->isImm()) ? 2 : 3;
     if (!static_cast<ARMOperand &>(*Operands[Op]).isSignedOffset<20, 1>())
       return Error(Operands[Op]->getStartLoc(), "branch target out of range");
     break;
   }
   case ARM::tCBZ:
   case ARM::tCBNZ: {
     if (!static_cast<ARMOperand &>(*Operands[2]).isUnsignedOffset<6, 1>())
       return Error(Operands[2]->getStartLoc(), "branch target out of range");
     break;

llvm/test/MC/ARM/thumb2-b.w-target.s

+# RUN: llvm-mc -triple=thumbv7 -filetype=obj %s | llvm-objdump --triple=thumbv7 -d - | FileCheck %s
+
+.syntax unified
+
+// arm-linux-gnueabihf-objdump produced
+//  b.w fffffc <start+0xfffffc>
+//  b.w a <start+0xa>
+
+// llvm-objdump produced the following
+// CHECK-LABEL: start
+// CHECK-NEXT:  b.w #16777208 <start+0xfffffc>
+// CHECK-NEXT:  b.w #2 <start+0xa>
+start:
+  b.w start - 1f + 0x1000000
+1:
+  b.w . + (2f - 1b + 2)
+2:

llvm/test/MC/ARM/thumb2-branch-ranges.s

 
 start6:
         .space 0x100000
 // branch to thumb function resolved at assembly time
 // CHECK-NOT: error
 // CHECK: [[@LINE+2]]:{{[0-9]}}: error: Relocation out of range
 // CHECK-LABEL: beq.w start6
         beq.w start6
+
+start7:
+// branch to thumb function resolved at assembly time
+// CHECK-NOT: error
+// CHECK: [[@LINE+1]]:{{[0-9]}}: error: Relocation out of range
+        b.w start8 - start7 + 0x1000000
+start8: