Bug 21334 - [MIPS] Undefined hidden symbols cause assertion failure bfd/elfxx-mips.c:3860
Summary: [MIPS] Undefined hidden symbols cause assertion failure bfd/elfxx-mips.c:3860
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: 2.28
: P2 normal
Target Milestone: ---
Assignee: Maciej W. Rozycki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-30 10:59 UTC by James Cowgill
Modified: 2018-07-02 22:59 UTC (History)
3 users (show)

See Also:
Host:
Target: mips*-*-*
Build:
Last reconfirmed: 2017-04-06 00:00:00


Attachments
Test object file (880 bytes, application/x-object)
2017-03-31 07:31 UTC, Alastair Hughes
Details
musl's crt1.o (701 bytes, application/x-object)
2017-03-31 07:32 UTC, Alastair Hughes
Details
WIP bug fix (313 bytes, patch)
2017-04-06 10:27 UTC, Maciej W. Rozycki
Details | Diff
WIP bug fix v2 (896 bytes, patch)
2017-04-07 15:31 UTC, Maciej W. Rozycki
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Cowgill 2017-03-30 10:59:56 UTC
Originally from the Debian supermin package:
https://buildd.debian.org/status/fetch.php?pkg=supermin&arch=mipsel&ver=5.1.17-7%2Bb1&stamp=1490538317&raw=0

And possibly related to a bug mentioned in PR/21233

Attempting to link any object containing an undefined hidden symbol causes ld to give an assertion failure.

$ cat test.c
extern int a __attribute__((visibility("hidden")));

int x(void)
{
    a = 1;
}

$ mipsel-linux-gnu-gcc -c test.c
$ readelf --syms test.o
Symbol table '.symtab' contains 14 entries:
   Num:    Value  Size Type    Bind   Vis      Ndx Name
     0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND
     1: 00000000     0 FILE    LOCAL  DEFAULT  ABS test.c
     2: 00000000     0 SECTION LOCAL  DEFAULT    1
     3: 00000000     0 SECTION LOCAL  DEFAULT    3
     4: 00000000     0 SECTION LOCAL  DEFAULT    4
     5: 00000000     0 SECTION LOCAL  DEFAULT    9
     6: 00000000     0 SECTION LOCAL  DEFAULT    5
     7: 00000000     0 SECTION LOCAL  DEFAULT    6
     8: 00000000     0 SECTION LOCAL  DEFAULT    7
     9: 00000000     0 SECTION LOCAL  DEFAULT   10
    10: 00000000     0 SECTION LOCAL  DEFAULT   11
    11: 00000000    60 FUNC    GLOBAL DEFAULT    1 x
    12: 00000000     0 OBJECT  GLOBAL DEFAULT  UND _gp_disp
    13: 00000000     0 NOTYPE  GLOBAL HIDDEN   UND a
$ ../build-mips/ld/ld-new test.o
../build-mips/ld/ld-new: warning: cannot find entry symbol __start; defaulting to 00000000004000f0
../build-mips/ld/ld-new: BFD (GNU Binutils) 2.28.51.20170330 assertion fail ../../binutils-gdb/bfd/elfxx-mips.c:3860
test.o: In function `x':
test.c:(.text+0x18): undefined reference to `a'

The bug in supermin was actually caused by musl's use of this feature in __libc_start_main, so I suspect that nothing will be able to link against musl libc when using binutils 2.28.
Comment 1 Alastair Hughes 2017-03-31 07:26:16 UTC
I believe I have encountered the same issue while attempting to build a MIPS cross compiler, statically linking against musl libc (2.28.51.20170331 and 2.28). I will attach the objects used during the link.

Here is the collect2 invocation:

$ /pkg/src/gcc-build/gcc/collect2 -plugin /pkg/src/gcc-build/gcc/liblto_plugin.so -plugin-opt=/pkg/src/gcc-build/gcc/lto-wrapper -plugin-opt=-fresolution=/tmp/ccRJho5Z.res -plugin-opt=-pass-through=-lgcc -plugin-opt=-pass-through=-lc -plugin-opt=-pass-through=-lgcc --sysroot=/sysroot --eh-frame-hdr -EB -dynamic-linker /lib/ld-musl-mips.so.1 -melf32btsmip /sysroot/usr/lib/crt1.o /sysroot/usr/lib/crti.o /pkg/src/gcc-build/gcc/crtbegin.o -L/pkg/src/gcc-build/gcc -L/opt/mips-linux-musl/mips-linux-musl/bin -L/opt/mips-linux-musl/mips-linux-musl/lib -L/sysroot/usr/lib /tmp/cc64Bubr.o -lgcc -lc -lgcc /pkg/src/gcc-build/gcc/crtend.o /sysroot/usr/lib/crtn.o

