Re: PATCH: PR ld/14170: ld: assertion fail bfd/linker.c:641

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Sun, May 27, 2012 at 4:06 AM, Alan Modra <amodra@gmail.com> wrote:
> On Sat, May 26, 2012 at 07:59:37AM -0700, H.J. Lu wrote:
>> @@ -1217,7 +1217,7 @@ _bfd_elf_merge_symbol (bfd *abfd,
>> ? ? ? }
>>
>> ? ? ? ?if ((h->root.u.undef.next || info->hash->undefs_tail == &h->root)
>> - ? ? ? && bfd_is_und_section (sec))
>> + ? ? ? && !newdef)
>
> This is the same logic as the patch I added to your bugzilla, stating
> that it wasn't a complete fix.
>
> As best I can see, this is the correct fix. ?Please review.
>
> ? ? ? ?PR ld/14170
> ? ? ? ?* elflink.c (_bfd_elf_merge_symbol): When a symbol defined in
> ? ? ? ?a dynamic library finds a new instance with non-default
> ? ? ? ?visibility in a regular object, correctly handle symbols
> ? ? ? ?already on the undefs list and undo dynamic symbol state when
> ? ? ? ?the new symbol is hidden or internal.
>
> Index: bfd/elflink.c
> ===================================================================
> RCS file: /cvs/src/src/bfd/elflink.c,v
> retrieving revision 1.445
> diff -u -p -r1.445 elflink.c
> --- bfd/elflink.c ? ? ? 25 May 2012 01:12:19 -0000 ? ? ?1.445
> +++ bfd/elflink.c ? ? ? 27 May 2012 10:42:33 -0000
> @@ -1216,15 +1216,15 @@ _bfd_elf_merge_symbol (bfd *abfd,
> ? ? ? ? ? ?h = *sym_hash;
> ? ? ? ?}
>
> - ? ? ?if ((h->root.u.undef.next || info->hash->undefs_tail == &h->root)
> - ? ? ? ? && bfd_is_und_section (sec))
> + ? ? ?/* If the old symbol was undefined before, then it will still be
> + ? ? ? ?on the undefs list. ?If the new symbol is undefined or
> + ? ? ? ?common, we can't make it bfd_link_hash_new here, because new
> + ? ? ? ?undefined or common symbols will be added to the undefs list
> + ? ? ? ?by _bfd_generic_link_add_one_symbol. ?Symbols may not be
> + ? ? ? ?added twice to the undefs list. ?Also, if the new symbol is
> + ? ? ? ?undefweak then we don't want to lose the strong undef. ?*/
> + ? ? ?if (h->root.u.undef.next || info->hash->undefs_tail == &h->root)
> ? ? ? ?{
> - ? ? ? ? /* If the new symbol is undefined and the old symbol was
> - ? ? ? ? ? ?also undefined before, we need to make sure
> - ? ? ? ? ? ?_bfd_generic_link_add_one_symbol doesn't mess
> - ? ? ? ? ? ?up the linker hash table undefs list. ?Since the old
> - ? ? ? ? ? ?definition came from a dynamic object, it is still on the
> - ? ? ? ? ? ?undefs list. ?*/
> ? ? ? ? ?h->root.type = bfd_link_hash_undefined;
> ? ? ? ? ?h->root.u.undef.abfd = abfd;
> ? ? ? ?}
> @@ -1234,11 +1234,21 @@ _bfd_elf_merge_symbol (bfd *abfd,
> ? ? ? ? ?h->root.u.undef.abfd = NULL;
> ? ? ? ?}
>
> - ? ? ?if (h->def_dynamic)
> + ? ? ?if (ELF_ST_VISIBILITY (sym->st_other) != STV_PROTECTED)
> + ? ? ? {
> + ? ? ? ? /* If the new symbol is hidden or internal, completely undo
> + ? ? ? ? ? ?any dynamic link state. ?*/
> + ? ? ? ? (*bed->elf_backend_hide_symbol) (info, h, TRUE);
> + ? ? ? ? h->forced_local = 0;
> + ? ? ? ? h->def_dynamic = 0;
> + ? ? ? ? h->ref_dynamic = 0;
> + ? ? ? }
> + ? ? ?else if (h->def_dynamic)

It looks OK, except for " if (h->def_dynamic)".  h->def_dynamic is
always true when we get it.

> ? ? ? ?{
> ? ? ? ? ?h->def_dynamic = 0;
> ? ? ? ? ?h->ref_dynamic = 1;
> ? ? ? ?}
> + ? ? ?h->dynamic_def = 0;
> ? ? ? /* FIXME: Should we check type and size for protected symbol? ?*/
> ? ? ? h->size = 0;
> ? ? ? h->type = 0;
>

Thanks.


-- 
H.J.