[llvm][MC][ARM] Don't autoresolve fixups #76574

eleanor-arm · 2023-12-29T17:51:19Z

Removes logic that caused some fixups to be marked as resolved in the
assembler without actually resolving them. Assembler must either resolve
the fixup, reject the code (error out) or defer the resolution to the
linker. In general assembler can resolve offsets in pcrel instructions
if the symbol referred to is in the same section and it cannot make
assumptions about relative position of sections. For example, when LDRD
instruction in arm mode is encountered, fixup_arm_pcrel_10_unscaled is
raised. Prior to #72873 the
assembler would always mark it as resolved without generating a
relocation. The resulting code would likely be incorrect whenever the
label referred to is in a different section.

This patch finishes the series to prevent incorrect code being generated
for pcrel instructions referring to labels in different sections. This
kind of assembly code is very rare and most likely a user error, so both
options (relocation or error) are acceptable. In previous patches this
was resolved by adding relocations. Here, for VLDR instructions an error
is generated as relocation does not exist for Thumb mode and we wanted
the tool's behaviour to be consistent across modes. In the LDRD case,
Thumb mode does not have a relocation and errors out, but LDRD in Arm
mode generates R_ARM_LDRS_PC_G0 relocation because its fixup kind is
shared with other instructions.

It also fixed the case when ADR is used in the big-endian mode, which is
not covered by the ADR patch.

Patch series:
#72873 - LDRx
#73834 - ADR
this PR - LDRD and VLDR

llvmbot · 2023-12-29T17:51:49Z

@llvm/pr-subscribers-backend-arm

@llvm/pr-subscribers-mc

Author: Eleanor Bonnici (eleanor-arm)

Changes

Removes logic that caused some fixups to be marked as resolved in the
assembler without actually resolving them. Sometimes to resolve these
fixups a relocation was needed, which was not generated, resulting in
invalid code. Now in situations when the fixup cannot be resolved by the
assembler, either a relocation is generated, or an error is produced.

