Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 15, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bcdfa80
[llvm][MC][ARM] Don't autoresolve fixups
eleanor-arm Dec 5, 2023
d331b07
fixup! [llvm][MC][ARM] Don't autoresolve fixups
eleanor-arm Jan 2, 2024
0048c1f
amend! [llvm][MC][ARM] Don't autoresolve fixups
eleanor-arm Jan 2, 2024
57c39d7
Fix "ldr" in the disassembly for vldr
eleanor-arm Jan 15, 2024
2a20e1b
Move ERR check in pcrel-ldrd-diff-section.s closer to instruction
eleanor-arm Jan 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -79,13 +77,14 @@ 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 |
Expand Down Expand Up @@ -140,19 +139,22 @@ 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},
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/ARM/MCTargetDesc/ARMFixupKinds.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
6 changes: 1 addition & 5 deletions llvm/test/MC/ARM/pcrel-global.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,14 @@
@ 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.
@ DISASM-NOT: {{.}}

.syntax unified

.globl foo
foo:
vldr d0, foo @ arm_pcrel_10

.thumb
.thumb_func
.type bar, %function
Expand Down
8 changes: 4 additions & 4 deletions llvm/test/MC/ARM/pcrel-ldr-relocs.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
36 changes: 36 additions & 0 deletions llvm/test/MC/ARM/pcrel-ldrd-diff-section.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
@ RUN: not llvm-mc -filetype=obj --defsym=ERR=1 -o /dev/null %s 2>&1 -triple=thumbv7 | FileCheck %s --check-prefix=ERR
@ 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ 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
.ifdef ERR
@ ERR:[[#@LINE-3]]:5: error: unsupported relocation type
@ ERR:[[#@LINE-3]]:5: error: unsupported relocation type
.endif
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]
39 changes: 39 additions & 0 deletions llvm/test/MC/ARM/pcrel-ldrd-same-section.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ 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>
14 changes: 14 additions & 0 deletions llvm/test/MC/ARM/pcrel-vldr-diff-section.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
@ RUN: not llvm-mc -filetype=obj -o /dev/null %s 2>&1 -triple=armv8.2a-eabi | FileCheck %s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ 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
61 changes: 61 additions & 0 deletions llvm/test/MC/ARM/pcrel-vldr-same-section.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
@ 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: vldr s0, [pc, #44] @ 0x38 <foo>
@ THUMB_OFFSET: vldr d0, [pc, #40] @ 0x38 <foo>
@ THUMB_OFFSET: vldr.16 s0, [pc, #36] @ 0x38 <foo>
@ THUMB_OFFSET: vldr s0, [pc, #32] @ 0x38 <foo>
@ THUMB_OFFSET: vldr d0, [pc, #28] @ 0x38 <foo>
@ THUMB_OFFSET: vldr.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>