elf/riscv: Fix relaxation with aliases [PR28021]

Message ID alpine.LSU.2.20.2106281605320.23996@wotan.suse.de
State New
Headers show
Series
  • elf/riscv: Fix relaxation with aliases [PR28021]
Related show

Commit Message

Michael Matz June 28, 2021, 4:07 p.m.
the fix for PR22756 only changed behaviour for hidden aliases,
but the same situation exists for non-hidden aliases: sym_hashes[]
can contain multiple entries pointing to the same symbol structure
leading to relaxation adjustment to be applied twice.

Fix this by testing for duplicates for everything that looks like it
has a version.

[tested by a testsuite run on a x86-64->riscv64 cross binutils]

PR ld/28021

bfd/
	* elfnn-riscv.c (riscv_relax_delete_bytes): Check for any
	versioning.

ld/
	* testsuite/ld-riscv-elf/relax-twice.d: New.
	* testsuite/ld-riscv-elf/relax-twice.ver: New.
	* testsuite/ld-riscv-elf/relax-twice-1.s: New.
	* testsuite/ld-riscv-elf/relax-twice-2.s: New.
	* testsuite/ld-riscv-elf/ld-riscv-elf.exp: Updated for above.
---
 bfd/elfnn-riscv.c                          |  2 +-
 ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp |  1 +
 ld/testsuite/ld-riscv-elf/relax-twice-1.s  | 15 ++++++++
 ld/testsuite/ld-riscv-elf/relax-twice-2.s  | 44 ++++++++++++++++++++++
 ld/testsuite/ld-riscv-elf/relax-twice.d    | 15 ++++++++
 ld/testsuite/ld-riscv-elf/relax-twice.ver  | 11 ++++++
 6 files changed, 87 insertions(+), 1 deletion(-)
 create mode 100644 ld/testsuite/ld-riscv-elf/relax-twice-1.s
 create mode 100644 ld/testsuite/ld-riscv-elf/relax-twice-2.s
 create mode 100644 ld/testsuite/ld-riscv-elf/relax-twice.d
 create mode 100644 ld/testsuite/ld-riscv-elf/relax-twice.ver

-- 
2.23.0

Comments

Michael Matz June 28, 2021, 4:36 p.m. | #1
Hello,

On Mon, 28 Jun 2021, Michael Matz wrote:

> the fix for PR22756 only changed behaviour for hidden aliases,

> but the same situation exists for non-hidden aliases: sym_hashes[]

> can contain multiple entries pointing to the same symbol structure

> leading to relaxation adjustment to be applied twice.

> 

> Fix this by testing for duplicates for everything that looks like it

> has a version.

> 

> [tested by a testsuite run on a x86-64->riscv64 cross binutils]


Oh, and, I'm unhappy with how I test for the situation, but couldn't find 
a different way.  Ideally I want to test that both symbols have the same 
value in the resulting shared lib.  But that's outside the power of 
regexps, and our test infrastructure doesn't seem to have an easy way for 
such more complete matching.  A runtime test would be easy, but I don't 
want that either.

So, if there are ideas how to test for equality of two symbol addresses 
within our test framework: I'm all ears.


Ciao,
Michael.
Andreas Schwab June 28, 2021, 5:04 p.m. | #2
On Jun 28 2021, Michael Matz wrote:

> So, if there are ideas how to test for equality of two symbol addresses 

> within our test framework: I'm all ears.


You can probably use ld_nm, see ld-scripts/script.exp for example.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."
Nelson Chu June 29, 2021, 4:18 a.m. | #3
Hi,

On Tue, Jun 29, 2021 at 12:36 AM Michael Matz <matz@suse.de> wrote:
> On Mon, 28 Jun 2021, Michael Matz wrote:

>

> > the fix for PR22756 only changed behaviour for hidden aliases,

> > but the same situation exists for non-hidden aliases: sym_hashes[]

> > can contain multiple entries pointing to the same symbol structure

> > leading to relaxation adjustment to be applied twice.

> >

> > Fix this by testing for duplicates for everything that looks like it

> > has a version.


Thanks, it looks reasonable and good.

> > [tested by a testsuite run on a x86-64->riscv64 cross binutils]


I get the errors when running the riscv32 regression, since the ld/sd
are unrecognized by riscv32 toolchain.  There are two solutions we
usually use, one is to support two testcases for riscv32 (use lw/sw)
and riscv64 (use ld/sw), or you can merge them into one source by
using .ifdef and .endif, and then pass --defsym when assembling them.
The other solution is to fix the march, for example, -march=rv64gc.
Both of the two solutions are good to me, so it depends on what you
think is better.

> Oh, and, I'm unhappy with how I test for the situation, but couldn't find

> a different way.  Ideally I want to test that both symbols have the same

> value in the resulting shared lib.  But that's outside the power of

> regexps, and our test infrastructure doesn't seem to have an easy way for

> such more complete matching.  A runtime test would be easy, but I don't

> want that either.

>

> So, if there are ideas how to test for equality of two symbol addresses

> within our test framework: I'm all ears.