This was partially addressed previously
(#72873,
#73834) specifically for LDRx
and ADR instructions, This patch expands it LDRD and VLDR, which should
cover all instructions affected.

Full diff: https://github.com/llvm/llvm-project/pull/76574.diff

9 Files Affected:

(modified) llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp (+7-9)
(modified) llvm/lib/Target/ARM/MCTargetDesc/ARMFixupKinds.h (+1-1)
(added) llvm/test/MC/ARM/pcrel-arm-ldrd-relocs.s (+30)
(modified) llvm/test/MC/ARM/pcrel-global.s (+1-5)
(modified) llvm/test/MC/ARM/pcrel-ldr-relocs.s (+4-4)
(added) llvm/test/MC/ARM/pcrel-ldrd-same-section.s (+39)
(added) llvm/test/MC/ARM/pcrel-thumb-ldrd-relocs.s (+6)
(added) llvm/test/MC/ARM/pcrel-vldr-diff-section.s (+15)
(added) llvm/test/MC/ARM/pcrel-vldr-same-section.s (+62)

diff --git a/llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp b/llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
index 534434fef5ac80..f1259524b86b64 100644
--- a/llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
+++ b/llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
@@ -67,8 +67,6 @@ ARMAsmBackendELF::getFixupKind(StringRef Name) const {
 }
 
 const MCFixupKindInfo &ARMAsmBackend::getFixupKindInfo(MCFixupKind Kind) const {
-  unsigned IsPCRelConstant =
-      MCFixupKindInfo::FKF_IsPCRel | MCFixupKindInfo::FKF_Constant;
   const static MCFixupKindInfo InfosLE[ARM::NumTargetFixupKinds] = {
       // This table *must* be in the order that the fixup_* kinds are defined in
       // ARMFixupKinds.h.
@@ -79,13 +77,13 @@ const MCFixupKindInfo &ARMAsmBackend::getFixupKindInfo(MCFixupKind Kind) const {
        MCFixupKindInfo::FKF_IsPCRel |
            MCFixupKindInfo::FKF_IsAlignedDownTo32Bits},
       {"fixup_arm_pcrel_10_unscaled", 0, 32, MCFixupKindInfo::FKF_IsPCRel},
-      {"fixup_arm_pcrel_10", 0, 32, IsPCRelConstant},
+      {"fixup_arm_pcrel_10", 0, 32, MCFixupKindInfo::FKF_IsPCRel},
       {"fixup_t2_pcrel_10", 0, 32,
        MCFixupKindInfo::FKF_IsPCRel |
            MCFixupKindInfo::FKF_IsAlignedDownTo32Bits},
       {"fixup_arm_pcrel_9", 0, 32, MCFixupKindInfo::FKF_IsPCRel},
       {"fixup_t2_pcrel_9", 0, 32,
-       IsPCRelConstant | MCFixupKindInfo::FKF_IsAlignedDownTo32Bits},
+       MCFixupKindInfo::FKF_IsPCRel | MCFixupKindInfo::FKF_IsAlignedDownTo32Bits},
       {"fixup_arm_ldst_abs_12", 0, 32, 0},
       {"fixup_thumb_adr_pcrel_10", 0, 8,
        MCFixupKindInfo::FKF_IsPCRel |
@@ -140,19 +138,19 @@ const MCFixupKindInfo &ARMAsmBackend::getFixupKindInfo(MCFixupKind Kind) const {
        MCFixupKindInfo::FKF_IsPCRel |
            MCFixupKindInfo::FKF_IsAlignedDownTo32Bits},
       {"fixup_arm_pcrel_10_unscaled", 0, 32, MCFixupKindInfo::FKF_IsPCRel},
-      {"fixup_arm_pcrel_10", 0, 32, IsPCRelConstant},
+      {"fixup_arm_pcrel_10", 0, 32, MCFixupKindInfo::FKF_IsPCRel},
       {"fixup_t2_pcrel_10", 0, 32,
        MCFixupKindInfo::FKF_IsPCRel |
            MCFixupKindInfo::FKF_IsAlignedDownTo32Bits},
       {"fixup_arm_pcrel_9", 0, 32, MCFixupKindInfo::FKF_IsPCRel},
       {"fixup_t2_pcrel_9", 0, 32,
-       IsPCRelConstant | MCFixupKindInfo::FKF_IsAlignedDownTo32Bits},
+       MCFixupKindInfo::FKF_IsPCRel | MCFixupKindInfo::FKF_IsAlignedDownTo32Bits},
       {"fixup_arm_ldst_abs_12", 0, 32, 0},
       {"fixup_thumb_adr_pcrel_10", 8, 8,
-       IsPCRelConstant | MCFixupKindInfo::FKF_IsAlignedDownTo32Bits},
-      {"fixup_arm_adr_pcrel_12", 0, 32, IsPCRelConstant},
+       MCFixupKindInfo::FKF_IsPCRel | MCFixupKindInfo::FKF_IsAlignedDownTo32Bits},
+      {"fixup_arm_adr_pcrel_12", 0, 32, MCFixupKindInfo::FKF_IsPCRel},
       {"fixup_t2_adr_pcrel_12", 0, 32,
-       IsPCRelConstant | MCFixupKindInfo::FKF_IsAlignedDownTo32Bits},
+       MCFixupKindInfo::FKF_IsPCRel | MCFixupKindInfo::FKF_IsAlignedDownTo32Bits},
       {"fixup_arm_condbranch", 8, 24, MCFixupKindInfo::FKF_IsPCRel},
       {"fixup_arm_uncondbranch", 8, 24, MCFixupKindInfo::FKF_IsPCRel},
       {"fixup_t2_condbranch", 0, 32, MCFixupKindInfo::FKF_IsPCRel},