Linking succeeds without the crt1.o object, albeit complaining about being unable to find __start - I'm not sure whether this is significant or not, but presumably you will be able to tell :)

I played around with the file that the configure script was attempting to build:

$ cat config.c
int
main ()
{

  ;
  return 0;
}
$ /pkg/src/gcc-build/gcc/xgcc -B/pkg/src/gcc-build/gcc -B/opt/mips-linux-musl/mips-linux-musl/bin/ -B/opt/mips-linux-musl/mips-linux-musl/lib/ -isystem /opt/mips-linux-musl/mips-linux-musl/include -isystem /opt/mips-linux-musl/mips-linux-musl-sys-include -g -O2 -minterlink-mips16 config.c -v
$ /opt/mips-linux-musl/mips-linux-musl/bin/ld -melf32btsmip /sysroot/usr/lib/crti.o /pkg/src/gcc-build/gcc/crtbegin.o -L/pkg/src/gcc-build/gcc -L/opt/mips-linux-musl/mips-linux-musl/bin -L/opt/mips-linux-musl/mips-linux-musl/lib -L/sysroot/usr/lib config.o -lgcc -lc -lgcc /pkg/src/gcc-build/gcc/crtend.o /sysroot/usr/lib/crtn.o
/opt/mips-linux-musl/mips-linux-musl/bin/ld: BFD (GNU Binutils) 2.28.51.20170331 assertion fail /pkg/src/binutils-gdb/bfd/elfxx-mips.c:3860
$ readelf --syms config.o
Symbol table '.symtab' contains 20 entries:
   Num:    Value  Size Type    Bind   Vis      Ndx Name
     0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND 
     1: 00000000     0 FILE    LOCAL  DEFAULT  ABS config.c
     2: 00000000     0 SECTION LOCAL  DEFAULT    1 
     3: 00000000     0 SECTION LOCAL  DEFAULT    2 
     4: 00000000     0 SECTION LOCAL  DEFAULT    3 
     5: 00000000     0 SECTION LOCAL  DEFAULT    8 
     6: 00000000     0 SECTION LOCAL  DEFAULT    9 
     7: 00000000     0 SECTION LOCAL  DEFAULT   10 
     8: 00000000     0 SECTION LOCAL  DEFAULT   12 
     9: 00000000     0 SECTION LOCAL  DEFAULT   13 
    10: 00000000     0 SECTION LOCAL  DEFAULT   15 
    11: 00000000     0 SECTION LOCAL  DEFAULT   17 
    12: 00000000     0 SECTION LOCAL  DEFAULT   19 
    13: 00000000     0 SECTION LOCAL  DEFAULT   21 
    14: 00000000     0 SECTION LOCAL  DEFAULT    4 
    15: 00000000     0 SECTION LOCAL  DEFAULT    5 
    16: 00000000     0 SECTION LOCAL  DEFAULT    6 
    17: 00000000     0 SECTION LOCAL  DEFAULT   20 
    18: 00000000     0 SECTION LOCAL  DEFAULT   23 
    19: 00000000     8 FUNC    GLOBAL DEFAULT    9 main
Comment 2 Alastair Hughes 2017-03-31 07:31:20 UTC
Created attachment 9956 [details]
Test object file

Test object file generated from config.c; links without the assertion failure unless musl's crt1.o is included.
Comment 3 Alastair Hughes 2017-03-31 07:32:25 UTC
Created attachment 9957 [details]
musl's crt1.o

musl's crt1.o file which causes the assertion failure
Comment 4 Maciej W. Rozycki 2017-04-06 10:27:37 UTC
Created attachment 9975 [details]
WIP bug fix

The problem here is the MIPS backend sorts the dynamic symbol table
even if no dynamic sections have been created.  The latter means
`local_dynsymcount' remains unchanged from its initial 0 value even
though `a' is local and has been speculatively assigned a `dynindx'
upon LD encountering a GOT relocation (NB you need to compile test.c
with `-fPIE' or suchlike for the issue to trigger).

Normally `_bfd_elf_link_renumber_dynsyms', first called from
`bfd_elf_size_dynsym_hash_dynstr', would have set `local_dynsymcount'. 
This situation then results in the assertion failure, as `a' despite
being local has been assigned a `dynindx' beyond `local_dynsymcount'.

James and Alastair, can you please try the patch attached then and see
if it fixes the issue for you?  I'll proceed with the usual integration
dance if so.

NB we have what looks like a bug to me in that another call to
`_bfd_elf_link_renumber_dynsyms', arranged in
`bfd_elf_size_dynamic_sections' after section GC has completed,
is made even if no dynamic sections have been created.  Consequently
you can work this PR around by using `--gc-sections' with LD (and with
James's test case also say `-e x' so that the reference to `a' itself
is not GCed).  I'll be sending a fix proposal for this bug separately.

