[lld][ELF] Allow Arm PC-relative relocations in PIC links #77304

eleanor-arm · 2024-01-08T13:43:58Z

The relocations that map to R_ARM_PCA are equivalent to R_PC. They are
PC-relative and safe to use in shared libraries, but have a different
relocation code as they are evaluated differently. Now that LLVM may
generate these relocations in object files, they may occur in
shared libraries or position-independent executables.

llvmbot · 2024-01-08T13:44:29Z

@llvm/pr-subscribers-lld-elf

Author: Eleanor Bonnici (eleanor-arm)

Changes

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

1 Files Affected:

(modified) lld/ELF/Relocations.cpp (+1-1)

diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index 210b4d1eb1a7a6..6a7976f41320f1 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -210,7 +210,7 @@ bool lld::elf::needsGot(RelExpr expr) {
 // True if this expression is of the form Sym - X, where X is a position in the
 // file (PC, or GOT for example).
 static bool isRelExpr(RelExpr expr) {
-  return oneof<R_PC, R_GOTREL, R_GOTPLTREL, R_MIPS_GOTREL, R_PPC64_CALL,
+  return oneof<R_PC, R_GOTREL, R_GOTPLTREL, R_ARM_PCA, R_MIPS_GOTREL, R_PPC64_CALL,
                R_PPC64_RELAX_TOC, R_AARCH64_PAGE_PC, R_RELAX_GOT_PC,
                R_RISCV_PC_INDIRECT, R_PPC64_RELAX_GOT_PC, R_LOONGARCH_PAGE_PC>(
       expr);

llvmbot · 2024-01-08T13:44:30Z

@llvm/pr-subscribers-lld

Author: Eleanor Bonnici (eleanor-arm)

Changes

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

1 Files Affected:

(modified) lld/ELF/Relocations.cpp (+1-1)

diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index 210b4d1eb1a7a6..6a7976f41320f1 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -210,7 +210,7 @@ bool lld::elf::needsGot(RelExpr expr) {
 // True if this expression is of the form Sym - X, where X is a position in the
 // file (PC, or GOT for example).
 static bool isRelExpr(RelExpr expr) {
-  return oneof<R_PC, R_GOTREL, R_GOTPLTREL, R_MIPS_GOTREL, R_PPC64_CALL,
+  return oneof<R_PC, R_GOTREL, R_GOTPLTREL, R_ARM_PCA, R_MIPS_GOTREL, R_PPC64_CALL,
                R_PPC64_RELAX_TOC, R_AARCH64_PAGE_PC, R_RELAX_GOT_PC,
                R_RISCV_PC_INDIRECT, R_PPC64_RELAX_GOT_PC, R_LOONGARCH_PAGE_PC>(
       expr);

github-actions · 2024-01-08T13:46:34Z

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

eleanor-arm · 2024-01-08T15:39:02Z

The bolt test results are not affected by this change.

smithp35

The addition of R_PC_A looks good to me.

Please could you add a test?

The tests are in lld/test/ELF . All the arm ones are prefixed with arm- and must start with REQUIRES ARM (so that they are skipped if the ARM backend isn't present).

A suitable test that fails currently:

        .text
        .type _start, %function
        .global _start
        .arm
_start: adr r0, .Ldest
        bx lr
        .section .text.1, "ax", %progbits
.Ldest: .word 10

using:

llvm-mc --triple=armv7-a adr.s -filetype=obj -o adr.o
ld.lld adr.o --shared -o adr.so
ld.lld: error: relocation R_ARM_ALU_PC_G0 cannot be used against symbol '.Ldest'; recompile with -fPIC
>>> defined in adr.o
>>> referenced by adr.o:(.text+0x0)

After your change that error should disappear.

Please can you add a description for the commit message too. Something like.

The relocations that map to R_ARM_PCA are equivalent to R_PC they are PC-relative and safe to use in shared-libraries, but have a different relocation code as they are evaluated
differently. Now that LLVM may generate these relocations in object files they may occur in shared-libries or position-independent executables.

smithp35 · 2024-01-08T17:16:34Z

lld/ELF/Relocations.cpp

@@ -504,8 +504,7 @@ int64_t RelocationScanner::computeMipsAddend(const RelTy &rel, RelExpr expr,
 }

 // Custom error message if Sym is defined in a discarded section.
-template <class ELFT>
-static std::string maybeReportDiscarded(Undefined &sym) {
+template <class ELFT> static std::string maybeReportDiscarded(Undefined &sym) {


Looks like some clang-format changes to unchanged code have made it into the patch. If these are still worth doing can you split these up into a separate [NFC] change mentioning clang-format

eleanor-arm · 2024-01-09T23:42:40Z

Apologies for the force push, it would have been better to do a fixup but the original version is still visible here on the main page.

MaskRay · 2024-01-10T01:08:42Z

lld/test/ELF/arm-adr-pic.s

@@ -0,0 +1,13 @@
+// REQUIRES: arm


Instead of a new file, you can reuse an existing file. You can add a -shared RUN line to arm-adr.s with a comment stating that `/// R_ARM_ALU_PC_G0 referencing a non-preemptible symbol can be used in PIC links.'

Thanks, that's a good idea -> DONE

MaskRay · 2024-01-10T01:11:22Z

Apologies for the force push, it would have been better to do a fixup but the original version is still visible here on the main page.

Some people don't prefer force pushes but it's not a problem for me :) For shorter patches perhaps it matter even less.

The lld/ELF patches usually have a [ELF] tag. Some use a more verbose [lld][ELF].

smithp35

Thanks for the updates. LGTM. Will be worth leaving some time to see if MaskRay has any additional comments.

smithp35

Mentioning this here in case this does not fix the BSD problem. LLD will still give an error message if the relocation is to a pre-emptible symbol (.global in a --shared link). Strictly speaking this is correct as the global can be preempted and the R_ARM_PCA won't generate a dynamic relocation, which could lead to problems if the symbol is preempted.

I'm hoping that if this is the case the BSD case can use hidden visibility for the symbol, which will make it non-pre-emptible.

Adding R_ARM_PCA to the expressions that always calculate static link time constants in https://github.com/llvm/llvm-project/blob/main/lld/ELF/Relocations.cpp#L972 would permit its use to globals in shared libraries, but would be at risk of bugs if the symbol were pre-empted.

jrtc27 · 2024-01-10T18:18:45Z

What "BSD" problem is this? (Semi-interested if it's FreeBSD)

smithp35 · 2024-01-10T18:21:37Z

What "BSD" problem is this? (Semi-interested if it's FreeBSD)

Looks like it is FreeBSD. It was a report that the change to make ADR emit a relocation had broken a BSD program #73834 (comment) (link refers to https://reviews.freebsd.org/D2035)

I think the problem will still occur (if this code can be put in a shared object) as the adr target is .global _C_LABEL(cachebailout)

I'm hoping that .global can be replaced with .hidden or .protected so cachebailout can be visible inside the program, but not preemptible.

jrtc27 · 2024-01-10T18:35:56Z

What "BSD" problem is this? (Semi-interested if it's FreeBSD)

Looks like it is FreeBSD. It was a report that the change to make ADR emit a relocation had broken a BSD program #73834 (comment) (link refers to https://reviews.freebsd.org/D2035)

I think the problem will still occur (if this code can be put in a shared object) as the adr target is .global _C_LABEL(cachebailout)

I'm hoping that .global can be replaced with .hidden or .protected so cachebailout can be visible inside the program, but not preemptible.

That's code in the kernel, so not preemptible, though if the code is still wrong for other reasons it's not a problem to fix it to support newer toolchains, there's normally some amount of that across the tree on each LLVM bump.

(And to answer a question I see in there: FreeBSD uses its vendored Clang+LLD by default, but can be configured to use an external toolchain, whether LLVM, GNU or, in theory (but not really tested) a mix)

MaskRay · 2024-01-10T19:22:46Z

That's code in the kernel, so not preemptible, though if the code is still wrong for other reasons it's not a problem to fix it to support newer toolchains, there's normally some amount of that across the tree on each LLVM bump.

Thanks for the comment. Linux kernel uses -shared -Bsymbolic to make defined symbols non-preemptible. Perhaps FreeBSD kernel uses something similar. Then placing R_ARM_PCA into isRelExpr should create no risk.

MaskRay · 2024-01-10T19:24:27Z

lld/test/ELF/arm-adr.s

@@ -1,6 +1,8 @@
 // REQUIRES: arm
 // RUN: llvm-mc --triple=armv7a-none-eabi --arm-add-build-attributes -filetype=obj -o %t.o %s
 // RUN: ld.lld %t.o -o %t
+/// R_ARM_ALU_PC_G0 referencing a non-preemptible symbol can be used in PIC links.
+// RUN: ld.lld %t.o --shared -o %t.so


If we don't use the output, the convention is /dev/null.

Sometimes inspecting the output is useful (https://maskray.me/blog/2021-08-08-toolchain-testing#the-test-checks-too-little), but this simple change may not be worth the trouble.

MaskRay · 2024-01-10T19:25:36Z

Allow Arm PC-relative relocations in pic or shared library

PIC links include both -pie and -shared.
-shared is for shared libraries.
So "pic or shared library" is redundant.

Perhaps Allow ... in PIC links.

jrtc27 · 2024-01-10T19:25:57Z

That's code in the kernel, so not preemptible, though if the code is still wrong for other reasons it's not a problem to fix it to support newer toolchains, there's normally some amount of that across the tree on each LLVM bump.

Thanks for the comment. Linux kernel uses -shared -Bsymbolic to make defined symbols non-preemptible. Perhaps FreeBSD kernel uses something similar. Then placing R_ARM_PCA into isRelExpr should create no risk.

No -shared, and it uses -Bdynamic.

The relocations that map to R_ARM_PCA are equivalent to R_PC. They are PC-relative and safe to use in shared libraries, but have a different relocation code as they are evaluated differently. Now that LLVM may generate these relocations in object files, they may occur in shared libraries or position-independent executables.

llvmbot added lld lld:ELF labels Jan 8, 2024

eleanor-arm force-pushed the pic branch from b8c4832 to 00e3294 Compare January 8, 2024 13:52

eleanor-arm mentioned this pull request Jan 8, 2024

[llvm][MC][ARM][Assembly] Emit relocations for ADRs and big-endian targets #73834

Merged

smithp35 reviewed Jan 8, 2024

View reviewed changes

eleanor-arm force-pushed the pic branch 2 times, most recently from 8a310b6 to 6d756b9 Compare January 9, 2024 23:40

MaskRay reviewed Jan 10, 2024

View reviewed changes

eleanor-arm force-pushed the pic branch from 6d756b9 to c4c4195 Compare January 10, 2024 11:39

eleanor-arm changed the title ~~Allow Arm PC-relative relocations in pic or shared library~~ [lld][ELF] Allow Arm PC-relative relocations in pic or shared library Jan 10, 2024

smithp35 approved these changes Jan 10, 2024

View reviewed changes

smithp35 reviewed Jan 10, 2024

View reviewed changes

MaskRay approved these changes Jan 10, 2024

View reviewed changes

eleanor-arm changed the title ~~[lld][ELF] Allow Arm PC-relative relocations in pic or shared library~~ [lld][ELF] Allow Arm PC-relative relocations in PIC links Jan 11, 2024

eleanor-arm force-pushed the pic branch from c4c4195 to db1a8c1 Compare January 11, 2024 11:10

eleanor-arm merged commit d21fb06 into llvm:main Jan 11, 2024

eleanor-arm deleted the pic branch January 11, 2024 16:49

[lld][ELF] Allow Arm PC-relative relocations in PIC links #77304

[lld][ELF] Allow Arm PC-relative relocations in PIC links #77304

Uh oh!

Conversation

eleanor-arm commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jan 8, 2024

Uh oh!

llvmbot commented Jan 8, 2024

Uh oh!

github-actions bot commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eleanor-arm commented Jan 8, 2024

Uh oh!

smithp35 left a comment • 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!

smithp35 Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

eleanor-arm commented Jan 9, 2024

Uh oh!

MaskRay Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

eleanor-arm Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

MaskRay commented Jan 10, 2024

Uh oh!

smithp35 left a comment

Choose a reason for hiding this comment

Uh oh!

smithp35 left a comment

Choose a reason for hiding this comment

Uh oh!

jrtc27 commented Jan 10, 2024

Uh oh!

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

Uh oh!

Uh oh!

jrtc27 commented Jan 10, 2024

Uh oh!

MaskRay commented Jan 10, 2024

Uh oh!

MaskRay Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

jrtc27 commented Jan 10, 2024

Uh oh!

Uh oh!

eleanor-arm commented Jan 8, 2024 •

edited

Loading

github-actions bot commented Jan 8, 2024 •

edited

Loading

smithp35 left a comment •

edited

Loading

smithp35 commented Jan 10, 2024 •

edited

Loading

MaskRay commented Jan 10, 2024 •

edited

Loading