Fix for pr24346: arm asm label calculation error in sub

[laszio]

Summary

Some ARM instructions encode 32-bit immediates as a 8-bit integer (0-255)
and a 4-bit rotation (0-30, even) in its least significant 12 bits. The
original fixup, FK_Data_4, patches the instruction by the value bit-to-bit,
regardless of the encoding. For example, assuming the label L1 and L2 are
0x0 and 0x104 respectively, the following instruction:

add r0, r0, #(L2 - L1) ; expects 0x104, i.e., 260

would be assembled to the following, which adds 1 to r0, instead of 260:

e2800104 add r0, r0, #4, 2 ; equivalently 1

The new fixup kind fixup_arm_mod_imm takes care of the encoding:

e2800f41 add r0, r0, #260

[laszio]

Hi Asiri,

It seems that the fixup for modified-immediates needs some adjustment. Would you mind to take a look or suggest some other reviewers?

[rmaprath]

The patch looks fine for me.

I'd rather let someone with access to source/ARMARM approve this (I'm on holiday). @olista01: can you please have a look?

\ Asiri

[jmolloy]

Hi,

This seems correct to me.

Cheers,

James

[laszio]

Hi James, Asiri and Oliver,

Could you please help me commit this? I've read the developer policy several times but still can't figure out how to get this landed without bothering a developer with commit access.

[t.p.northover]

If you're planning to be doing more LLVM work, then it's easy enough to request commit access: http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access (basically, e-mail Chris with the necessary details for the account you want).

Otherwise, we're generally quite happy to commit on other people's behalf. It's really not much work when the patch applies cleanly. Let us know which you want.

Cheers.

Tim.

[laszio]

Hi Tim,

Thanks for the information. I read it again and according to the first sentence of the quoted section:

"We grant commit access to contributors with a track record of submitting high quality patches."

Although it is subjective, and despite of the fact that I shall be working on LLVM frequently, I'm not confident enough because this is my first patch :)

May I ask you for a favor to commit this for me? Or is it OK to request commit access directly?

[jmolloy]

Hi,

r265122

Thanks!

James

lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp

const static MCFixupKindInfo InfosLE[ARM::NumTargetFixupKinds] = {
            MCFixupKindInfo::FKF_IsAlignedDownTo32Bits},
       {"fixup_arm_thumb_bcc", 0, 8, MCFixupKindInfo::FKF_IsPCRel},
       // movw / movt: 16-bits immediate but scattered into two chunks 0 - 12, 16
       // - 19.
       {"fixup_arm_movt_hi16", 0, 20, 0},
       {"fixup_arm_movw_lo16", 0, 20, 0},
       {"fixup_t2_movt_hi16", 0, 20, 0},
       {"fixup_t2_movw_lo16", 0, 20, 0},
+      {"fixup_arm_mod_imm", 0, 12, 0},
   };
   const static MCFixupKindInfo InfosBE[ARM::NumTargetFixupKinds] = {
       // This table *must* be in the order that the fixup_* kinds are defined in
       // ARMFixupKinds.h.
       //
       // Name                      Offset (bits) Size (bits)     Flags
       {"fixup_arm_ldst_pcrel_12", 0, 32, MCFixupKindInfo::FKF_IsPCRel},
       {"fixup_t2_ldst_pcrel_12", 0, 32,
const static MCFixupKindInfo InfosBE[ARM::NumTargetFixupKinds] = {
            MCFixupKindInfo::FKF_IsAlignedDownTo32Bits},
       {"fixup_arm_thumb_bcc", 8, 8, MCFixupKindInfo::FKF_IsPCRel},
       // movw / movt: 16-bits immediate but scattered into two chunks 0 - 12, 16
       // - 19.
       {"fixup_arm_movt_hi16", 12, 20, 0},
       {"fixup_arm_movw_lo16", 12, 20, 0},
       {"fixup_t2_movt_hi16", 12, 20, 0},
       {"fixup_t2_movw_lo16", 12, 20, 0},
+      {"fixup_arm_mod_imm", 20, 12, 0},
   };
 
   if (Kind < FirstTargetFixupKind)
     return MCAsmBackend::getFixupKindInfo(Kind);
 
   assert(unsigned(Kind - FirstTargetFixupKind) < getNumFixupKinds() &&
          "Invalid kind!");
   return (IsLittleEndian ? InfosLE : InfosBE)[Kind - FirstTargetFixupKind];
case ARM::fixup_t2_pcrel_10: {
 
     // Same addressing mode as fixup_arm_pcrel_10, but with 16-bit halfwords
     // swapped.
     if (Kind == ARM::fixup_t2_pcrel_10)
       return swapHalfWords(Value, IsLittleEndian);
 
     return Value;
   }
+  case ARM::fixup_arm_mod_imm:
+    Value = ARM_AM::getSOImmVal(Value);
+    if (Ctx && Value >> 12) {
+      Ctx->reportError(Fixup.getLoc(), "out of range immediate fixup value");
+      return 0;
+    }
+    return Value;
   }
 }
 
 void ARMAsmBackend::processFixupValue(const MCAssembler &Asm,
                                       const MCAsmLayout &Layout,
                                       const MCFixup &Fixup,
                                       const MCFragment *DF,
                                       const MCValue &Target, uint64_t &Value,
static unsigned getFixupKindNumBytes(unsigned Kind) {
   case ARM::fixup_arm_thumb_bcc:
   case ARM::fixup_arm_thumb_cp:
   case ARM::fixup_thumb_adr_pcrel_10:
     return 1;
 
   case FK_Data_2:
   case ARM::fixup_arm_thumb_br:
   case ARM::fixup_arm_thumb_cb:
+  case ARM::fixup_arm_mod_imm:
     return 2;
 
   case ARM::fixup_arm_pcrel_10_unscaled:
   case ARM::fixup_arm_ldst_pcrel_12:
   case ARM::fixup_arm_pcrel_10:
   case ARM::fixup_arm_adr_pcrel_12:
   case ARM::fixup_arm_uncondbl:
   case ARM::fixup_arm_condbl:
static unsigned getFixupKindContainerSizeBytes(unsigned Kind) {
   case ARM::fixup_t2_pcrel_10:
   case ARM::fixup_t2_adr_pcrel_12:
   case ARM::fixup_arm_thumb_bl:
   case ARM::fixup_arm_thumb_blx:
   case ARM::fixup_arm_movt_hi16:
   case ARM::fixup_arm_movw_lo16:
   case ARM::fixup_t2_movt_hi16:
   case ARM::fixup_t2_movw_lo16:
+  case ARM::fixup_arm_mod_imm:
     // Instruction size is 4 bytes.
     return 4;
   }
 }
 
 void ARMAsmBackend::applyFixup(const MCFixup &Fixup, char *Data,
                                unsigned DataSize, uint64_t Value,
                                bool IsPCRel) const {

lib/Target/ARM/MCTargetDesc/ARMFixupKinds.h

enum Fixups {
 
   // The next two are for the movt/movw pair
   // the 16bit imm field are split into imm{15-12} and imm{11-0}
   fixup_arm_movt_hi16, // :upper16:
   fixup_arm_movw_lo16, // :lower16:
   fixup_t2_movt_hi16, // :upper16:
   fixup_t2_movw_lo16, // :lower16:
 
+  // fixup_arm_mod_imm - Fixup for mod_imm
+  fixup_arm_mod_imm,
+
   // Marker
   LastTargetFixupKind,
   NumTargetFixupKinds = LastTargetFixupKind - FirstTargetFixupKind
 };
 }
 }
 
 #endif

lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp

public:
   unsigned getModImmOpValue(const MCInst &MI, unsigned Op,
                             SmallVectorImpl<MCFixup> &Fixups,
                             const MCSubtargetInfo &ST) const {
     const MCOperand &MO = MI.getOperand(Op);
 
     // Support for fixups (MCFixup)
     if (MO.isExpr()) {
       const MCExpr *Expr = MO.getExpr();
-      // In instruction code this value always encoded as lowest 12 bits,
-      // so we don't have to perform any specific adjustments.
-      // Due to requirements of relocatable records we have to use FK_Data_4.
-      // See ARMELFObjectWriter::ExplicitRelSym and
-      //     ARMELFObjectWriter::GetRelocTypeInner for more details.
-      MCFixupKind Kind = MCFixupKind(FK_Data_4);
+      // Fixups resolve to plain values that need to be encoded.
+      MCFixupKind Kind = MCFixupKind(ARM::fixup_arm_mod_imm);
       Fixups.push_back(MCFixup::create(0, Expr, Kind, MI.getLoc()));
       return 0;
     }
 
     // Immediate is already in its encoded format
     return MO.getImm();
   }
 

test/MC/ARM/arm_fixups.s

 @ CHECK-BE: @   fixup A - offset: 0, value: _foo, kind: fixup_arm_movt_hi16
 
     mov r2, :lower16:fred
 
 @ CHECK: movw  r2, :lower16:fred                 @ encoding: [A,0x20'A',0b0000AAAA,0xe3]
 @ CHECK: @   fixup A - offset: 0, value: fred, kind: fixup_arm_movw_lo16
 @ CHECK-BE: movw  r2, :lower16:fred                 @ encoding: [0xe3,0b0000AAAA,0x20'A',A]
 @ CHECK-BE: @   fixup A - offset: 0, value: fred, kind: fixup_arm_movw_lo16
+
+    add r0, r0, #(L1 - L2)
+
+@ CHECK: add     r0, r0, #L1-L2          @ encoding: [A,0b0000AAAA,0x80,0xe2]
+@ CHECK: @   fixup A - offset: 0, value: L1-L2, kind: fixup_arm_mod_imm
+@ CHECK-BE: add     r0, r0, #L1-L2          @ encoding: [0xe2,0x80,0b0000AAAA,A]
+@ CHECK-BE: @   fixup A - offset: 0, value: L1-L2, kind: fixup_arm_mod_imm

test/MC/ARM/modified-immediate-fixup-error.s

+@ PR24346
+@ RUN: not llvm-mc -triple=arm-linux-gnueabi -filetype=obj < %s 2>&1 | FileCheck %s
+
+    .data
+    .align 8
+L2:
+    .word 0
+    .align 8
+    .byte 0
+L1:
+
+    .text
+@ CHECK: error: out of range immediate fixup value
+    add r0, r0, #(L1 - L2)

test/MC/ARM/modified-immediate-fixup.s

+@ PR24346
+@ RUN: llvm-mc < %s -triple=arm-linux-gnueabi -filetype=obj -o - \
+@ RUN: | llvm-objdump --disassemble -arch=arm - | FileCheck %s
+
+    .data
+    .align 8
+L2:
+    .word 0
+    .align 8
+    .word 0
+L1:
+
+    .text
+@ CHECK: add r0, r0, #260
+    add r0, r0, #(L1 - L2)