Refix MIPS GOT_PAGE counting

Richard Sandiford rdsandiford@googlemail.com
Sun May 15 19:09:00 GMT 2011


This message:

    http://sourceware.org/ml/binutils/2004-02/msg00195.html

was about a case in which we failed to allocate GOT entries for
GOT_PAGE references.  The thread included exchanges such as:

Richard Sandiford wrote:
> Daniel Jacobowitz wrote:
>> Richard Sandiford wrote:
>>> I've attached a shell archive (to be run from the top level of the
>>> build directory).  Before the patch, a.x needlessly had a global GOT
>>> entry for gs.  After the patch, both a.x and b.x only have a global
>>> GOT entry for "us".
>>
>> Interested in adding this to the testsuite? :)
> Maybe...

and:

Richard Sandiford wrote:
> I don't think it's worth adding the attachment to dejagnu since (a) the
> current sources pass it anyway and (b) we can use the existing elf-rel-got*
> tests to make sure that locally-binding symbols use page entries.  As below.

Well, the custard pie of fate has finished its seven year trajectory
and I now get to fix the test again.  If a GOT_PAGE entry is against
a global symbol, we need to account for both page and global entries,
because we don't know at this stage which is needed.

For the record, the regression was introduced by:

  http://sourceware.org/ml/binutils/2008-03/msg00233.html

which was itself fixing a bug I'd introduced with an earlier patch.
My patch only counted global entries, the fix only counted local entries,
and this one counts both.

I've now done what I should have done originally and added the
testcase to dejagnu.

At the moment, run_ld_link_tests postpones the decision about whether a
link failure is good or bad until it sees whether the first action is an
error-matching test.  It therefore passes any test that has no actions,
regardless of whether the link fails or succeeds.  That doesn't seem
particularly useful, so I've made "no actions" == "test that the
link succeeds".

Tested on mips64-linux-gnu, mipsisa32el-linux-gnu, mips64-elf and
x86_64-linux-gnu.  Also tested with a GCC regression test on
mips64-linux-gnu.  Applied to trunk and branch.

Many thanks to Wang Jiong for finding the bug and for tracking it down
to a regression in the original testcase.

Richard


bfd/
	* elfxx-mips.c (_bfd_mips_elf_check_relocs): Record both local and
	global GOT entries for GOT_PAGE relocations against global symbols.

ld/testsuite/
	* lib/ld-lib.exp (run_ld_link_tests): Simplify pass/fail logic.
	Fail if the link command fails and if no test rules are defined.
	* ld-mips-elf/reloc-6a.s, ld-mips-elf/reloc-6b.s: New tests.
	* ld-mips-elf/mips-elf.exp: Run them.

Index: bfd/elfxx-mips.c
===================================================================
--- bfd/elfxx-mips.c	2011-05-15 13:50:51.000000000 +0100
+++ bfd/elfxx-mips.c	2011-05-15 13:52:26.000000000 +0100
@@ -7743,7 +7743,6 @@ _bfd_mips_elf_check_relocs (bfd *abfd, s
 	      if (!mips_elf_record_got_page_entry (info, abfd, r_symndx,
 						   addend))
 		return FALSE;
-	      break;
 	    }
 	  /* Fall through.  */
 
Index: ld/testsuite/lib/ld-lib.exp
===================================================================
--- ld/testsuite/lib/ld-lib.exp	2011-05-15 14:02:18.000000000 +0100
+++ ld/testsuite/lib/ld-lib.exp	2011-05-15 15:19:03.000000000 +0100
@@ -978,26 +978,21 @@ proc run_ld_link_tests { ldtests } {
 	}
 
 	# Catch assembler errors.
