[PATCH] MIPS: Do not return after calling undefined_symbol hook

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] MIPS: Do not return after calling undefined_symbol hook

James Cowgill-3
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.

Fix this by continuing after calling the undefined_symbol hook.

I need someone to commit this for me.

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.
---
 bfd/elfxx-mips.c                          |  2 +-
 ld/testsuite/ld-mips-elf/mips-elf.exp     |  1 +
 ld/testsuite/ld-mips-elf/undefined-warn.d | 15 +++++++++++++++
 3 files changed, 17 insertions(+), 1 deletion(-)
 create mode 100644 ld/testsuite/ld-mips-elf/undefined-warn.d

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;
  }
 
       target_is_16_bit_code_p = ELF_ST_IS_MIPS16 (h->root.other);
diff --git a/ld/testsuite/ld-mips-elf/mips-elf.exp b/ld/testsuite/ld-mips-elf/mips-elf.exp
index 13dbbc64f9..e6cd41ed79 100644
--- a/ld/testsuite/ld-mips-elf/mips-elf.exp
+++ b/ld/testsuite/ld-mips-elf/mips-elf.exp
@@ -994,6 +994,7 @@ if { $linux_gnu } {
 }
 
 run_dump_test "undefined"
+run_dump_test "undefined-warn"
 
 # Test the conversion from jr to b
 if { $linux_gnu } {
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
+#warning: \A[^\n]*\.o: In function `foo':\n\(\.text\+0x0\): warning: undefined reference to `bar'\Z
+#objdump: -d
+
+.*:     file format .*
+
+Disassembly of section \.text:
+
+.* <foo>:
+# Loaded value must not be 0
+.*: 2402.... li v0,[-1-9][0-9]*
+ \.\.\.
--
2.16.1

Reply | Threaded
Open this post in threaded view
|

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

Maciej W. Rozycki-2
From: James Cowgill <[hidden email]>

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