[committed] PR ld/21900: MIPS: Fix relocation processing with undefined symbols

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

From: James Cowgill <james.cowgill@mips.com>

Currently, when `mips_elf_calculate_relocation' is asked to relocate an 
undefined symbol, it reports an error or a warning and immediately 
returns without performing the relocation.  This is fine if the link 
fails, but if unresolved_syms_in_objects == RM_GENERATE_WARNING, the 
link will continue and output some unrelocated code, which is a 
regression from commit e7e2196da3f0 ("MIPS/BFD: Correctly report 
undefined relocations").

Fix this by continuing after calling the `undefined_symbol' hook unless 
this is an error condition.

	bfd/
	PR ld/21900
	* elfxx-mips.c (mips_elf_calculate_relocation): Only return 
	after calling `undefined_symbol' hook if this is an error 
	condition.  Assume the value of 0 for the symbol requested 
	otherwise.

	ld/
	PR ld/21900
	* testsuite/ld-mips-elf/undefined-warn.d: New test.
	* testsuite/ld-mips-elf/undefined.s: Add padding at the end.
	* testsuite/ld-mips-elf/mips-elf.exp: Run the new test.
---
Hi James,

> Currently, when mips_elf_calculate_relocation is asked to relocate an
> undefined symbol, it reports the error and immediately returns without
> performing the relocation. This is fine if the link fails, but if
> unresolved_syms_in_objects == RM_GENERATE_WARNING, the link will
> continue and output some unrelocated code.

 Two spaces are required between a full stop and the following sentence 
according to the GNU Coding Standard.

> Fix this by continuing after calling the undefined_symbol hook.
> 
> I need someone to commit this for me.

 In a GIT-formatted submission please separate the commit description and 
any additional comments with a `---' marker on a separate line.  Try `git 
am' on the outgoing message or a copy sent to yourself and the tip of the 
master branch to see if it comes out as required.

> bfd/
> 	PR 21900
> 	* elfxx-mips.c (mips_elf_calculate_relocation): Do not return
> 	after calling undefined_symbol hook.
> 
> ld/
> 	PR 21900
> 	* testsuite/ld-mips-elf/mips-elf.exp: Run new test.
> 	* testsuite/ld-mips-elf/undefined-warn.d: New test.

 We usually list the component along with the PR number, i.e. PR ld/21900 
in this case.

> diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
> index 246d51c9cc..76e079170e 100644
> --- a/bfd/elfxx-mips.c
> +++ b/bfd/elfxx-mips.c
> @@ -5483,7 +5483,7 @@ mips_elf_calculate_relocation (bfd *abfd, bfd *input_bfd,
>  	     input_section, relocation->r_offset,
>  	     (info->unresolved_syms_in_objects == RM_GENERATE_ERROR)
>  	     || ELF_ST_VISIBILITY (h->root.other));
> -	  return bfd_reloc_undefined;
> +	  symbol = 0;
>  	}

 The problem here is we genuinely want the error to be returned under the 
conditions also used to set the last argument to `->undefined_symbol'.

> diff --git a/ld/testsuite/ld-mips-elf/undefined-warn.d b/ld/testsuite/ld-mips-elf/undefined-warn.d
> new file mode 100644
> index 0000000000..2c61aa8706
> --- /dev/null
> +++ b/ld/testsuite/ld-mips-elf/undefined-warn.d
> @@ -0,0 +1,15 @@
> +#name: MIPS undefined reference with --warn-unresolved-symbols
> +#source: undefined.s
> +#as: -EB -32
> +#ld: -EB -e foo --warn-unresolved-symbols

 No need to force endianness or ABI with this test, and avoiding it allows 
for wider test coverage.

> +#warning: \A[^\n]*\.o: In function `foo':\n\(\.text\+0x0\): warning: undefined reference to `bar'\Z
> +#objdump: -d

 Using `--prefix-addresses --show-raw-insn' often produces less clutter 
in `objdump -d' output, e.g. we do not really care about `foo' here, and 
with some targets a different overlapping symbol may be produced and 
then appear in a dump instead.

> +
> +.*:     file format .*
> +
> +Disassembly of section \.text:
> +
> +.* <foo>:
> +# Loaded value must not be 0

 A full stop is required at the end of a sentence according to the GNU 
Coding Standard.

> +.*:	2402.... 	li	v0,[-1-9][0-9]*
> +	\.\.\.

 Section padding is not guaranteed to be the same across MIPS targets.  
