Re: [PATCH][PR ld/10144] MIPS/BFD: Don't make debug section relocs dynamic

["Maciej W. Rozycki" <macro at codesourcery dot com>]

Hi Richard,

 Back to this change as I think it would be best fixed before 2.21 is out.

> >  There is a bug in BFD for MIPS ELF targets that affects symbol processing 
> > by the linker.  The prerequisite is a pair objects being linked together 
> > -- a shared object and a relocatable object -- for the purpose of creating 
> > an executable.  They have to multiply-define a symbol, which has to be a 
> > regular definition in the shared object and a common one in the 
> > relocatable object.  Additionally there has to be a relocation from an 
> > unallocatable (e.g. debug) section referring to the symbol in the 
> > relocatable object.
> >
> >  If a symbol is multiply-defined like this, then the regular definition 
> > should override the common one and the latter should be converted to an 
> > undefined reference.  The relocation, if from a debug section, can be 
> > discarded and resolve to nil as the symbol debug records describe is no 
> > longer there; this is what other targets do and also what the MIPS target 
> > did before non-PIC support was added.  If not from a debug section, then 
> > other targets complain and fail, because a dynamic relocation cannot work 
> > from an unallocatable section.
> >
> >  What the MIPS target does now is as follows:
> >
> > 1. If copy relocations are enabled, then the common definition is retained 
> >    in the relocatable object and the relocation is statically resolved 
> >    against it.  This invalid symbol definition may be discarded by the 
> >    dynamic linker, covering the bug; I have not checked it.
> >
> > 2. If copy relocations are disabled, then the static link fails, 
> >    complaining about the inability to convert the relocation to a dynamic 
> >    one.
> >
> >  Here is a fix that works for me.  Following the practice from other 
> > targets I decided to explicitly check for the SEC_DEBUGGING flag (rather 
> > than SEC_ALLOC).  This makes our code work for the two cases mentioned 
> > above as proved by the test case below.
> >
> >  The case of a non-debug non-alloc section is not covered; this is a rare 
> > corner case and I haven't looked into it (not that it shouldn't be fixed).
> 
> This is what worries me.  The first part of the comment, which I realise
> you've taken from elsewhere:
> 
> > +	  /* Ignore relocs from SEC_DEBUGGING sections because such
> > +	     sections are not SEC_ALLOC and thus ld.so will not process
> > +	     them.  Don't set has_static_relocs for the corresponding
> > +	     symbol.
> > +
> > +	     This is needed in cases such as a global symbol definition
> > +	     in a shared library causing a common symbol from an object
> > +	     file to be converted to an undefined reference.  If that
> > +	     happens, then all the relocations against this symbol from
> > +	     SEC_DEBUGGING sections in the object file will resolve to
> > +	     nil.  */
> > +	  if ((sec->flags & SEC_DEBUGGING) != 0)
> > +	    break;
> >  	  /* Fall through.  */
> 
> makes it sound like we're doing something that would be acceptable for
> all !SEC_ALLOC sections, which then raises the question why we're only
> testing SEC_DEBUGGING.  If I've understood correctly, we're really
> saying something like "We know what debugging sections are for.  It's
> OK to silently set external addresses to 0: at worst it will impair
> the debugging information."  I.e. it's _not_ something we can safely
> apply to all SEC_ALLOC sections.

 Yes, that's what I inferred applies elsewhere.  The SEC_ALLOC attribute 
is referred to, because it's the lack of it that is causing problems, but 
if the SEC_DEBUGGING attribute is set at the same time, then we can avoid 
them at a negligible cost.

> There also seems to be the implicit judgement call: "If the only
> reference to a symbol is via debugging information, it's not worth
> creating a copy reloc for it."  That has nothing to do with ld.so;
> it's a decision we can make entirely in the static linker.  And it's
> a perfectly sensible call of course, and maybe the thinking was that
> it was so obvious it didn't need to be stated, but still, the comment
> seems to gloss over the rationale.

 IMHO creating a copy reloc for a read-only data object is the wrong thing 
