[llvm] [llvm][ARM] Add Addend Checks for MOVT and MOVW instructions. (PR #111970)
Jack Styles via llvm-commits
llvm-commits at lists.llvm.org
Fri Oct 11 07:56:22 PDT 2024
https://github.com/Stylie777 updated https://github.com/llvm/llvm-project/pull/111970
>From 6bdde349fb65ab0298febf723b7642abf6db0bf1 Mon Sep 17 00:00:00 2001
From: Jack Styles <jack.styles at arm.com>
Date: Fri, 4 Oct 2024 15:19:01 +0100
Subject: [PATCH 1/6] [ARM][MC] Add Range Checks for MOVT and MOVW Instructions
As per the ARM ABI, the MOVT and MOVW instructions should have
addends that fall within a 16bit signed range. LLVM does not check
this so it is possible to use addends that are beyond the accepted
range. These addends are silently truncated.
A new check is added to ensure the addend falls within the expected
range, rejecting an addend that falls outside with an error.
Information relating to the ABI requirements can be found here:
https://github.com/ARM-software/abi-aa/blob/main/aaelf32/aaelf32.rst#addends-and-pc-bias-compensation
---
.../Target/ARM/MCTargetDesc/ARMAsmBackend.cpp | 9 +++++++++
llvm/test/MC/ARM/arm-movt-movw-range-fail.s | 13 +++++++++++++
llvm/test/MC/ARM/arm-movt-movw-range-pass.s | 13 +++++++++++++
llvm/test/MC/ARM/macho-movwt.s | 16 ++++++++--------
4 files changed, 43 insertions(+), 8 deletions(-)
create mode 100644 llvm/test/MC/ARM/arm-movt-movw-range-fail.s
create mode 100644 llvm/test/MC/ARM/arm-movt-movw-range-pass.s
diff --git a/llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp b/llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
index 1223210a76f6e3..184bc28a6653b2 100644
--- a/llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
+++ b/llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
@@ -34,6 +34,7 @@
#include "llvm/Support/EndianStream.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/Format.h"
+#include "llvm/Support/MathExtras.h"
#include "llvm/Support/raw_ostream.h"
using namespace llvm;
@@ -472,11 +473,19 @@ unsigned ARMAsmBackend::adjustFixupValue(const MCAssembler &Asm,
case FK_SecRel_4:
return Value;
case ARM::fixup_arm_movt_hi16:
+ if(!(minIntN(16) <= static_cast<int64_t>(Value) && static_cast<int64_t>(Value) <= maxIntN(16))) {
+ Ctx.reportError(Fixup.getLoc(), "Relocation Not In Range");
+ return 0;
+ }
assert(STI != nullptr);
if (IsResolved || !STI->getTargetTriple().isOSBinFormatELF())
Value >>= 16;
[[fallthrough]];
case ARM::fixup_arm_movw_lo16: {
+ if(!(minIntN(16) <= static_cast<int64_t>(Value) && static_cast<int64_t>(Value) <= maxIntN(16))) {
+ Ctx.reportError(Fixup.getLoc(), "Relocation Not In Range");
+ return 0;
+ }
unsigned Hi4 = (Value & 0xF000) >> 12;
unsigned Lo12 = Value & 0x0FFF;
// inst{19-16} = Hi4;
diff --git a/llvm/test/MC/ARM/arm-movt-movw-range-fail.s b/llvm/test/MC/ARM/arm-movt-movw-range-fail.s
new file mode 100644
index 00000000000000..2961b9bfcb64d2
--- /dev/null
+++ b/llvm/test/MC/ARM/arm-movt-movw-range-fail.s
@@ -0,0 +1,13 @@
+ at RUN: not llvm-mc -triple armv7-eabi -filetype obj -o - %s 2>&1 | FileCheck %s
+
+ .global v
+ .text
+ movw r1, #:lower16:v + -65536
+ movt r1, #:upper16:v + 65536
+
+ at CHECK: error: Relocation Not In Range
+ at CHECK: movw r1, #:lower16:v + -65536
+ at CHECK: ^
+ at CHECK: error: Relocation Not In Range
+ at CHECK: movt r1, #:upper16:v + 65536
+ at CHECK: ^
diff --git a/llvm/test/MC/ARM/arm-movt-movw-range-pass.s b/llvm/test/MC/ARM/arm-movt-movw-range-pass.s
new file mode 100644
index 00000000000000..41f19565a46c4a
--- /dev/null
+++ b/llvm/test/MC/ARM/arm-movt-movw-range-pass.s
@@ -0,0 +1,13 @@
+ at RUN: llvm-mc -triple armv7-eabi -filetype obj -o - %s 2>&1 | FileCheck %s
+
+ .global v
+ .text
+ movw r1, #:lower16:v + -20000
+ movt r1, #:upper16:v + 20000
+
+ at CHECK-NOT: error: Relocation Not In Range
+ at CHECK-NOT: movw r1, #:lower16:v + -20000
+ at CHECK-NOT: ^
+ at CHECK-NOT: error: Relocation Not In Range
+ at CHECK-NOT: movt r1, #:upper16:v + 20000
+ at CHECK-NOT: ^
diff --git a/llvm/test/MC/ARM/macho-movwt.s b/llvm/test/MC/ARM/macho-movwt.s
index 6f067cd86dc15d..b2c0587ca7fe59 100644
--- a/llvm/test/MC/ARM/macho-movwt.s
+++ b/llvm/test/MC/ARM/macho-movwt.s
@@ -8,8 +8,8 @@
movw r0, :lower16:_x+4
movt r0, :upper16:_x+4
- movw r0, :lower16:_x+0x10000
- movt r0, :upper16:_x+0x10000
+ movw r0, :lower16:_x+0x1000
+ movt r0, :upper16:_x+0x1000
.arm
movw r0, :lower16:_x
@@ -18,8 +18,8 @@
movw r0, :lower16:_x+4
movt r0, :upper16:_x+4
- movw r0, :lower16:_x+0x10000
- movt r0, :upper16:_x+0x10000
+ movw r0, :lower16:_x+0x1000
+ movt r0, :upper16:_x+0x1000
@ Enter the bizarre world of MachO relocations. First, they're in reverse order
@ to the actual instructions
@@ -30,10 +30,10 @@
@ Third column identifies ARM/Thumb & HI/LO.
@ CHECK: 0x2C 0 1 1 ARM_RELOC_HALF 0 _x
-@ CHECK: 0x0 0 1 0 ARM_RELOC_PAIR 0 -
+@ CHECK: 0x1000 0 1 0 ARM_RELOC_PAIR 0 -
@ CHECK: 0x28 0 0 1 ARM_RELOC_HALF 0 _x
-@ CHECK: 0x1 0 0 0 ARM_RELOC_PAIR 0 -
+@ CHECK: 0x0 0 0 0 ARM_RELOC_PAIR 0 -
@ CHECK: 0x24 0 1 1 ARM_RELOC_HALF 0 _x
@ CHECK: 0x4 0 1 0 ARM_RELOC_PAIR 0 -
@@ -48,10 +48,10 @@
@ CHECK: 0x0 0 0 0 ARM_RELOC_PAIR 0 -
@ CHECK: 0x14 0 3 1 ARM_RELOC_HALF 0 _x
-@ CHECK: 0x0 0 3 0 ARM_RELOC_PAIR 0 -
+@ CHECK: 0x1000 0 3 0 ARM_RELOC_PAIR 0 -
@ CHECK: 0x10 0 2 1 ARM_RELOC_HALF 0 _x
-@ CHECK: 0x1 0 2 0 ARM_RELOC_PAIR 0 -
+@ CHECK: 0x0 0 2 0 ARM_RELOC_PAIR 0 -
@ CHECK: 0xC 0 3 1 ARM_RELOC_HALF 0 _x
@ CHECK: 0x4 0 3 0 ARM_RELOC_PAIR 0 -
>From b052af7b6a246d9ac4b8c54aa790360392a48bb7 Mon Sep 17 00:00:00 2001
From: Jack Styles <jack.styles at arm.com>
Date: Fri, 11 Oct 2024 09:34:24 +0100
Subject: [PATCH 2/6] [ARM] Check for addend before the Value is adjusted
As part of the checks, in some cases the Value is changed according
to if the Thumb bit is needed. This changes the value and can cause
the calcuation to check the range to falsely fail.
Moving the check before the Value is changed to ensure this cannot
occur.
Previously, tests existed that exploited the truncation of the
addend into a 16bit signed value, so these tests have been updated
to reflect the fact this is no longer allowed. It still tests the
instruction, but the expected outcome has been updated with new
values.
---
.../Target/ARM/MCTargetDesc/ARMAsmBackend.cpp | 18 ++++++++++--------
llvm/test/MC/ARM/Windows/mov32t-range.s | 6 +++---
llvm/test/MC/MachO/ARM/thumb2-movw-fixup.s | 10 +++++-----
3 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp b/llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
index 184bc28a6653b2..4d643d001d7541 100644
--- a/llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
+++ b/llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
@@ -447,6 +447,16 @@ unsigned ARMAsmBackend::adjustFixupValue(const MCAssembler &Asm,
const MCSubtargetInfo* STI) const {
unsigned Kind = Fixup.getKind();
+ // For MOVW/MOVT Instructions, the Fixup Value needs to be 16 bit aligned.
+ // If this is not the case, we should reject compilation.
+ if((Kind == ARM::fixup_arm_movw_lo16 || Kind == ARM::fixup_arm_movt_hi16 ||
+ Kind == ARM::fixup_t2_movw_lo16 || Kind == ARM::fixup_t2_movt_hi16) &&
+ (!(minIntN(16) <= static_cast<int64_t>(Value) &&
+ static_cast<int64_t>(Value) <= maxIntN(16)))) {
+ Ctx.reportError(Fixup.getLoc(), "Relocation Not In Range");
+ return 0;
+ }
+
// MachO tries to make .o files that look vaguely pre-linked, so for MOVW/MOVT
// and .word relocations they put the Thumb bit into the addend if possible.
// Other relocation types don't want this bit though (branches couldn't encode
@@ -473,19 +483,11 @@ unsigned ARMAsmBackend::adjustFixupValue(const MCAssembler &Asm,
case FK_SecRel_4:
return Value;
case ARM::fixup_arm_movt_hi16:
- if(!(minIntN(16) <= static_cast<int64_t>(Value) && static_cast<int64_t>(Value) <= maxIntN(16))) {
- Ctx.reportError(Fixup.getLoc(), "Relocation Not In Range");
- return 0;
- }
assert(STI != nullptr);
if (IsResolved || !STI->getTargetTriple().isOSBinFormatELF())
Value >>= 16;
[[fallthrough]];
case ARM::fixup_arm_movw_lo16: {
- if(!(minIntN(16) <= static_cast<int64_t>(Value) && static_cast<int64_t>(Value) <= maxIntN(16))) {
- Ctx.reportError(Fixup.getLoc(), "Relocation Not In Range");
- return 0;
- }
unsigned Hi4 = (Value & 0xF000) >> 12;
unsigned Lo12 = Value & 0x0FFF;
// inst{19-16} = Hi4;
diff --git a/llvm/test/MC/ARM/Windows/mov32t-range.s b/llvm/test/MC/ARM/Windows/mov32t-range.s
index 7e16105cddc6f3..386893a078de40 100644
--- a/llvm/test/MC/ARM/Windows/mov32t-range.s
+++ b/llvm/test/MC/ARM/Windows/mov32t-range.s
@@ -21,7 +21,7 @@ truncation:
.section .rdata,"rd"
.Lbuffer:
- .zero 65536
+ .zero 32767
.Lerange:
.asciz "-erange"
@@ -32,6 +32,6 @@ truncation:
@ CHECK-RELOCATIONS: }
@ CHECK-RELOCATIONS: ]
-@ CHECK-ENCODING: 0: f240 0000
-@ CHECK-ENCODING-NEXT: 4: f2c0 0001
+@ CHECK-ENCODING: 0: f647 70ff
+@ CHECK-ENCODING-NEXT: 4: f2c0 0000
diff --git a/llvm/test/MC/MachO/ARM/thumb2-movw-fixup.s b/llvm/test/MC/MachO/ARM/thumb2-movw-fixup.s
index 18182d1affb063..0d6f38325b1d3c 100644
--- a/llvm/test/MC/MachO/ARM/thumb2-movw-fixup.s
+++ b/llvm/test/MC/MachO/ARM/thumb2-movw-fixup.s
@@ -11,7 +11,7 @@
movt r2, :upper16:L1
movw r12, :lower16:L2
movt r12, :upper16:L2
- .space 70000
+ .space 16382
.data
L1: .long 0
@@ -30,7 +30,7 @@ L2: .long 0
@ CHECK: Section: __data (2)
@ CHECK: }
@ CHECK: Relocation {
-@ CHECK: Offset: 0x1184
+@ CHECK: Offset: 0x4012
@ CHECK: PCRel: 0
@ CHECK: Length: 3
@ CHECK: Type: ARM_RELOC_PAIR (1)
@@ -44,7 +44,7 @@ L2: .long 0
@ CHECK: Section: __data (2)
@ CHECK: }
@ CHECK: Relocation {
-@ CHECK: Offset: 0x1
+@ CHECK: Offset: 0x0
@ CHECK: PCRel: 0
@ CHECK: Length: 2
@ CHECK: Type: ARM_RELOC_PAIR (1)
@@ -58,7 +58,7 @@ L2: .long 0
@ CHECK: Section: __data (2)
@ CHECK: }
@ CHECK: Relocation {
-@ CHECK: Offset: 0x1180
+@ CHECK: Offset: 0x400E
@ CHECK: PCRel: 0
@ CHECK: Length: 3
@ CHECK: Type: ARM_RELOC_PAIR (1)
@@ -72,7 +72,7 @@ L2: .long 0
@ CHECK: Section: __data (2)
@ CHECK: }
@ CHECK: Relocation {
-@ CHECK: Offset: 0x1
+@ CHECK: Offset: 0x0
@ CHECK: PCRel: 0
@ CHECK: Length: 2
@ CHECK: Type: ARM_RELOC_PAIR (1)
>From 81eec5f7dd6acd10383c7afea6647f602206acd5 Mon Sep 17 00:00:00 2001
From: Jack Styles <jack.styles at arm.com>
Date: Fri, 11 Oct 2024 10:27:18 +0100
Subject: [PATCH 3/6] [DOCS] Add Release Note for change
---
llvm/docs/ReleaseNotes.md | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/llvm/docs/ReleaseNotes.md b/llvm/docs/ReleaseNotes.md
index dcdd7a25c7fbee..dc308838c36c6d 100644
--- a/llvm/docs/ReleaseNotes.md
+++ b/llvm/docs/ReleaseNotes.md
@@ -125,6 +125,11 @@ Changes to the ARM Backend
the required alignment space with a sequence of `0x0` bytes (the requested
fill value) rather than NOPs.
+* When using the `MOVT` or `MOVW` instructions, the Assembler will now check to
+ ensure that any addend that is used is within a 16bit Signed value range. If the
+ addend falls outside of this range, the LLVM backend will emit an error like so
+ `Relocation Not In Range`.
+
Changes to the AVR Backend
--------------------------
>From 0e5d9ea7484893a1cb3408ed1863cbf3fb1f2e77 Mon Sep 17 00:00:00 2001
From: Jack Styles <jack.styles at arm.com>
Date: Fri, 11 Oct 2024 10:44:01 +0100
Subject: [PATCH 4/6] fixup! [ARM] Check for addend before the Value is
adjusted
---
llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp b/llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
index 4d643d001d7541..530657b837d166 100644
--- a/llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
+++ b/llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
@@ -449,10 +449,10 @@ unsigned ARMAsmBackend::adjustFixupValue(const MCAssembler &Asm,
// For MOVW/MOVT Instructions, the Fixup Value needs to be 16 bit aligned.
// If this is not the case, we should reject compilation.
- if((Kind == ARM::fixup_arm_movw_lo16 || Kind == ARM::fixup_arm_movt_hi16 ||
- Kind == ARM::fixup_t2_movw_lo16 || Kind == ARM::fixup_t2_movt_hi16) &&
+ if ((Kind == ARM::fixup_arm_movw_lo16 || Kind == ARM::fixup_arm_movt_hi16 ||
+ Kind == ARM::fixup_t2_movw_lo16 || Kind == ARM::fixup_t2_movt_hi16) &&
(!(minIntN(16) <= static_cast<int64_t>(Value) &&
- static_cast<int64_t>(Value) <= maxIntN(16)))) {
+ static_cast<int64_t>(Value) <= maxIntN(16)))) {
Ctx.reportError(Fixup.getLoc(), "Relocation Not In Range");
return 0;
}
>From 835348d2717fa8ee9fa3212f28409241ca1f8c63 Mon Sep 17 00:00:00 2001
From: Jack Styles <jack.styles at arm.com>
Date: Fri, 11 Oct 2024 15:19:17 +0100
Subject: [PATCH 5/6] fixup! [DOCS] Add Release Note for change
---
llvm/docs/ReleaseNotes.md | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/docs/ReleaseNotes.md b/llvm/docs/ReleaseNotes.md
index dc308838c36c6d..6e37852b4574ed 100644
--- a/llvm/docs/ReleaseNotes.md
+++ b/llvm/docs/ReleaseNotes.md
@@ -126,7 +126,7 @@ Changes to the ARM Backend
fill value) rather than NOPs.
* When using the `MOVT` or `MOVW` instructions, the Assembler will now check to
- ensure that any addend that is used is within a 16bit Signed value range. If the
+ ensure that any addend that is used is within a 16-bit signed value range. If the
addend falls outside of this range, the LLVM backend will emit an error like so
`Relocation Not In Range`.
>From c0ed52db6b1fbe84c7d0477121d8e21fec69c88a Mon Sep 17 00:00:00 2001
From: Jack Styles <jack.styles at arm.com>
Date: Fri, 11 Oct 2024 15:19:43 +0100
Subject: [PATCH 6/6] fixup! [ARM] Check for addend before the Value is
adjusted
---
llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp b/llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
index 530657b837d166..eb8cb178d6d21b 100644
--- a/llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
+++ b/llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
@@ -447,12 +447,11 @@ unsigned ARMAsmBackend::adjustFixupValue(const MCAssembler &Asm,
const MCSubtargetInfo* STI) const {
unsigned Kind = Fixup.getKind();
- // For MOVW/MOVT Instructions, the Fixup Value needs to be 16 bit aligned.
- // If this is not the case, we should reject compilation.
+ // For MOVW/MOVT Instructions, the fixup value must already be 16-bit aligned.
if ((Kind == ARM::fixup_arm_movw_lo16 || Kind == ARM::fixup_arm_movt_hi16 ||
Kind == ARM::fixup_t2_movw_lo16 || Kind == ARM::fixup_t2_movt_hi16) &&
- (!(minIntN(16) <= static_cast<int64_t>(Value) &&
- static_cast<int64_t>(Value) <= maxIntN(16)))) {
+ (static_cast<int64_t>(Value) < minIntN(16) ||
+ static_cast<int64_t>(Value) > maxIntN(16))) {
Ctx.reportError(Fixup.getLoc(), "Relocation Not In Range");
return 0;
}
More information about the llvm-commits
mailing list