Finally I think we have another issue in the MIPS backend, with handling
hidden and internal weak undefined symbols.  References to such symbols
are supposed to resolve to 0, however if a GOT relocation is used, then
a local GOT entry is created and used to satisfy the reference.  Such an
entry is then subject to the usual constant load-time relocation, which
means a non-zero value will be returned if the base address is non-zero.
This will defeat the usual run-time sequence like:

void a (void) __attribute__ ((visibility ("hidden"), weak));

void
x (void)
{
  if (a)
    a ();
}

What instead we should do I believe is create an *external* GOT entry
for the weak undefined symbol entered in the dynamic symbol table as
local absolute and having the value of 0.  I'll handle this separately
as well.
Comment 5 James Cowgill 2017-04-06 12:59:22 UTC
Maciej, your fix does fix my testcase, but doesn't seem to fix supermin. I now get this error repeatedly when I try to build supermin or build anything statically:

./ld: BFD (GNU Binutils) 2.28.51.20170406 assertion fail ../../binutils-gdb/bfd/elfxx-mips.c:3551
Comment 6 Maciej W. Rozycki 2017-04-07 15:31:14 UTC
Created attachment 9977 [details]
WIP bug fix v2

I have looked into it and it looks to me like rewriting MIPS backend code
to make it avoid relying on dynsym sorting in the case of the presence
of GOT relocations in a static binary (i.e. linking in PIC code such as
pieces of libgcc.a into a static binary) requiring the backend to set up
what in a dynamic binary would become the local part of the GOT would be
too costly to be justified for such a corner case.

So let's make `_bfd_elf_link_renumber_dynsyms' called in all cases, which
I hope is going to be safe for all backends.

In fact, based on the observation previously made that this function is
already inconsistently called in the `--gc-sections' case even in the
static case, making the call unconditionally should have been largely
validated by Debian packaging already, which has been using
`--gc-sections' distribution-wide across all its targets for a while now.
James, can you confirm this has been the case?