to do too in the first place, as far as I understand copy reloc's 
semantics -- the original segment's write protection attribute will not be 
carried over to the copy of the symbol (and propagated to the MMU via 
mprotect(2) as applicable) as this is not the intended use of the copy 
reloc, right?

> (E.g. with the simple case you're describing:
> 
> --------------------------------------------------------------
> cat <<EOF >foo.c
> int x = 1;
> EOF
> cat <<EOF >bar.c
> int x;
> int main (void)
> {
>   return 0;
> }
> EOF
> cat <<EOF >commands
> break main
> run
> print x
> EOF
> gcc foo.c -o foo.so -shared -g
> gcc bar.c foo.so -o bar -g
> LD_LIBRARY_PATH=. gdb --batch --command=commands bar
> --------------------------------------------------------------
> 
> I get:
> 
> --------------------------------------------------------------
> Breakpoint 1 at 0x4005c8: file bar.c, line 4.
> 
> Breakpoint 1, main () at bar.c:4
> 4         return 0;
> /tmp/commands:3: Error in sourced command file:
> Cannot access memory at address 0x0
> --------------------------------------------------------------
> 
> on my x86_64 box.  With a copy reloc I'd be able to see "1" instead.)
> 
> All of which is fine with me.  And even if it wasn't, I wouldn't
> suggest MIPS should be different from other targets.  It's just that
> the comments we're using to justify the code seem a little lacking.

 Frankly what I would be comfortable with the most is an arrangement where 
we would simply discard the corresponding DWARF-2 records altogether.  My 
understanding is LD is not immediately capable of doing such section 
surgery.

> So with all that, the patch is OK, thanks.  However, I think you
> could change:
> 
> 	.ifdef	ELF64
> 	.8byte	foo
> 	.else
> 	.4byte	foo
> 	.endif
> 
> to:
> 
> 	dc.a	foo
> 
> and avoid the ELF64 symbol.  If that fails for any target, I think we
> could legitimately say it's a bug in the target and Not Your Fault.

 Sneaky!  I didn't know about this pseudo-op; I wouldn't mind if such bits 
were actually documented, sigh...  Is this meant to emit a data entity of 
the address size?  If so, then it's broken for MIPS/n32 -- it produces a 
64-bit relocatable field.  Given this obvious failure, how confident do 
you feel this directive is reliable across other targets?  It looks like a 
relatively recent addition to me.

 The MIPS/n32 failure may be trivial to fix, but I may not have time 
immediately available to look into it.  I'll see what I can do though.

> Also, in the MIPS test:
> 
> +# Exclude non-ELF targets.
> +if ![is_elf_format] {
> +    return
> +}
> +
> +# Exclude non-Linux targets; feel free to include your favourite one
> +# if you like.
> +if ![istarget mips*-*-linux*] {
> +    return
> +}
> 
> I think we can safely dispense with the first check.  I realise you're
> doing it for consistency with the target-independent version,
> but I wouldn't expect to have to test both of these elsewhere
> in ld-mips-elf.

 My understanding is the base framework just runs */*.exp from within 
ld/testsuite/ and it is the responsibility of the scripts themselves to 
filter the targets of concern.  Certainly the two preexisting scripts in 
ld-mips-elf/ do check for ![is_elf_format] explicitly.

 I have updated the comment as well taking your concerns into account.  
Please let me know if the new comment -- a bit lengthy, but there you go 
-- sounds reasonable to you.  I have regression-tested the new change for 
the mips-sde-elf target with no problems.

2010-11-04  Maciej W. Rozycki  <macro@codesourcery.com>

	PR ld/10144

	bfd/
	* elfxx-mips.c (_bfd_mips_elf_check_relocs)
	[R_MIPS_32, R_MIPS_REL32, R_MIPS_64]: Ignore relocs from
	SEC_DEBUGGING sections.

	ld/testsuite/
	* lib/ld-lib.exp (run_ld_link_tests): Handle sources from other
	directories.
	(run_ld_link_exec_tests): Likewise.
	(run_cc_link_tests): Likewise.
	* ld-elf/comm-data1.sd: New test.
	* ld-elf/comm-data1.s: Source for the new test.
	* ld-elf/comm-data2.sd: New test.
	* ld-elf/comm-data2.rd: Likewise.
	* ld-elf/comm-data2.xd: Likewise.
	* ld-elf/comm-data2.s: Source for the new tests.
	* ld-elf/comm-data.exp: Run the new tests.
	* ld-mips-elf/comm-data.exp: Likewise.

  Maciej

binutils-mips-debug-reloc.patch
Index: binutils-fsf-trunk-quilt/bfd/elfxx-mips.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/bfd/elfxx-mips.c	2010-11-04 14:26:19.000000000 +0000
+++ binutils-fsf-trunk-quilt/bfd/elfxx-mips.c	2010-11-04 14:58:41.000000000 +0000
@@ -7583,6 +7583,25 @@ _bfd_mips_elf_check_relocs (bfd *abfd, s
 		elf_hash_table (info)->dynobj = dynobj = abfd;
 	      break;
 	    }
+	  /* For sections that are not SEC_ALLOC a copy reloc would be
+	     output if possible (implying questionable semantics for
+	     read-only data objects) or otherwise the final link would
+	     fail as ld.so will not process them and could not therefore
+	     handle any outstanding dynamic relocations.
+
+	     For such sections that are also SEC_DEBUGGING, we can avoid
+	     these problems by simply ignoring any relocs as these
+	     sections have a predefined use and we know it is safe to do
+	     so.
+
+	     This is needed in cases such as a global symbol definition
+	     in a shared library causing a common symbol from an object
+	     file to be converted to an undefined reference.  If that
+	     happens, then all the relocations against this symbol from
+	     SEC_DEBUGGING sections in the object file will resolve to
+	     nil.  */
+	  if ((sec->flags & SEC_DEBUGGING) != 0)
+	    break;
 	  /* Fall through.  */
 
 	default:
Index: binutils-fsf-trunk-quilt/ld/testsuite/ld-elf/comm-data.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/ld/testsuite/ld-elf/comm-data.exp	2010-11-04 14:29:17.000000000 +0000
@@ -0,0 +1,70 @@
+# Expect script for common symbol override.
+#
+#   Copyright 2010  Free Software Foundation, Inc.
+#
+# This file is part of the GNU Binutils.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
+# MA 02110-1301, USA.
+#
+
+#
+# Written by Maciej W. Rozycki <macro@codesourcery.com>
+#
+
+# Exclude non-ELF targets.
+if ![is_elf_format] {
+    return
+}
+
+# Exclude non-Linux targets; feel free to include your favourite one
+# if you like.
+if ![istarget *-*-linux*] {
+    return
+}
+
+set testname "Common symbol override test"
+
+# Define a global symbol.
+run_ld_link_tests [list \
+    [list \
+	"$testname (auxiliary shared object build)" \
+	"-shared" \
+	"" \
+	{ comm-data1.s } \
+	{ \
+	    { readelf -s comm-data1.sd } \
+	} \
+	"libcomm-data.so" \
+    ] \
+]
+
+# Verify that a common symbol has been converted to an undefined
+# reference to the global symbol of the same name defined above
+# and that the debug reference has been dropped.
+run_ld_link_tests [list \
+    [list \
+	"$testname" \
+	"-Ltmpdir -lcomm-data" \
+	"" \
+	{ comm-data2.s } \
+	{ \
+	    { readelf -s comm-data2.sd } \
+	    { readelf -r comm-data2.rd } \
+	    { readelf "-x .debug_foo" comm-data2.xd } \
+	} \
+	"comm-data" \
+    ] \
+]
Index: binutils-fsf-trunk-quilt/ld/testsuite/ld-elf/comm-data1.s
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/ld/testsuite/ld-elf/comm-data1.s	2010-11-04 14:26:44.000000000 +0000
@@ -0,0 +1,6 @@
+	.section .rodata,"a",%progbits
+	.balign	8
+	.globl	foo
+	.type	foo,%object
+foo:
+	.skip	4, 0
Index: binutils-fsf-trunk-quilt/ld/testsuite/ld-elf/comm-data1.sd
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/ld/testsuite/ld-elf/comm-data1.sd	2010-11-04 14:26:44.000000000 +0000
@@ -0,0 +1,10 @@
+Symbol table '\.dynsym' contains [0-9]+ entries:
+ +Num: +Value +Size +Type +Bind +Vis +Ndx +Name
+#...
+ +[0-9]+: +[0-9a-f]+ +0 +OBJECT +GLOBAL +DEFAULT +[0-9]+ +foo
+#...
+Symbol table '\.symtab' contains [0-9]+ entries:
+ +Num: +Value +Size +Type +Bind +Vis +Ndx +Name
+#...
+ +[0-9]+: +[0-9a-f]+ +0 +OBJECT +GLOBAL +DEFAULT +[0-9]+ +foo
+#pass
Index: binutils-fsf-trunk-quilt/ld/testsuite/ld-elf/comm-data2.rd
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/ld/testsuite/ld-elf/comm-data2.rd	2010-11-04 14:26:44.000000000 +0000
@@ -0,0 +1 @@
+There are no relocations in this file\.
Index: binutils-fsf-trunk-quilt/ld/testsuite/ld-elf/comm-data2.s
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/ld/testsuite/ld-elf/comm-data2.s	2010-11-04 14:29:59.000000000 +0000
@@ -0,0 +1,10 @@
+	.text
+	.globl	_start
+	.globl	__start
+_start:
+__start:
+	.comm	foo, 4, 4
+	.section .debug_foo,"",%progbits
+	.balign	16
+	.dc.a	foo
+	.balign	16
Index: binutils-fsf-trunk-quilt/ld/testsuite/ld-elf/comm-data2.sd
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/ld/testsuite/ld-elf/comm-data2.sd	2010-11-04 14:26:44.000000000 +0000
@@ -0,0 +1,10 @@
+Symbol table '\.dynsym' contains [0-9]+ entries:
+ +Num: +Value +Size +Type +Bind +Vis +Ndx +Name
+#...
+ +[0-9]+: +0+ +0 +OBJECT +GLOBAL +DEFAULT +UND +foo
+#...
+Symbol table '\.symtab' contains [0-9]+ entries:
+ +Num: +Value +Size +Type +Bind +Vis +Ndx +Name
+#...
+ +[0-9]+: +0+ +0 +OBJECT +GLOBAL +DEFAULT +UND +foo
+#pass
Index: binutils-fsf-trunk-quilt/ld/testsuite/ld-elf/comm-data2.xd
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/ld/testsuite/ld-elf/comm-data2.xd	2010-11-04 14:26:44.000000000 +0000
@@ -0,0 +1,2 @@
+Hex dump of section '\.debug_foo':
+ +0x0+ +00000000 00000000 00000000 00000000 +\.\.\.\.\.\.\.\.\.\.\.\.\.\.\.\.
Index: binutils-fsf-trunk-quilt/ld/testsuite/ld-mips-elf/comm-data.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils-fsf-trunk-quilt/ld/testsuite/ld-mips-elf/comm-data.exp	2010-11-04 14:30:59.000000000 +0000
@@ -0,0 +1,83 @@
+# Expect script for common symbol override, MIPS variation.
+#
+#   Copyright 2010  Free Software Foundation, Inc.
+#
+# This file is part of the GNU Binutils.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
+# MA 02110-1301, USA.
+#
+
+#
+# Written by Maciej W. Rozycki <macro@codesourcery.com>
+#
+
+# Exclude non-ELF targets.
+if ![is_elf_format] {
+    return
+}
+
+# Exclude non-Linux targets; feel free to include your favourite one
+# if you like.
+if ![istarget mips*-*-linux*] {
+    return
+}
+
+proc mips_comm_data_test { abi flag emul reloc } {
+
+    set testname "MIPS $abi/$reloc common symbol override test"
+    set AFLAGS "$flag -EB"
+    set LDFLAGS "-m$emul"
+
+    # Define a global symbol.
+    run_ld_link_tests [list \
+	[list \
+	    "$testname (auxiliary shared object build)" \
+	    "$LDFLAGS -shared" \
+	    "$AFLAGS -call_shared" \
+	    { ../ld-elf/comm-data1.s } \
+	    { \
+		{ readelf -s ../ld-elf/comm-data1.sd } \
+	    } \
+	  "libmips-$abi-$reloc-comm-data.so" \
+	] \
+    ]
+
+    # Verify that a common symbol has been converted to an undefined
+    # reference to the global symbol of the same name defined above
+    # and that the debug reference has been dropped.
+    run_ld_link_tests [list \
+	[list \
+	    "$testname" \
+	    "$LDFLAGS -z $reloc -Ltmpdir -lmips-$abi-$reloc-comm-data" \
+	    "$AFLAGS -call_nonpic" \
+	    { ../ld-elf/comm-data2.s } \
+	    { \
+		{ readelf -s ../ld-elf/comm-data2.sd } \
+		{ readelf -r ../ld-elf/comm-data2.rd } \
+		{ readelf "-x .debug_foo" ../ld-elf/comm-data2.xd } \
+	    } \
+	    "mips-$abi-$reloc-comm-data" \
+	] \
+    ]
+}
+
+set abis { o32 -32 elf32btsmip n32 -n32 elf32btsmipn32 n64 -64 elf64btsmip }
+set relocs { copyreloc nocopyreloc }
+foreach { abi flag emul } $abis {
+    foreach reloc $relocs {
+	mips_comm_data_test $abi $flag $emul $reloc
+    }
+}
Index: binutils-fsf-trunk-quilt/ld/testsuite/lib/ld-lib.exp
===================================================================
--- binutils-fsf-trunk-quilt.orig/ld/testsuite/lib/ld-lib.exp	2010-11-04 14:26:19.000000000 +0000
+++ binutils-fsf-trunk-quilt/ld/testsuite/lib/ld-lib.exp	2010-11-04 14:26:44.000000000 +0000
@@ -1266,11 +1266,12 @@ proc run_ld_link_tests { ldtests } {
 
 	# Assemble each file in the test.
 	foreach src_file $src_files {
-	    set objfile "tmpdir/[file rootname $src_file].o"
+	    set fileroot "[file rootname [file tail $src_file]]"
+	    set objfile "tmpdir/$fileroot.o"
 	    lappend objfiles $objfile
 
 	    if { [file extension $src_file] == ".c" } {
-		set as_file "tmpdir/[file rootname $src_file].s"
+		set as_file "tmpdir/$fileroot.s"
 		if ![ld_compile "$CC -S $CFLAGS $cflags" $srcdir/$subdir/$src_file $as_file] {
 		    set is_unresolved 1
 		    break
@@ -1476,7 +1477,8 @@ proc run_ld_link_exec_tests { targets_to
 
 	# Assemble each file in the test.
 	foreach src_file $src_files {
-	    set objfile "tmpdir/[file rootname $src_file].o"
+	    set fileroot "[file rootname [file tail $src_file]]"
+	    set objfile "tmpdir/$fileroot.o"
 	    lappend objfiles $objfile
 
 	    # We ignore warnings since some compilers may generate
@@ -1595,7 +1597,8 @@ proc run_cc_link_tests { ldtests } {
 
 	# Compile each file in the test.
 	foreach src_file $src_files {
-	    set objfile "tmpdir/[file rootname $src_file].o"
+	    set fileroot "[file rootname [file tail $src_file]]"
+	    set objfile "tmpdir/$fileroot.o"
 	    lappend objfiles $objfile
 
 	    # We ignore warnings since some compilers may generate