[PATCH 0/3] MIPS/LD: Fix high-part relocations in PIC code with absolute symbols

[Maciej W. Rozycki]

Hi,

I have recently tried to build a piece of firmware (Broadcom CFE, for the
SWARM board) that links as PIC code, so that it can move itself around in
memory.

That has failed, relying on LD accepting an R_MIPS_HI16/R_MIPS_LO16
relocation pair against an absolute symbol, which no longer happens as
from commit 861fb55ab50a ("Defer allocation of R_MIPS_REL32 GOT slots"),
<https://sourceware.org/ml/binutils/2008-08/msg00096.html>. I could not
find an easy way around it and absolute symbols are conceptually deferred
link-time constants, so I have decided to correct LD instead and make it
accept R_MIPS_HI16 and similar relocations even in PIC code as long as the
symbol referred is absolute (R_MIPS_26, etc. are still not accepted, as
their run-time calculation is not position-independent).

This turned out to be a simple change, however I decided to take the
opportunity to switch to using `->einfo' rather than `_bfd_error_handler'
for error reporting here, so that location details are provided about the
relocation complained about and any further link issues are also reported
rather than just the first one, so that they can all be observed at once
eliminating the need to discover them iteratively. Then verification
revealed an inconsistency in error message composition, analogous to one
previously addressed with commit 174d0a74a2e6 ("PowerPC/BFD: Convert `%P:
%H:' to `%H:' in error messages").

Hence I made this a small patch series, including 1/3 as a preparatory
change that requires a general maintainer's approval as it applies to
generic linker code. I'll commit 2/3 and 3/3 once 1/3 has been approved.

See individual change descriptions for further details. I have
regression-tested these changes against my usual targets with no problems
observed (i.e. sadly this is not covered anywhere).

Maciej

[Maciej W. Rozycki]

Similarly to commit 174d0a74a2e6 ("PowerPC/BFD: Convert `%P: %H:' to
`%H:' in error messages") convert linker relocation error messages to
use `%H:' rather `%P: %H:', removing inconsistent message composition
like:

$ cat reloc-j.s
.text
.globl foo
.ent foo
foo:
j bar
j bar
.end foo
$ cat reloc-j.ld
SECTIONS
{
bar = 0x12345678;
.text : { *(.text) }
/DISCARD/ : { *(*) }
}
$ as -o reloc-j.o reloc-j.s
$ ld -T reloc-j.ld -o reloc-j reloc-j.o
ld: tmpdir/reloc-j.o: in function `foo':
(.text+0x0): relocation truncated to fit: R_MIPS_26 against `bar'
ld: (.text+0x8): relocation truncated to fit: R_MIPS_26 against `bar'
$

where subsequent lines referring to issues within a single function have
the name of the linker executable prepended, but the first one does not.

As noted with the commit referred this breaks a GNU Coding Standard's
requirement that error messages from compilers should look like this:

source-file-name:lineno: message

also quoted in `vfinfo' code handling these specifiers.

Remove the linker name prefix then, making the messages now look like:

$ ld -T reloc-j.ld -o reloc-j reloc-j.o
tmpdir/reloc-j.o: in function `foo':
(.text+0x0): relocation truncated to fit: R_MIPS_26 against `bar'
(.text+0x8): relocation truncated to fit: R_MIPS_26 against `bar'
$

instead.

ld/
* ldmain.c (reloc_overflow): Use `%H:' rather than `%P: %H:'
with `einfo'.
(reloc_dangerous): Likewise.
(unattached_reloc): Likewise.
---
Hi,

To double-check I have not been doing something silly here I ran GCC over
this broken program:

$ cat bad.c
int foo;

int main(void)
{
return foo();
}
$

and that caused the compiler to report an error with a message structured
similarly to our linker message concerned here, which varied slightly
depending on the version of the compiler used, like:

$ gcc -O2 -o bad bad.c
bad.c: In function 'main':
bad.c:5: error: called object 'foo' is not a function
$

or:

$ gcc -O2 -o bad bad.c
bad.c: In function 'main':
bad.c:5:9: error: called object 'foo' is not a function or function pointer
return foo();
^~~
bad.c:1:5: note: declared here
int foo;
^~~
$

however neither variant of the message was prefixed with the name of the
reporting executable (`cc1'). Which makes me feel confident I am on the
right track.

I think we might want to convert `%P: %C:' similarly, however I do not
have a test case readily available, so I decided not to experiment with
that at this time. A MIPS-specific test case for `reloc_overflow' is
included with 3/3, so at least we'll have minimal coverage.

OK to apply then?

Maciej
---
ld/ldmain.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

binutils-ld-vfinfo-file-null-reloc.diff
Index: src/ld/ldmain.c
===================================================================
--- src.orig/ld/ldmain.c
+++ src/ld/ldmain.c
@@ -1421,7 +1421,7 @@ reloc_overflow (struct bfd_link_info *in
if (overflow_cutoff_limit == -1)
return;

- einfo ("%X%P: %H:", abfd, section, address);
+ einfo ("%X%H:", abfd, section, address);

if (overflow_cutoff_limit >= 0
&& overflow_cutoff_limit-- == 0)
@@ -1474,7 +1474,7 @@ reloc_dangerous (struct bfd_link_info *i
asection *section,
bfd_vma address)
{
- einfo (_("%X%P: %H: dangerous relocation: %s\n"),
+ einfo (_("%X%H: dangerous relocation: %s\n"),
abfd, section, address, message);
}

@@ -1488,7 +1488,7 @@ unattached_reloc (struct bfd_link_info *
asection *section,
bfd_vma address)
{
- einfo (_("%X%P: %H: reloc refers to symbol `%pT' which is not being output\n"),
+ einfo (_("%X%H: reloc refers to symbol `%pT' which is not being output\n"),
abfd, section, address, name);
}

[Alan Modra]

Post by Maciej W. Rozycki
* ldmain.c (reloc_overflow): Use `%H:' rather than `%P: %H:'
with `einfo'.
(reloc_dangerous): Likewise.
(unattached_reloc): Likewise.

OK.

[Maciej W. Rozycki]

Post by Maciej W. Rozycki
* ldmain.c (reloc_overflow): Use `%H:' rather than `%P: %H:'
with `einfo'.
(reloc_dangerous): Likewise.
(unattached_reloc): Likewise.

OK.

Thank you for your review. I have now committed this change along with
the remaining two.

Maciej

[Maciej W. Rozycki]

Switch from `_bfd_error_handler' to `info->callbacks->einfo' with error
reporting concerning the use of position-dependent relocations such as
R_MIPS_HI16 or R_MIPS_26 in PIC code and continue processing so that any
subsequent link errors are also shown rather than the linker terminating
right away. This can reduce user frustration where correcting one error
only reveals another one; instead all are shown together making them all
possible to investigate at once. The use of the `%X' specifier causes
the linker to terminate unsuccessfully at the end of processing.

Also fix the message to say `cannot' rather than `can not'.

bfd/
* elfxx-mips.c (_bfd_mips_elf_check_relocs) <R_MIPS16_26>
<R_MIPS_26, R_MICROMIPS_26_S1>: Use `info->callbacks->einfo'
rather than `_bfd_error_handler' to report refused relocations
in PIC code and continue processing. Fix error message: `can
not' -> `cannot'.
---
bfd/elfxx-mips.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)

binutils-mips-reloc-pic-check-einfo.diff
Index: src/bfd/elfxx-mips.c
===================================================================
--- src.orig/bfd/elfxx-mips.c
+++ src/bfd/elfxx-mips.c
@@ -9077,14 +9077,13 @@ _bfd_mips_elf_check_relocs (bfd *abfd, s
case R_MIPS_26:
case R_MICROMIPS_26_S1:
howto = MIPS_ELF_RTYPE_TO_HOWTO (abfd, r_type, FALSE);
- _bfd_error_handler
+ info->callbacks->einfo
/* xgettext:c-format */
- (_("%pB: relocation %s against `%s' can not be used"
- " when making a shared object; recompile with -fPIC"),
- abfd, howto->name,
+ (_("%X%H: relocation %s against `%s' cannot be used"
+ " when making a shared object; recompile with -fPIC\n"),
+ abfd, sec, rel->r_offset, howto->name,
(h) ? h->root.root.string : "a local symbol");
- bfd_set_error (bfd_error_bad_value);
- return FALSE;
+ break;
default:
break;
}

[Maciej W. Rozycki]

Accept R_MIPS_HI16, R_MIPS_HIGHER and R_MIPS_HIGHEST relocations and
their compressed counterparts in PIC code where the symbol referred is
absolute. Such an operation is meaningful, because an absolute symbol
effectively is a constant the calculation of the value of which has been
deferred to the static link time, and which is not going to change any
further at the dynamic load time. Therefore there is no need ever to
refuse the use of these relocations with such symbols, as the resulting
run-time value observed by the program will be correct even in PIC code.

This is not the case with R_MIPS_26 and its compressed counterparts,
because the run-time value calculated by the instructions these
relocations are used with depends on the address of the instruction
itself, and that can change according to the base address used by the
dynamic loader. Therefore these relocations have to continue being
rejected in PIC code even with absolute symbols.

This allows successful linking of code that relies on previous linker
behavior up to commit 861fb55ab50a ("Defer allocation of R_MIPS_REL32
GOT slots"), <https://sourceware.org/ml/binutils/2008-08/msg00096.html>,
which introduced the problematic check missing this special exception
for absolute symbols.

bfd/
* elfxx-mips.c (_bfd_mips_elf_check_relocs) <R_MIPS16_HI16>
<R_MIPS_HI16, R_MIPS_HIGHER, R_MIPS_HIGHEST, R_MICROMIPS_HI16>
<R_MICROMIPS_HIGHER, R_MICROMIPS_HIGHEST>: Also accept an
absolute symbol in PIC code.

ld/
* testsuite/ld-mips-elf/pic-reloc-0.d: New test.
* testsuite/ld-mips-elf/pic-reloc-1.d: New test.
* testsuite/ld-mips-elf/pic-reloc-2.d: New test.
* testsuite/ld-mips-elf/pic-reloc-3.d: New test.
* testsuite/ld-mips-elf/pic-reloc-4.d: New test.
* testsuite/ld-mips-elf/pic-reloc-absolute-hi.ld: New test
linker script.
* testsuite/ld-mips-elf/pic-reloc-absolute-lo.ld: New test
linker script.
* testsuite/ld-mips-elf/pic-reloc-ordinary.ld: New test linker
script.
* testsuite/ld-mips-elf/pic-reloc-j.s: New test source.
* testsuite/ld-mips-elf/pic-reloc-lui.s: New test source.
* testsuite/ld-mips-elf/mips-elf.exp: Run the new tests.
---
bfd/elfxx-mips.c | 4 ++++
ld/testsuite/ld-mips-elf/mips-elf.exp | 7 +++++++
ld/testsuite/ld-mips-elf/pic-reloc-0.d | 7 +++++++
ld/testsuite/ld-mips-elf/pic-reloc-1.d | 14 ++++++++++++++
ld/testsuite/ld-mips-elf/pic-reloc-2.d | 7 +++++++
ld/testsuite/ld-mips-elf/pic-reloc-3.d | 7 +++++++
ld/testsuite/ld-mips-elf/pic-reloc-4.d | 9 +++++++++
ld/testsuite/ld-mips-elf/pic-reloc-absolute-hi.ld | 6 ++++++
ld/testsuite/ld-mips-elf/pic-reloc-absolute-lo.ld | 6 ++++++
ld/testsuite/ld-mips-elf/pic-reloc-j.s | 11 +++++++++++
ld/testsuite/ld-mips-elf/pic-reloc-lui.s | 13 +++++++++++++
ld/testsuite/ld-mips-elf/pic-reloc-ordinary.ld | 6 ++++++
12 files changed, 97 insertions(+)

binutils-mips-reloc-pic-absolute.diff
Index: src/bfd/elfxx-mips.c
===================================================================
--- src.orig/bfd/elfxx-mips.c
+++ src/bfd/elfxx-mips.c
@@ -9061,6 +9061,10 @@ _bfd_mips_elf_check_relocs (bfd *abfd, s
if (r_symndx == STN_UNDEF)
break;

+ /* Likewise an absolute symbol. */
+ if (bfd_is_abs_symbol (&h->root))
+ break;
+
/* R_MIPS_HI16 against _gp_disp is used for $gp setup,
and has a special meaning. */
if (!NEWABI_P (abfd) && h != NULL
Index: src/ld/testsuite/ld-mips-elf/mips-elf.exp
===================================================================
--- src.orig/ld/testsuite/ld-mips-elf/mips-elf.exp
+++ src/ld/testsuite/ld-mips-elf/mips-elf.exp
@@ -1629,3 +1629,10 @@ if $has_abi(n64) {
run_mips_undefweak_test "shared library (n64, microMIPS, hidden)" \
n64 dso umips hidden
}
+
+# PIC relocation acceptance tests.
+run_dump_test "pic-reloc-0"
+run_dump_test "pic-reloc-1"
+run_dump_test "pic-reloc-2"
+run_dump_test "pic-reloc-3"
+run_dump_test "pic-reloc-4"
Index: src/ld/testsuite/ld-mips-elf/pic-reloc-0.d
===================================================================
--- /dev/null
+++ src/ld/testsuite/ld-mips-elf/pic-reloc-0.d
@@ -0,0 +1,7 @@
+#name: MIPS PIC relocation 0
+#ld: -shared -T pic-reloc-ordinary.ld
+#target: [check_shared_lib_support]
+#source: pic-reloc-lui.s
+#error: \A[^\n]*: in function `foo':\n
+#error: \(\.text\+0x0\): relocation R_MIPS_HI16 against `bar' cannot be used when making a shared object; recompile with -fPIC\n
+#error: \(\.text\+0x8\): relocation R_MIPS_HI16 against `bar' cannot be used when making a shared object; recompile with -fPIC\Z
Index: src/ld/testsuite/ld-mips-elf/pic-reloc-1.d
===================================================================
--- /dev/null
+++ src/ld/testsuite/ld-mips-elf/pic-reloc-1.d
@@ -0,0 +1,14 @@
+#name: MIPS PIC relocation 1
+#ld: -shared -T pic-reloc-absolute-hi.ld
+#objdump: -d --prefix-addresses --show-raw-insn
+#target: [check_shared_lib_support]
+#source: pic-reloc-lui.s
+
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+[0-9a-f]+ <[^>]*> 3c021234 lui v0,0x1234
+[0-9a-f]+ <[^>]*> 24425678 addiu v0,v0,22136
+[0-9a-f]+ <[^>]*> 3c021234 lui v0,0x1234
+[0-9a-f]+ <[^>]*> 24425678 addiu v0,v0,22136
+ \.\.\.
Index: src/ld/testsuite/ld-mips-elf/pic-reloc-2.d
===================================================================
--- /dev/null
+++ src/ld/testsuite/ld-mips-elf/pic-reloc-2.d
@@ -0,0 +1,7 @@
+#name: MIPS PIC relocation 2
+#ld: -shared -T pic-reloc-ordinary.ld
+#target: [check_shared_lib_support]
+#source: pic-reloc-j.s
+#error: \A[^\n]*: in function `foo':\n
+#error: \(\.text\+0x0\): relocation R_MIPS_26 against `bar' cannot be used when making a shared object; recompile with -fPIC\n
+#error: \(\.text\+0x8\): relocation R_MIPS_26 against `bar' cannot be used when making a shared object; recompile with -fPIC\Z
Index: src/ld/testsuite/ld-mips-elf/pic-reloc-3.d
===================================================================
--- /dev/null
+++ src/ld/testsuite/ld-mips-elf/pic-reloc-3.d
@@ -0,0 +1,7 @@
+#name: MIPS PIC relocation 3
+#ld: -shared -T pic-reloc-absolute-lo.ld
+#target: [check_shared_lib_support]
+#source: pic-reloc-j.s
+#error: \A[^\n]*: in function `foo':\n
+#error: \(\.text\+0x0\): relocation R_MIPS_26 against `bar' cannot be used when making a shared object; recompile with -fPIC\n
+#error: \(\.text\+0x8\): relocation R_MIPS_26 against `bar' cannot be used when making a shared object; recompile with -fPIC\Z
Index: src/ld/testsuite/ld-mips-elf/pic-reloc-4.d
===================================================================
--- /dev/null
+++ src/ld/testsuite/ld-mips-elf/pic-reloc-4.d
@@ -0,0 +1,9 @@
+#name: MIPS PIC relocation 4
+#ld: -shared -T pic-reloc-absolute-hi.ld
+#target: [check_shared_lib_support]
+#source: pic-reloc-j.s
+#error: \A[^\n]*: in function `foo':\n
+#error: \(\.text\+0x0\): relocation R_MIPS_26 against `bar' cannot be used when making a shared object; recompile with -fPIC\n
+#error: \(\.text\+0x8\): relocation R_MIPS_26 against `bar' cannot be used when making a shared object; recompile with -fPIC\n
+#error: \(\.text\+0x0\): relocation truncated to fit: R_MIPS_26 against `bar'\n
+#error: \(\.text\+0x8\): relocation truncated to fit: R_MIPS_26 against `bar'\Z
Index: src/ld/testsuite/ld-mips-elf/pic-reloc-absolute-hi.ld
===================================================================
--- /dev/null
+++ src/ld/testsuite/ld-mips-elf/pic-reloc-absolute-hi.ld
@@ -0,0 +1,6 @@
+SECTIONS
+{
+ bar = 0x12345678;
+ .text : { *(.text) }
+ /DISCARD/ : { *(*) }
+}
Index: src/ld/testsuite/ld-mips-elf/pic-reloc-absolute-lo.ld
===================================================================
--- /dev/null
+++ src/ld/testsuite/ld-mips-elf/pic-reloc-absolute-lo.ld
@@ -0,0 +1,6 @@
+SECTIONS
+{
+ bar = 0x2345678;
+ .text : { *(.text) }
+ /DISCARD/ : { *(*) }
+}
Index: src/ld/testsuite/ld-mips-elf/pic-reloc-j.s
===================================================================
--- /dev/null
+++ src/ld/testsuite/ld-mips-elf/pic-reloc-j.s
@@ -0,0 +1,11 @@
+ .text
+ .globl foo
+ .ent foo
+foo:
+ j bar
+ j bar
+ .end foo
+
+# Force some (non-delay-slot) zero bytes, to make 'objdump' print ...
+ .align 4, 0
+ .space 16
Index: src/ld/testsuite/ld-mips-elf/pic-reloc-lui.s
===================================================================
--- /dev/null
+++ src/ld/testsuite/ld-mips-elf/pic-reloc-lui.s
@@ -0,0 +1,13 @@
+ .text
+ .globl foo
+ .ent foo
+foo:
+ lui $2, %hi(bar)
+ addiu $2, %lo(bar)
+ lui $2, %hi(bar)
+ addiu $2, %lo(bar)
+ .end foo
+
+# Force some (non-delay-slot) zero bytes, to make 'objdump' print ...
+ .align 4, 0
+ .space 16
Index: src/ld/testsuite/ld-mips-elf/pic-reloc-ordinary.ld
===================================================================
--- /dev/null
+++ src/ld/testsuite/ld-mips-elf/pic-reloc-ordinary.ld
@@ -0,0 +1,6 @@
+SECTIONS
+{
+ bar = foo;
+ .text : { *(.text) }
+ /DISCARD/ : { *(*) }
+}