diff --git a/llvm/lib/Target/ARM/MCTargetDesc/ARMFixupKinds.h b/llvm/lib/Target/ARM/MCTargetDesc/ARMFixupKinds.h
index 3bcea577b9b63b..003d5414fab44f 100644
--- a/llvm/lib/Target/ARM/MCTargetDesc/ARMFixupKinds.h
+++ b/llvm/lib/Target/ARM/MCTargetDesc/ARMFixupKinds.h
@@ -28,7 +28,7 @@ enum Fixups {
   // immediate).
   fixup_arm_pcrel_10,
   // Equivalent to fixup_arm_pcrel_10, accounting for the short-swapped encoding
-  // of Thumb2 instructions.
+  // of Thumb2 instructions. Also used by LDRD in Thumb mode.
   fixup_t2_pcrel_10,
   // 9-bit PC relative relocation for symbol addresses used in VFP instructions
   // where bit 0 not encoded (so it's encoded as an 8-bit immediate).
diff --git a/llvm/test/MC/ARM/pcrel-arm-ldrd-relocs.s b/llvm/test/MC/ARM/pcrel-arm-ldrd-relocs.s
new file mode 100644
index 00000000000000..ab217a9effa6ee
--- /dev/null
+++ b/llvm/test/MC/ARM/pcrel-arm-ldrd-relocs.s
@@ -0,0 +1,30 @@
+@ RUN: llvm-mc -filetype=obj -triple=armv7 %s -o %t
+@ RUN: llvm-readelf -r %t | FileCheck %s --check-prefix=ARM
+@ RUN: llvm-objdump -d --triple=armv7 %t | FileCheck %s --check-prefix=ARM_ADDEND
+@ RUN: llvm-mc -filetype=obj -triple=armebv7 %s -o %t
+@ RUN: llvm-readelf -r %t | FileCheck %s --check-prefix=ARM
+@ RUN: llvm-objdump -d --triple=armebv7 %t | FileCheck %s --check-prefix=ARM_ADDEND
+
+    .section .text.bar, "ax"
+    .balign 4
+    .global bar
+    .type bar, %function
+
+bar:
+    ldrd r0, r1, foo1    @ arm_pcrel_10_unscaled
+    ldrd r0, r1, foo2-8  @ arm_pcrel_10_unscaled
+    bx lr
+
+    .section .data.foo, "a", %progbits
+    .balign 4
+    .global foo1
+    .global foo2
+foo1:
+    .word 0x11223344, 0x55667788
+foo2:
+    .word 0x99aabbcc, 0xddeeff00
+
+@ ARM: R_ARM_LDRS_PC_G0
+
+@ ARM_ADDEND: ldrd r0, r1, [pc, #-8]
+@ ARM_ADDEND: ldrd r0, r1, [pc, #-16]
diff --git a/llvm/test/MC/ARM/pcrel-global.s b/llvm/test/MC/ARM/pcrel-global.s
index 1e9e6e989356ec..702f3d1bfa5b62 100644
--- a/llvm/test/MC/ARM/pcrel-global.s
+++ b/llvm/test/MC/ARM/pcrel-global.s
@@ -7,7 +7,7 @@
 @ CHECK: There are no relocations in this file.
 
 @ DISASM-LABEL: <bar>:
-@ DISASM-NEXT:    ldr     r0, [pc, #0x0]          @ 0x8 <bar+0x4>
+@ DISASM-NEXT:    ldr     r0, [pc, #0x0]          @ 0x4 <bar+0x4>
 @ DISASM-NEXT:    add     r0, pc
 @ DISASM-NEXT:   .word   0xfffffffb
 @@ GNU assembler creates an R_ARM_REL32 referencing bar.
@@ -15,10 +15,6 @@
 
 .syntax unified
 
-.globl foo
-foo:
-vldr d0, foo     @ arm_pcrel_10
-
 .thumb
 .thumb_func
 .type bar, %function
diff --git a/llvm/test/MC/ARM/pcrel-ldr-relocs.s b/llvm/test/MC/ARM/pcrel-ldr-relocs.s
index e0f27f29949993..4a182be128d545 100644
--- a/llvm/test/MC/ARM/pcrel-ldr-relocs.s
+++ b/llvm/test/MC/ARM/pcrel-ldr-relocs.s
@@ -34,10 +34,10 @@
     .global bar
     .type bar, %function
 bar:
-    ldr r0, foo1
-    ldrb r0, foo1
-    ldr r0, foo2-8
-    ldrb r0, foo1+8
+    ldr r0, foo1      @ arm_ldst_pcrel_12 / t2_ldst_pcrel_12
+    ldrb r0, foo1     @ arm_ldst_pcrel_12 / t2_ldst_pcrel_12
+    ldr r0, foo2-8    @ arm_ldst_pcrel_12 / t2_ldst_pcrel_12
+    ldrb r0, foo1+8   @ arm_ldst_pcrel_12 / t2_ldst_pcrel_12
     bx lr
 
     .section .data.foo, "a", %progbits
diff --git a/llvm/test/MC/ARM/pcrel-ldrd-same-section.s b/llvm/test/MC/ARM/pcrel-ldrd-same-section.s
new file mode 100644
index 00000000000000..b176a7562d7818
--- /dev/null
+++ b/llvm/test/MC/ARM/pcrel-ldrd-same-section.s
@@ -0,0 +1,39 @@
+@ RUN: llvm-mc -filetype=obj -o %t %s -triple=armv7
+@ RUN: llvm-readelf -r %t | FileCheck %s --check-prefix=RELOC
+@ RUN: llvm-objdump -d --triple=armv7 %t | FileCheck %s --check-prefix=ARM_OFFSET
+
+@ RUN: llvm-mc -filetype=obj -o %t %s -triple=armebv7
+@ RUN: llvm-readelf -r %t | FileCheck %s --check-prefix=RELOC
+@ RUN: llvm-objdump -d --triple=armebv7  %t | FileCheck %s --check-prefix=ARM_OFFSET
+
+@ RUN: llvm-mc -filetype=obj -o %t %s -triple=thumbv7
+@ RUN: llvm-readelf -r %t | FileCheck %s --check-prefix=RELOC
+@ RUN: llvm-objdump -d --triple=thumbv7  %t | FileCheck %s --check-prefix=THUMB_OFFSET
+
+@ RUN: llvm-mc -filetype=obj -o %y %s -triple=thumbebv7
+@ RUN: llvm-readelf -r %t | FileCheck %s --check-prefix=RELOC
+@ RUN: llvm-objdump -d --triple=thumbebv7 %t | FileCheck %s --check-prefix=THUMB_OFFSET
+
+baz:
+    .word 0x11223344, 0x55667788
+label:
+
+    ldrd r0, r1, foo      @ arm_pcrel_10_unscaled / t2_pcrel_10
+    ldrd r0, r1, bar-8    @ arm_pcrel_10_unscaled / t2_pcrel_10
+
+    ldrd r0, r1, baz      @ arm_pcrel_10_unscaled / t2_pcrel_10
+    ldrd r0, r1, label-8  @ arm_pcrel_10_unscaled / t2_pcrel_10
+foo:
+    .word 0x11223344, 0x55667788
+bar:
+
+@ RELOC: There are no relocations in this file.
+
+@ ARM_OFFSET:   ldrd	r0, r1, [pc, #8]        @ 0x18 <foo>
+@ ARM_OFFSET:   ldrd	r0, r1, [pc, #4]        @ 0x18 <foo>
+@ ARM_OFFSET:   ldrd	r0, r1, [pc, #-24]      @ 0x0 <baz>
+@ ARM_OFFSET:   ldrd	r0, r1, [pc, #-28]      @ 0x0 <baz>
+@ THUMB_OFFSET: ldrd	r0, r1, [pc, #12]       @ 0x18 <foo>
+@ THUMB_OFFSET: ldrd	r0, r1, [pc, #8]        @ 0x18 <foo>
+@ THUMB_OFFSET: ldrd	r0, r1, [pc, #-20]      @ 0x0 <baz>
+@ THUMB_OFFSET: ldrd	r0, r1, [pc, #-24]      @ 0x0 <baz>
diff --git a/llvm/test/MC/ARM/pcrel-thumb-ldrd-relocs.s b/llvm/test/MC/ARM/pcrel-thumb-ldrd-relocs.s
new file mode 100644
index 00000000000000..6b53595b055d56
--- /dev/null
+++ b/llvm/test/MC/ARM/pcrel-thumb-ldrd-relocs.s
@@ -0,0 +1,6 @@
+@ RUN: not llvm-mc -filetype=obj -o /dev/null %s 2>&1 -triple=thumbv7   | FileCheck %s
+@ RUN: not llvm-mc -filetype=obj -o /dev/null %s 2>&1 -triple=thumbebv7 | FileCheck %s
+
+   ldrd r0, r1, foo
+
+@ CHECK: :[[#@LINE-2]]:4: error: unsupported relocation type
diff --git a/llvm/test/MC/ARM/pcrel-vldr-diff-section.s b/llvm/test/MC/ARM/pcrel-vldr-diff-section.s
new file mode 100644
index 00000000000000..aa259b6e95f509
--- /dev/null
+++ b/llvm/test/MC/ARM/pcrel-vldr-diff-section.s
@@ -0,0 +1,15 @@
+@ RUN: not llvm-mc -filetype=obj -o /dev/null %s 2>&1 -triple=armv8.2a-eabi     | FileCheck %s
+@ RUN: not llvm-mc -filetype=obj -o /dev/null %s 2>&1 -triple=armebv8.2a-eabi   | FileCheck %s
+@ RUN: not llvm-mc -filetype=obj -o /dev/null %s 2>&1 -triple=thumbv8.2a-eabi   | FileCheck %s
+@ RUN: not llvm-mc -filetype=obj -o /dev/null %s 2>&1 -triple=thumbebv8.2a-eabi | FileCheck %s
+
+  .arch_extension fp16
+
+vldr s0, foo     @ arm_pcrel_10 / t2_pcrel_10
+vldr d0, foo     @ arm_pcrel_10 / t2_pcrel_10
+vldr.16 s0,foo   @ arm_pcrel_9 / t2_pcrel_9
+
+@ CHECK: :[[#@LINE-4]]:1: error: unsupported relocation type
+@ CHECK: :[[#@LINE-4]]:1: error: unsupported relocation type
+@ CHECK: :[[#@LINE-4]]:1: error: unsupported relocation type
+
diff --git a/llvm/test/MC/ARM/pcrel-vldr-same-section.s b/llvm/test/MC/ARM/pcrel-vldr-same-section.s
new file mode 100644
index 00000000000000..493a34a1e46cbe
--- /dev/null
+++ b/llvm/test/MC/ARM/pcrel-vldr-same-section.s
@@ -0,0 +1,62 @@
+@ RUN: llvm-mc -filetype=obj -o %t %s -triple=armv8.2a-eabi
+@ RUN: llvm-readelf -r %t | FileCheck %s --check-prefix=RELOC
+@ RUN: llvm-objdump -d --triple=armv8.2a-eabi      --mattr=+fullfp16 %t | FileCheck %s --check-prefix=ARM_OFFSET
+@ RUN: llvm-mc -filetype=obj -o %t %s -triple=armebv8.2a-eabi
+@ RUN: llvm-readelf -r %t | FileCheck %s --check-prefix=RELOC
+@ RUN: llvm-objdump -d --triple=armebv8.2a-eabi    --mattr=+fullfp16 %t | FileCheck %s --check-prefix=ARM_OFFSET
+@ RUN: llvm-mc -filetype=obj -o %t %s -triple=thumbv8.2a-eabi
+@ RUN: llvm-readelf -r %t | FileCheck %s --check-prefix=RELOC
+@ RUN: llvm-objdump -d --triple=thumbv8.2a-eabi    --mattr=+fullfp16 %t | FileCheck %s --check-prefix=THUMB_OFFSET
+@ RUN: llvm-mc -filetype=obj -o %y %s -triple=thumbebv8.2a-eabi
+@ RUN: llvm-readelf -r %t | FileCheck %s --check-prefix=RELOC
+@ RUN: llvm-objdump -d --triple=thumbebv8.2a-eabi  --mattr=+fullfp16 %t | FileCheck %s --check-prefix=THUMB_OFFSET
+
+         .arch_extension fp16
+baz:
+    .word 0x11223344, 0x55667788
+label:
+
+    vldr s0, foo     @ arm_pcrel_10 / t2_pcrel_10
+    vldr d0, foo     @ arm_pcrel_10 / t2_pcrel_10
+    vldr.16 s0, foo  @ arm_pcrel_9 / t2_pcrel_9
+    vldr s0, bar-8
+    vldr d0, bar-8
+    vldr.16 s0, bar-8
+    vldr s0, baz
+    vldr d0, baz
+    vldr.16 s0, baz
+    vldr s0, label-8
+    vldr d0, label-8
+    vldr.16 s0, label-8
+
+foo:
+    .word 0x11223344, 0x55667788
+bar:
+
+@ RELOC: There are no relocations in this file.
+
+@ ARM_OFFSET:   vldr	s0, [pc, #40]           @ 0x38 <foo>
+@ ARM_OFFSET:   vldr	d0, [pc, #36]           @ 0x38 <foo>
+@ ARM_OFFSET:   vldr.16	s0, [pc, #32]           @ 0x38 <foo>
+@ ARM_OFFSET:   vldr	s0, [pc, #28]           @ 0x38 <foo>
+@ ARM_OFFSET:   vldr	d0, [pc, #24]           @ 0x38 <foo>
+@ ARM_OFFSET:   vldr.16	s0, [pc, #20]           @ 0x38 <foo>
+@ ARM_OFFSET:   vldr	s0, [pc, #-40]          @ 0x0 <baz>
+@ ARM_OFFSET:   vldr	d0, [pc, #-44]          @ 0x0 <baz>
+@ ARM_OFFSET:   vldr.16	s0, [pc, #-48]          @ 0x0 <baz>
+@ ARM_OFFSET:   vldr	s0, [pc, #-52]          @ 0x0 <baz>
+@ ARM_OFFSET:   vldr	d0, [pc, #-56]          @ 0x0 <baz>
+@ ARM_OFFSET:   vldr.16	s0, [pc, #-60]          @ 0x0 <baz>
+@ THUMB_OFFSET: ldr	s0, [pc, #44]           @ 0x38 <foo>
+@ THUMB_OFFSET: ldr	d0, [pc, #40]           @ 0x38 <foo>
+@ THUMB_OFFSET: ldr.16	s0, [pc, #36]           @ 0x38 <foo>
+@ THUMB_OFFSET: ldr	s0, [pc, #32]           @ 0x38 <foo>
+@ THUMB_OFFSET: ldr	d0, [pc, #28]           @ 0x38 <foo>
+@ THUMB_OFFSET: ldr.16	s0, [pc, #24]           @ 0x38 <foo>
+@ THUMB_OFFSET: vldr	s0, [pc, #-36]          @ 0x0 <baz>
+@ THUMB_OFFSET: vldr	d0, [pc, #-40]          @ 0x0 <baz>
+@ THUMB_OFFSET: vldr.16	s0, [pc, #-44]          @ 0x0 <baz>
+@ THUMB_OFFSET: vldr	s0, [pc, #-48]          @ 0x0 <baz>
+@ THUMB_OFFSET: vldr	d0, [pc, #-52]          @ 0x0 <baz>
+@ THUMB_OFFSET: vldr.16	s0, [pc, #-56]          @ 0x0 <baz>
+

github-actions · 2023-12-29T17:53:37Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Removes logic that caused some fixups to be marked as resolved in the assembler without actually resolving them. Sometimes to resolve these fixups a relocation was needed, which was not generated, resulting in invalid code. Now in situations when the fixup cannot be resolved by the assembler, either a relocation is generated, or an error is produced. This was partially addressed previously (llvm#72873, llvm#73834) specifically for LDRx and ADR instructions, This patch expands it LDRD and VLDR, which should cover all instructions affected.

llvm/test/MC/ARM/pcrel-global.s

MaskRay · 2024-01-01T15:03:30Z

llvm/test/MC/ARM/pcrel-thumb-ldrd-relocs.s

+@ RUN: not llvm-mc -filetype=obj -o /dev/null %s 2>&1 -triple=thumbv7   | FileCheck %s
+@ RUN: not llvm-mc -filetype=obj -o /dev/null %s 2>&1 -triple=thumbebv7 | FileCheck %s
+
+   ldrd r0, r1, foo


Is this related to positive tests in another test file? If true, we can merge the test files by utilizing .ifdef ERR and --defsym ERR=1. Grep will a few examples.

Thanks for the tip. I've merged pcrel-thumb-ldrd-relocs.s and pcrel-arm-ldrd-relocs.s into pcrel-ldrd-diff-section.s

MaskRay · 2024-01-01T15:04:08Z

llvm/test/MC/ARM/pcrel-thumb-ldrd-relocs.s

+
+   ldrd r0, r1, foo
+
+@ CHECK: :[[#@LINE-2]]:4: error: unsupported relocation type


The CHECK is usually immediately before or after the instruction, so that we can use -1 +1 instead of -N +N

As this check has been merged into pcrel-ldrd-diff-section.s I found it quite hard to read when it was placed next to the instructions, with all other checks in the same file. I placed it at the of the file next to the other checks.

MaskRay · 2024-01-01T15:04:23Z

llvm/test/MC/ARM/pcrel-vldr-diff-section.s

+@ CHECK: :[[#@LINE-4]]:1: error: unsupported relocation type
+@ CHECK: :[[#@LINE-4]]:1: error: unsupported relocation type
+@ CHECK: :[[#@LINE-4]]:1: error: unsupported relocation type
+


no blank line at the end

Leaving a blank line at the end of file is fairly common, some tools add it automatically and older tools might expect it to be there. I think we have a mixture of both in this repo. I don't think it really matters, so my preference would be to leave it.

Update: As I was making a change anyway, I also removed the blank lines.

Leaving a comment at the end of file is very uncommon in the repository. A small set of very old tests may do this. If you are modifying lines at the end, just remove the blank line; otherwise, you can keep the EOL untouched.

MaskRay · 2024-01-01T15:05:46Z

Removes logic that caused some fixups to be marked as resolved in the assembler without actually resolving them.

Maybe worth updating the description to include an example like

xxx  // previously incorrectly resolved, now ...

eleanor-arm

Review comments addressed.

llvm/test/MC/ARM/pcrel-global.s

eleanor-arm · 2024-01-02T10:12:37Z

llvm/test/MC/ARM/pcrel-vldr-diff-section.s

+@ CHECK: :[[#@LINE-4]]:1: error: unsupported relocation type
+@ CHECK: :[[#@LINE-4]]:1: error: unsupported relocation type
+@ CHECK: :[[#@LINE-4]]:1: error: unsupported relocation type
+


Leaving a blank line at the end of file is fairly common, some tools add it automatically and older tools might expect it to be there. I think we have a mixture of both in this repo. I don't think it really matters, so my preference would be to leave it.

eleanor-arm · 2024-01-02T11:04:29Z

llvm/test/MC/ARM/pcrel-vldr-diff-section.s

+@ CHECK: :[[#@LINE-4]]:1: error: unsupported relocation type
+@ CHECK: :[[#@LINE-4]]:1: error: unsupported relocation type
+@ CHECK: :[[#@LINE-4]]:1: error: unsupported relocation type
+


Update: As I was making a change anyway, I also removed the blank lines.

eleanor-arm · 2024-01-02T11:07:59Z

llvm/test/MC/ARM/pcrel-thumb-ldrd-relocs.s

+@ RUN: not llvm-mc -filetype=obj -o /dev/null %s 2>&1 -triple=thumbv7   | FileCheck %s
+@ RUN: not llvm-mc -filetype=obj -o /dev/null %s 2>&1 -triple=thumbebv7 | FileCheck %s
+
+   ldrd r0, r1, foo


Thanks for the tip. I've merged pcrel-thumb-ldrd-relocs.s and pcrel-arm-ldrd-relocs.s into pcrel-ldrd-diff-section.s

eleanor-arm · 2024-01-02T11:12:35Z

llvm/test/MC/ARM/pcrel-thumb-ldrd-relocs.s

+
+   ldrd r0, r1, foo
+
+@ CHECK: :[[#@LINE-2]]:4: error: unsupported relocation type


As this check has been merged into pcrel-ldrd-diff-section.s I found it quite hard to read when it was placed next to the instructions, with all other checks in the same file. I placed it at the of the file next to the other checks.

[llvm][MC][ARM] Don't autoresolve fixups Removes logic that caused some fixups to be marked as resolved in the assembler without actually resolving them. Assembler must either resolve the fixup, reject the code (error out) or defer the resolution to the linker. In general assembler can resolve offsets in pcrel instructions if the symbol referred to is in the same section and it cannot make assumptions about relative position of sections. For example, when LDRD instruction in arm mode is encountered, fixup_arm_pcrel_10_unscaled is raised. Prior to llvm#72873 the assembler would always mark it as resolved without generating a relocation. The resulting code would likely be incorrect whenever the label referred to is in a different section. This patch finishes the series to prevent incorrect code being generated for pcrel instructions referring to labels in different sections. This kind of assembly code is very rare and most likely a user error, so both options (relocation or error) are acceptable. Previously this was resolved by adding relocations. Here, for VLDR instructions an error is generated as relocation does not exist for Thumb mode and we wanted the tool's behaviour to be consistent across modes. In the LDRD case, Thumb mode does not have a relocation and errors out, but LDRD in Arm mode generates relocation because its fixup kind is shared with other instructions. Patch series: llvm#72873 - LDRx llvm#73834 - ADR this PR - LDRD and VLDR

eleanor-arm · 2024-01-02T12:42:44Z

I also elaborated more in the description.

MaskRay

Please can you edit the description to not indent the whole message by 4? GitHub renders the whole message as code block. Can you also write the exact relocation type names in the description so that it's easier to search? The fixup kinds are internal and users don't grep them for a behavior change.

MaskRay · 2024-01-11T23:22:09Z

llvm/test/MC/ARM/pcrel-ldrd-diff-section.s

+@ RUN: not llvm-mc -filetype=obj --defsym=ERR=1 -o /dev/null %s 2>&1 -triple=thumbebv7 | FileCheck %s --check-prefix=ERR
+@ RUN: llvm-mc -filetype=obj -triple=armv7 %s -o %t
+@ RUN: llvm-readelf -r %t | FileCheck %s --check-prefix=ARM
+@ RUN: llvm-objdump -d --triple=armv7 %t | FileCheck %s --check-prefix=ARM_ADDEND


llvm-objdump -d -r --no-show-raw-insn dumps inline relocation when printing disassembly code. This allows to remove llvm-readelf

I know this is another way to do it but is it really better? I think using Filecheck can easily get confusing, so matching two simple strings seems like a better idea, in general, compared to matching one more complicated one (especially since the tools' output is in flux and we don't want big parts of tests suddenly failing because of a change in the output format of a tool). I copied that idea from other Arm assembly tests in there.

MaskRay · 2024-01-11T23:23:25Z

llvm/test/MC/ARM/pcrel-ldrd-same-section.s

+@ RUN: llvm-readelf -r %t | FileCheck %s --check-prefix=RELOC
+@ RUN: llvm-objdump -d --triple=armv7 %t | FileCheck %s --check-prefix=ARM_OFFSET
+
+@ RUN: llvm-mc -filetype=obj -o %t %s -triple=armebv7


Does it need to be replicated for {arm,thunb}x{le,be}?

Testing (arm,le) + (thumb,be) may suffice, if you think thumb is worth separate testing (in some cases it's not).

I don't think it hurts. When/if these start failing it might be helpful with debug. Internally all these 4 go through different logic to resolve the fixup. Arm/Thumb have separate fixup kinds and and BE/LE have different way of assigning FixupKindInfo in ARMAsmBackend.cpp.

MaskRay · 2024-01-11T23:28:26Z

llvm/test/MC/ARM/pcrel-vldr-diff-section.s

@@ -0,0 +1,14 @@
+@ RUN: not llvm-mc -filetype=obj -o /dev/null %s 2>&1 -triple=armv8.2a-eabi     | FileCheck %s


same-section and diff-section tests can be in one file. llvm/test/MC/ARM/pcrel-vldr-same-section.s contains a lot of details that seem unrelated to the main purpose of the patch and should be reduced. There are also lots of instructions that may contain duplicates.

How would you put them in the same test? Vldr_same/diff_section.s have slightly different code, one compiles, the other one doesn't. If I merge them it will fail as a whole.

As for the "unrelated details", I think you're referring to the checks of the addend values? And/or the symbol+imm checks? We've done something similar in all the tests related to pcrel relocations. It was recommended to me as a good extra check by @smithp35 . Here I just added a few more because we had a discussion on another review about whether an issue could be related to a pcrel instruction referring to a symbol before it rather than after. It wasn't but still I covered both cases from then on as to make it clearer.

I was thinking of unnecessary instructions. It seems that the latest patch removed them. Thanks

MaskRay · 2024-01-15T18:17:37Z

llvm/test/MC/ARM/pcrel-ldrd-diff-section.s

+    .word 0x99aabbcc, 0xddeeff00
+
+.ifdef ERR
+  @ ERR:[[#@LINE-14]]:5: error: unsupported relocation type


-14 is too far way from the instruction. Move this closer to the instructions so that it's easier to fix an issue if a new line is added.

Removes logic that caused some fixups to be marked as resolved in the assembler without actually resolving them. Assembler must either resolve the fixup, reject the code (error out) or defer the resolution to the linker. In general assembler can resolve offsets in pcrel instructions if the symbol referred to is in the same section and it cannot make assumptions about relative position of sections. For example, when LDRD instruction in arm mode is encountered, fixup_arm_pcrel_10_unscaled is raised. Prior to llvm#72873 the assembler would always mark it as resolved without generating a relocation. The resulting code would likely be incorrect whenever the label referred to is in a different section. This patch finishes the series to prevent incorrect code being generated for pcrel instructions referring to labels in different sections. This kind of assembly code is very rare and most likely a user error, so both options (relocation or error) are acceptable. In previous patches this was resolved by adding relocations. Here, for VLDR instructions an error is generated as relocation does not exist for Thumb mode and we wanted the tool's behaviour to be consistent across modes. In the LDRD case, Thumb mode does not have a relocation and errors out, but LDRD in Arm mode generates R_ARM_LDRS_PC_G0 relocation because its fixup kind is shared with other instructions. It also fixed the case when ADR is used in the big-endian mode, which is not covered by the ADR patch. Patch series: llvm#72873 - LDRx llvm#73834 - ADR this PR - LDRD and VLDR

This was an ARM workaround, which has been removed by #76574

This was an ARM workaround, which has been removed by llvm#76574

llvmbot added backend:ARM mc Machine (object) code labels Dec 29, 2023

eleanor-arm force-pushed the vldr_ldrd branch from 95f0421 to bcdfa80 Compare December 29, 2023 17:58

eleanor-arm requested a review from MaskRay December 29, 2023 18:03

MaskRay reviewed Jan 1, 2024

View reviewed changes

llvm/test/MC/ARM/pcrel-global.s Show resolved Hide resolved

MaskRay reviewed Jan 1, 2024

View reviewed changes

fixup! [llvm][MC][ARM] Don't autoresolve fixups

d331b07

eleanor-arm commented Jan 2, 2024

View reviewed changes

eleanor-arm force-pushed the vldr_ldrd branch from 5708289 to 0048c1f Compare January 2, 2024 12:40

eleanor-arm requested a review from MaskRay January 4, 2024 09:53

MaskRay reviewed Jan 11, 2024

View reviewed changes

Fix "ldr" in the disassembly for vldr

57c39d7

MaskRay reviewed Jan 15, 2024

View reviewed changes

MaskRay approved these changes Jan 15, 2024

View reviewed changes

Move ERR check in pcrel-ldrd-diff-section.s closer to instruction

2a20e1b

eleanor-arm merged commit c0944f5 into llvm:main Jan 15, 2024

eleanor-arm deleted the vldr_ldrd branch January 15, 2024 18:41

MaskRay added a commit that referenced this pull request May 20, 2025

MC: Remove unused MCFixupKindInfo::FKF_Constant

95202ab

This was an ARM workaround, which has been removed by #76574

sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Jun 3, 2025

MC: Remove unused MCFixupKindInfo::FKF_Constant

3f9c36e

This was an ARM workaround, which has been removed by llvm#76574

ajaden-codes pushed a commit to Jaddyen/llvm-project that referenced this pull request Jun 6, 2025

MC: Remove unused MCFixupKindInfo::FKF_Constant

08bab8d

This was an ARM workaround, which has been removed by llvm#76574


		ldrd r0, r1, foo

		@ CHECK: :[[#@LINE-2]]:4: error: unsupported relocation type

		@@ -0,0 +1,14 @@
		@ RUN: not llvm-mc -filetype=obj -o /dev/null %s 2>&1 -triple=armv8.2a-eabi \| FileCheck %s

[llvm][MC][ARM] Don't autoresolve fixups #76574

[llvm][MC][ARM] Don't autoresolve fixups #76574

Uh oh!

Conversation

eleanor-arm commented Dec 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Dec 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaskRay Jan 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaskRay commented Jan 1, 2024

Uh oh!

eleanor-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eleanor-arm commented Jan 2, 2024

Uh oh!

MaskRay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eleanor-arm commented Dec 29, 2023 •

edited

Loading

llvmbot commented Dec 29, 2023 •

edited

Loading

github-actions bot commented Dec 29, 2023 •

edited

Loading

MaskRay Jan 2, 2024 •

edited

Loading