[v2] elf/riscv: Fix relaxation with aliases [PR28021]

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

Commit Message

Michael Matz July 5, 2021, 4:22 p.m.
Hello,

On Tue, 29 Jun 2021, Nelson Chu wrote:

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


I opted for hardcoding 64bit in the end.

> > 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 figured something out, thanks Andreas for the ld_nm hint.  See below the 
second version of the patch.  I've verified that it figures out the 
correct entries from the nm output (and indeed fails without the 
elfnn-riscv.c change).  Again, tested with a x86-64 to riscv cross (this 
time to riscv32 and riscv64!).

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


So, I'll give it some time for comments and commit it tomorrow or so.


Ciao,
Michael.
P.S: I'm so happy that tcl is fine with '@@' in identifier names, 
otherwise I'd have to have figured out strange quoting rules ;-)

---- 8< ----
Subject: [PATCH] elf/riscv: Fix relaxation with aliases [PR28021]

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.

PR ld/28021

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

ld/
	* 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
	(run_relax_twice_test): New, and call it.
---
 bfd/elfnn-riscv.c                          |  2 +-
 ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp | 45 ++++++++++++++++++++++
 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.ver  | 11 ++++++
 5 files changed, 116 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.ver

-- 
2.23.0

Comments

Andreas Schwab July 5, 2021, 4:27 p.m. | #1
On Jul 05 2021, Michael Matz wrote:

> 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


Is the alignment still needed?

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."
Michael Matz July 5, 2021, 4:33 p.m. | #2
Hello,

On Mon, 5 Jul 2021, Andreas Schwab wrote:

> On Jul 05 2021, Michael Matz wrote:

> 

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

> 

> Is the alignment still needed?


Good point.  It's not, consider it removed from what I'll commit.


Ciao,
Michael.
Nelson Chu July 6, 2021, 2:19 a.m. | #3
Hi Michael,

On Tue, Jul 6, 2021 at 12:22 AM Michael Matz <matz@suse.de> wrote:
> On Tue, 29 Jun 2021, Nelson Chu wrote:

>

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

>

> I opted for hardcoding 64bit in the end.


Thanks, the rv32 binutils tests pass for now.

> I figured something out, thanks Andreas for the ld_nm hint.  See below the

> second version of the patch.  I've verified that it figures out the

> correct entries from the nm output (and indeed fails without the

> elfnn-riscv.c change).  Again, tested with a x86-64 to riscv cross (this

> time to riscv32 and riscv64!).


Wow, your solution should be the right way to check the nm, which I
never noticed.  Thanks, it looks really really good.   Just a minor
thing -

> +    } else {

> +       send_log "foobar_new == $nm_output(foobar_new)\n"

> +       verbose "foobar_new == $nm_output(foobar_new)"

> +       send_log "foobar@@New == $nm_output(foobar@@New)\n"

> +       verbose "foobar@@New == $nm_output(foobar@@New)"

> +       pass $testname

> +    }


showing the results when they are the same looks redundant, but this
is also OK to me.

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

>

> So, I'll give it some time for comments and commit it tomorrow or so.


Thanks, please commit it when you think it's time.

Nelson
Michael Matz July 6, 2021, 1:50 p.m. | #4
Hello,

On Tue, 6 Jul 2021, Nelson Chu wrote:

> > +    } else {

> > +       send_log "foobar_new == $nm_output(foobar_new)\n"

> > +       verbose "foobar_new == $nm_output(foobar_new)"

> > +       send_log "foobar@@New == $nm_output(foobar@@New)\n"

> > +       verbose "foobar@@New == $nm_output(foobar@@New)"

> > +       pass $testname

> > +    }

> 

> showing the results when they are the same looks redundant,


But I was so happy with the tcl hackery :-)  (removed in the commit now)


Thanks,
Michael.
Michael Matz July 6, 2021, 2:02 p.m. | #5
Hello Nick,

On Tue, 6 Jul 2021, Michael Matz wrote:

> > > +    } else {

> > > +       send_log "foobar_new == $nm_output(foobar_new)\n"

> > > +       verbose "foobar_new == $nm_output(foobar_new)"

> > > +       send_log "foobar@@New == $nm_output(foobar@@New)\n"

> > > +       verbose "foobar@@New == $nm_output(foobar@@New)"

> > > +       pass $testname

> > > +    }

> > 

> > showing the results when they are the same looks redundant,

> 

> But I was so happy with the tcl hackery :-)  (removed in the commit now)


May I cherry-pick 235f5ef4a6 into the 2.37 branch?  It's a risc-v bugfix.


Ciao,
Michael.
Libor Bukata via Binutils July 7, 2021, 3:56 p.m. | #6
Hi Michael,

> May I cherry-pick 235f5ef4a6 into the 2.37 branch?  It's a risc-v bugfix.


Please do.

Cheers
   Nick

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..2639358fd20 100644
--- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
+++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
@@ -78,6 +78,50 @@  proc run_dump_test_ifunc { name target output} {
 	      "$name-$target.$ext"]]
 }
 
+proc run_relax_twice_test {} {
+    global as
+    global ld
+    global nm
+    global nm_output
+    global srcdir
+    global subdir
+    global runtests
+
+    set testname "relax-twice"
+    if ![runtest_file_p $runtests $testname] then {
+	return
+    }
+
+    # assemble and link the two input files with a version script, then
+    # capture output of nm and compare addresses of the two symbols
+    # 'foobar_new' and 'foobar@@New'.  They must be equal.
+    # Bitness doesn't matter so we simply force 64bit.
+    if { ![ld_assemble_flags $as "-march=rv64i" $srcdir/$subdir/relax-twice-1.s tmpdir/relax-twice-1.o ]
+	|| ![ld_assemble_flags $as "-march=rv64i" $srcdir/$subdir/relax-twice-2.s tmpdir/relax-twice-2.o]
+	|| ![ld_link $ld tmpdir/relax-twice.so "-m[riscv_choose_lp64_emul] -shared --relax --version-script $srcdir/$subdir/relax-twice.ver tmpdir/relax-twice-1.o tmpdir/relax-twice-2.o"] } {
+	fail $testname
+    } elseif { ![ld_nm $nm "" tmpdir/relax-twice.so] } {
+	fail $testname
+    } elseif { ![info exists nm_output(foobar_new)]
+	       || ![info exists nm_output(foobar@@New)]} {
+	send_log "bad output from nm\n"
+	verbose "bad output from nm"
+	fail $testname
+    } elseif {$nm_output(foobar_new) != $nm_output(foobar@@New)} {
+	send_log "foobar_new == $nm_output(foobar_new)\n"
+	verbose "foobar_new == $nm_output(foobar_new)"
+	send_log "foobar@@New == $nm_output(foobar@@New)\n"
+	verbose "foobar@@New == $nm_output(foobar@@New)"
+	fail $testname
+    } else {
+	send_log "foobar_new == $nm_output(foobar_new)\n"
+	verbose "foobar_new == $nm_output(foobar_new)"
+	send_log "foobar@@New == $nm_output(foobar@@New)\n"
+	verbose "foobar@@New == $nm_output(foobar@@New)"
+	pass $testname
+    }
+}
+
 if [istarget "riscv*-*-*"] {
     run_dump_test "call-relax"
     run_dump_test "pcgp-relax"
@@ -127,6 +171,7 @@  if [istarget "riscv*-*-*"] {
 
     run_dump_test "relro-relax-lui"
     run_dump_test "relro-relax-pcrel"
+    run_relax_twice_test
 
     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.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;