This caused failures across bare-metal ELF targets, which I have fixed up 
by adjusting the source accordingly.  Please always verify changes and 
especially test suite additions against at least one OS (such as Linux) 
target and one bare-metal ELF target (and also state along with the 
submission how it was tested).

 Also in future submissions please quote any PR number in the change 
heading.

 As I am off the next two weeks I decided not to hold you with this issue, 
so I went ahead and made all the changes myself, committed the change now, 
and also closed the PR.  Please make sure though that you have your MIPS 
copyright assignment sorted out with the Free Software Foundation for your 
future submissions.

 Please let me know if you have any questions or comments.  I'll reply 
when I am back.

  Maciej
---
 bfd/elfxx-mips.c                          |   14 ++++++++++----
 ld/testsuite/ld-mips-elf/mips-elf.exp     |    1 +
 ld/testsuite/ld-mips-elf/undefined-warn.d |   13 +++++++++++++
 ld/testsuite/ld-mips-elf/undefined.s      |    4 ++++
 4 files changed, 28 insertions(+), 4 deletions(-)
 create mode 100644 ld/testsuite/ld-mips-elf/undefined-warn.d

binutils-jamesc-mips-warn-unresolved-symbols.diff
Index: binutils/bfd/elfxx-mips.c
===================================================================
--- binutils.orig/bfd/elfxx-mips.c	2018-03-02 20:32:34.717366552 +0000
+++ binutils/bfd/elfxx-mips.c	2018-03-02 21:15:59.386305290 +0000
@@ -5478,12 +5478,18 @@ mips_elf_calculate_relocation (bfd *abfd
 	}
       else
 	{
+	  bfd_boolean reject_undefined
+	    = (info->unresolved_syms_in_objects == RM_GENERATE_ERROR
+	       || ELF_ST_VISIBILITY (h->root.other) != STV_DEFAULT);
+
 	  (*info->callbacks->undefined_symbol)
 	    (info, h->root.root.root.string, input_bfd,
-	     input_section, relocation->r_offset,
-	     (info->unresolved_syms_in_objects == RM_GENERATE_ERROR)
-	     || ELF_ST_VISIBILITY (h->root.other));
-	  return bfd_reloc_undefined;
+	     input_section, relocation->r_offset, reject_undefined);
+
+	  if (reject_undefined)
+	    return bfd_reloc_undefined;
+
+	  symbol = 0;
 	}
 
       target_is_16_bit_code_p = ELF_ST_IS_MIPS16 (h->root.other);
Index: binutils/ld/testsuite/ld-mips-elf/mips-elf.exp
===================================================================
--- binutils.orig/ld/testsuite/ld-mips-elf/mips-elf.exp	2018-03-02 20:32:34.737560336 +0000
+++ binutils/ld/testsuite/ld-mips-elf/mips-elf.exp	2018-03-02 21:04:43.654614168 +0000
@@ -995,6 +995,7 @@ if { $linux_gnu } {
 }
 
 run_dump_test "undefined"
+run_dump_test "undefined-warn"
 
 # Test the conversion from jr to b
 if { $linux_gnu } {
Index: binutils/ld/testsuite/ld-mips-elf/undefined-warn.d
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-mips-elf/undefined-warn.d	2018-03-02 21:21:20.007842391 +0000
@@ -0,0 +1,13 @@
+#objdump: -d --prefix-addresses --show-raw-insn
+#name: MIPS undefined reference with --warn-unresolved-symbols
+#source: undefined.s
+#ld: -e foo --warn-unresolved-symbols
+#warning: \A[^\n]*\.o: in function `foo':\n\(\.text\+0x0\): warning: undefined reference to `bar'\Z
+
+.*:     file format .*
+
+Disassembly of section \.text:
+
+# Loaded value must not be 0.
+[0-9a-f]+ <[^>]*> 2402.... 	li	v0,[-1-9][0-9]*
+	\.\.\.
Index: binutils/ld/testsuite/ld-mips-elf/undefined.s
===================================================================
--- binutils.orig/ld/testsuite/ld-mips-elf/undefined.s	2017-12-12 00:50:21.000000000 +0000
+++ binutils/ld/testsuite/ld-mips-elf/undefined.s	2018-03-02 21:34:43.929826991 +0000
@@ -22,3 +22,7 @@
 	li	$2, %got_page(bar)
 	.end	foo
 	.size	foo, . - foo
+
+# Force some (non-delay-slot) zero bytes, to make 'objdump' print ...
+	.align	4, 0
+	.space	16