Thanks Andreas' suggestion, but I'm not familiar with the ld_nm.  I
usually give a linker script to fix the section addresses and symbols,
then the comparison will be easier.  The addresses of the above test
cases are simple enough to be fixed.  There is only "call sscanf@plt"
will be relaxed to jal, before foobar_new.  So if we force the start
address of the text section to 0xXXX, then the values of foobar_new
and foobar@@new will always be XXX + 0x2c.  It would be more safe if
we add the .option norvc/rvc with the .option pic, but that's fine
since it doesn't affect your example.


However, except for the minor failures of riscv32 toolchain
regressions, the patch looks reasonable and good to me.  Thanks for
the fix, you could commit it after the riscv32 problem is resolved.

Thank you very much
Nelson

Patch

diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
index f206708a9f3..a02f1af02bd 100644
--- a/bfd/elfnn-riscv.c
+++ b/bfd/elfnn-riscv.c
@@ -3993,7 +3993,7 @@  riscv_relax_delete_bytes (bfd *abfd, asection *sec, bfd_vma addr, size_t count,
 	 foo becomes an alias for foo@BAR, and hence they need the same
 	 treatment.  */
       if (link_info->wrap_hash != NULL
-	  || sym_hash->versioned == versioned_hidden)
+	  || sym_hash->versioned != unversioned)
 	{
 	  struct elf_link_hash_entry **cur_sym_hashes;
 
diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
index 1f1245af707..00b320c6494 100644
--- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
+++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
@@ -127,6 +127,7 @@  if [istarget "riscv*-*-*"] {
 
     run_dump_test "relro-relax-lui"
     run_dump_test "relro-relax-pcrel"
+    run_dump_test "relax-twice"
 
     set abis [list rv32gc ilp32 [riscv_choose_ilp32_emul] rv64gc lp64 [riscv_choose_lp64_emul]]
     foreach { arch abi emul } $abis {
diff --git a/ld/testsuite/ld-riscv-elf/relax-twice-1.s b/ld/testsuite/ld-riscv-elf/relax-twice-1.s
new file mode 100644
index 00000000000..0e3b079d64e
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/relax-twice-1.s
@@ -0,0 +1,15 @@ 
+	.file	"<artificial>"
+	.option pic
+	.text
+	# this align is here so that the .text section starts at 0x1000,
+	# in order to make matching of testcase results easier
+	.align 12
+	.globl	foobar_new
+	.weak	foobar_new
+	.type	foobar_new, @function
+foobar_new:
+	jr ra
+	.size	foobar_new, .-foobar_new
+	.symver	foobar_new, foobar@@New
+
+	.section	.note.GNU-stack,"",@progbits
diff --git a/ld/testsuite/ld-riscv-elf/relax-twice-2.s b/ld/testsuite/ld-riscv-elf/relax-twice-2.s
new file mode 100644
index 00000000000..39b82b50d19
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/relax-twice-2.s
@@ -0,0 +1,44 @@ 
+	.file	"<artificial>"
+	.option pic
+	.text
+	.section	.rodata.str1.8,"aMS",@progbits,1
+	.align	3
+.LC0:
+	.string	"%u"
+	.text
+	.align	1
+	.globl	relaxme
+	.type	relaxme, @function
+relaxme:
+	addi	sp,sp,-32
+	addi	a2,sp,12
+	lla	a1,.LC0
+	li	a0,0
+	sd	ra,24(sp)
+	call	sscanf@plt
+	ld	ra,24(sp)
+	addi	sp,sp,32
+	jr	ra
+	.size	relaxme, .-relaxme
+	.align	1
+	.globl	foobar_new
+	.type	foobar_new, @function
+foobar_new:
+	li	a0,1
+	ret
+	.size	foobar_new, .-foobar_new
+	.symver	foobar_new, foobar@@New
+	.align	1
+	.globl	foobar_old
+	.type	foobar_old, @function
+foobar_old:
+	addi	sp,sp,-16
+	sd	ra,8(sp)
+	call	foobar@plt
+	ld	ra,8(sp)
+	snez	a0,a0
+	addi	sp,sp,16
+	jr	ra
+	.size	foobar_old, .-foobar_old
+	.symver	foobar_old, foobar@Old
+	.section	.note.GNU-stack,"",@progbits
diff --git a/ld/testsuite/ld-riscv-elf/relax-twice.d b/ld/testsuite/ld-riscv-elf/relax-twice.d
new file mode 100644
index 00000000000..9bb5d1df468
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/relax-twice.d
@@ -0,0 +1,15 @@ 
+#source: relax-twice-1.s
+#source: relax-twice-2.s
+#as:
+#ld: -shared --relax --version-script relax-twice.ver
+#nm: -n
+
+### remark on modifying this test: what needs to be tested
+### is the fact that 'foobar@@New' and 'foobar_new' have
+### the same address (approximated here by requiring the same
+### hardcoded last three digits).
+
+#...
+[a-f0-9]+02c +T +foobar@@New
+[a-f0-9]+02c +t +foobar_new
+#pass
diff --git a/ld/testsuite/ld-riscv-elf/relax-twice.ver b/ld/testsuite/ld-riscv-elf/relax-twice.ver
new file mode 100644
index 00000000000..a6d80e7b458
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/relax-twice.ver
@@ -0,0 +1,11 @@ 
+Old {
+        global:
+                foobar;
+                relaxme;
+        local:
+                *;
+};
+New {
+        global:
+                foobar;
+} Old;