-	if { $is_unresolved != 0 } {
+	if { $is_unresolved } {
 	    unresolved $testname
 	    continue
 	}
 
 	if { [regexp ".*\\.a$" $binfile] } {
 	    if { ![ar_simple_create $ar $ld_options $binfile "$objfiles"] } {
-		fail $testname
 		set failed 1
-	    } else {
-		set failed 0
 	    }
 	} elseif { ![ld_simple_link $ld $binfile "-L$srcdir/$subdir $ld_options $objfiles"] } {
 	    set maybe_failed 1
 	    set ld_output "$exec_output"
-	} else {
-	    set failed 0
 	}
 
-	if { $failed == 0 } {
+	if { !$failed } {
 	    foreach actionlist $actions {
 		set action [lindex $actionlist 0]
 		set progopts [lindex $actionlist 1]
@@ -1034,10 +1029,7 @@ proc run_ld_link_tests { ldtests } {
 			break
 		    }
 		    set maybe_failed 0
-		} elseif { $maybe_failed != 0 } {
-		    set failed 1
-		    break
-		} elseif { $dump_prog != "" } {
+		} elseif { !$maybe_failed && $dump_prog != "" } {
 		    set dumpfile [lindex $actionlist 2]
 		    set binary $dump_prog
 
@@ -1079,18 +1071,14 @@ proc run_ld_link_tests { ldtests } {
 		    remote_file host delete "dump.out"
 		}
 	    }
-
-	    if { $failed != 0 } {
-		fail $testname
-	    } else { if { $is_unresolved == 0 } {
-		pass $testname
-	    } }
 	}
 
-	# Catch action errors.
-	if { $is_unresolved != 0 } {
+	if { $is_unresolved } {
 	    unresolved $testname
-	    continue
+	} elseif { $maybe_failed || $failed } {
+	    fail $testname
+	} else {
+	    pass $testname
 	}
     }
 }
Index: ld/testsuite/ld-mips-elf/reloc-6a.s
===================================================================
--- /dev/null	2011-05-15 08:56:06.273397198 +0100
+++ ld/testsuite/ld-mips-elf/reloc-6a.s	2011-05-15 13:54:43.000000000 +0100
@@ -0,0 +1,11 @@
+	.globl	us
+	.globl	gs
+us:
+gs:
+ls:
+	lw	$4,%got_page(us)($gp)
+	addiu	$4,$4,%got_ofst(us)
+	lw	$4,%got_page(gs)($gp)
+	addiu	$4,$4,%got_ofst(gs)
+	lw	$4,%got_page(ls)($gp)
+	addiu	$4,$4,%got_ofst(ls)
Index: ld/testsuite/ld-mips-elf/reloc-6b.s
===================================================================
--- /dev/null	2011-05-15 08:56:06.273397198 +0100
+++ ld/testsuite/ld-mips-elf/reloc-6b.s	2011-05-15 13:54:59.000000000 +0100
@@ -0,0 +1,11 @@
+	.globl	__start
+	.globl	gs
+__start:
+gs:
+ls:
+	lw	$4,%got_page(us)($gp)
+	addiu	$4,$4,%got_ofst(us)
+	lw	$4,%got_page(gs)($gp)
+	addiu	$4,$4,%got_ofst(gs)
+	lw	$4,%got_page(ls)($gp)
+	addiu	$4,$4,%got_ofst(ls)
Index: ld/testsuite/ld-mips-elf/mips-elf.exp
===================================================================
--- ld/testsuite/ld-mips-elf/mips-elf.exp	2011-05-15 13:51:16.000000000 +0100
+++ ld/testsuite/ld-mips-elf/mips-elf.exp	2011-05-15 14:09:01.000000000 +0100
@@ -298,6 +298,18 @@ if {$has_newabi} {
 }
 run_dump_test "reloc-4"
 run_dump_test "reloc-5"
+if { $has_newabi } {
+    run_ld_link_tests {
+	{"reloc test 6a" "-shared"
+	 "-n32" "reloc-6a.s"
+	 {}
+	 "reloc-6a.so"}
+	{"reloc test 6b" "tmpdir/reloc-6a.so"
+	 "-n32" "reloc-6b.s"
+	 {}
+	 "reloc-6b"}
+    }
+}
 
 if {$has_newabi && $linux_gnu} {
     run_dump_test "eh-frame1-n32"



More information about the Binutils mailing list