James and Alastair, can you please see if this updated version of the
the patch attached fixes the issue for you?
Comment 7 James Cowgill 2017-04-10 11:33:21 UTC
(In reply to Maciej W. Rozycki from comment #6)
> In fact, based on the observation previously made that this function is
> already inconsistently called in the `--gc-sections' case even in the
> static case, making the call unconditionally should have been largely
> validated by Debian packaging already, which has been using
> `--gc-sections' distribution-wide across all its targets for a while now.
> James, can you confirm this has been the case?

`--gc-sections` isn't enabled distribution-wide. The previous bugs were from packages which enabled it manually and from the KDE / Qt packages which seem to all have it enabled as well. For the packages where it is enabled, it is of course tested on all of Debian's targets.

> James and Alastair, can you please see if this updated version of the
> the patch attached fixes the issue for you?

Your second patch does solve the issue in supermin for me.

Thanks!
James
Comment 8 Maciej W. Rozycki 2017-04-11 23:41:50 UTC
Incorrect dynamic relocation arrangement for undefined hidden and
internal weak symbols now filed as PR ld/21375.
Comment 9 Alastair Hughes 2017-04-26 07:02:39 UTC
I can confirm that the second patch also fixes the issue that I had come across.
Comment 10 Sourceware Commits 2017-04-26 12:34:47 UTC
The master branch has been updated by Maciej W. Rozycki <macro@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=23ec1e32b1ab714649a7c25e49b5d721fe3bd3db

commit 23ec1e32b1ab714649a7c25e49b5d721fe3bd3db
Author: Maciej W. Rozycki <macro@imgtec.com>
Date:   Mon Apr 24 20:15:31 2017 +0100

    PR ld/21334: Always call `_bfd_elf_link_renumber_dynsyms' if required
    
    Complement commit e17b0c351f0b ("MIPS/BFD: Respect the ELF gABI dynamic
    symbol table sort requirement") and correct an inconsistency in dynamic
    symbol accounting data causing an assertion failure in the MIPS backend:
    
    ld: BFD (GNU Binutils) 2.28.51.20170330 assertion fail
    ../../binutils-gdb/bfd/elfxx-mips.c:3860
    
    in the course of making a GOT entry in a static binary to satisfy a GOT
    relocation present in input, due to the local dynamic symbol count not
    having been established.
    
    To do so let backends request `_bfd_elf_link_renumber_dynsyms' to be
    always called, rather than where a dynamic binary is linked only, and
    then make this request in the MIPS backend.
    
    	bfd/
    	PR ld/21334
    	* elf-bfd.h (elf_backend_data): Add `always_renumber_dynsyms'
    	member.
    	* elfxx-target.h [!elf_backend_always_renumber_dynsyms]
    	(elf_backend_always_renumber_dynsyms): Define.
    	(elfNN_bed): Initialize `always_renumber_dynsyms' member.
    	* elfxx-mips.h (elf_backend_always_renumber_dynsyms): Define.
    	* elflink.c (bfd_elf_size_dynamic_sections): Also call
    	`_bfd_elf_link_renumber_dynsyms' if the backend has requested
    	it.
    	(bfd_elf_size_dynsym_hash_dynstr): Likewise.
    
    	ld/
    	PR ld/21334
    	* testsuite/ld-mips-elf/pr21334.dd: New test.
    	* testsuite/ld-mips-elf/pr21334.gd: New test.
    	* testsuite/ld-mips-elf/pr21334.ld: New test linker script.
    	* testsuite/ld-mips-elf/pr21334.s: New test source.
    	* testsuite/ld-mips-elf/mips-elf.exp: Run the new tests.
Comment 11 Sourceware Commits 2017-04-26 21:37:46 UTC
The binutils-2_28-branch branch has been updated by Maciej W. Rozycki <macro@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=1be438fd16d8a42e2a09d7b932da27c0e36094b6

commit 1be438fd16d8a42e2a09d7b932da27c0e36094b6
Author: Maciej W. Rozycki <macro@imgtec.com>
Date:   Wed Apr 26 22:23:44 2017 +0100

    PR ld/21334: Always call `_bfd_elf_link_renumber_dynsyms' if required
    
    Complement commit e17b0c351f0b ("MIPS/BFD: Respect the ELF gABI dynamic
    symbol table sort requirement") and correct an inconsistency in dynamic
    symbol accounting data causing an assertion failure in the MIPS backend:
    
    ld: BFD (GNU Binutils) 2.28.51.20170330 assertion fail
    ../../binutils-gdb/bfd/elfxx-mips.c:3860
    
    in the course of making a GOT entry in a static binary to satisfy a GOT
    relocation present in input, due to the local dynamic symbol count not
    having been established.
    
    To do so let backends request `_bfd_elf_link_renumber_dynsyms' to be
    always called, rather than where a dynamic binary is linked only, and
    then make this request in the MIPS backend.
    
    	bfd/
    	PR ld/21334
    	* elf-bfd.h (elf_backend_data): Add `always_renumber_dynsyms'
    	member.
    	* elfxx-target.h [!elf_backend_always_renumber_dynsyms]
    	(elf_backend_always_renumber_dynsyms): Define.
    	(elfNN_bed): Initialize `always_renumber_dynsyms' member.
    	* elfxx-mips.h (elf_backend_always_renumber_dynsyms): Define.
    	* elflink.c (bfd_elf_size_dynsym_hash_dynstr): Also call
    	`_bfd_elf_link_renumber_dynsyms' if the backend has requested
    	it.
    	(elf_gc_sweep): Likewise.
    
    (backported from commit 23ec1e32b1ab714649a7c25e49b5d721fe3bd3db)
Comment 12 Maciej W. Rozycki 2017-04-26 21:47:39 UTC
Alastair, thanks for testing, however in the course of review we have
settled on a slightly different change, which as far as the MIPS target
is concerned is functionally equivalent to one you have validated.
That change has now been committed to master and backported to 2.28, as
per the automatic commit messages, closing bug.
Comment 13 Gianfranco 2017-04-27 06:31:36 UTC
Hello, can we have the testcase added to the testsuite to avoid regressions?

thanks!
Comment 14 Maciej W. Rozycki 2017-04-27 10:34:14 UTC
It's there on master: ld/testsuite/ld-mips-elf/pr21334.*; not backported
to 2.28 to avoid new features (`readelf -A' static GOT addition) on a
release branch.
Comment 15 Gianfranco 2017-04-27 12:53:49 UTC
makes sense, thanks!
Comment 16 Sourceware Commits 2018-07-02 22:59:39 UTC
The master branch has been updated by Maciej W. Rozycki <macro@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=30e5322741b7cb8fe90bec43ab93eecceaaec0e7

commit 30e5322741b7cb8fe90bec43ab93eecceaaec0e7
Author: Maciej W. Rozycki <macro@mips.com>
Date:   Mon Jul 2 23:57:21 2018 +0100

    MIPS/LD/testsuite: Fix a typo in PR ld/21334 test name
    
    	ld/
    	* testsuite/ld-mips-elf/mips-elf.exp: Fix a typo in PR ld/21334
    	test name.