Re: [PATCH] PR ld/19636: pie changes program behavior and generate unnecessary dynamic symbols

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

On Wed, Feb 17, 2016 at 4:42 PM, Alan Modra <amodra@gmail.com> wrote:
> On Wed, Feb 17, 2016 at 03:33:51PM -0800, H.J. Lu wrote:
>> On Tue, Feb 16, 2016 at 9:23 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> > On Mon, Feb 15, 2016 at 07:00:32AM -0800, H.J. Lu wrote:
>> >> I fixed x86.  Other backends need similar fix.  Any comments?
>
> If you aren't going to fix other backends too, have you at least run
> the testsuite for all the other ELF targets that use SYMBOLIC_BIND?

Should I enable them for all ELF target with PIE/PIC support?
All of them fail since none of them handle PIE correctly.

>> -/* Will a symbol be bound to the definition within the shared
>> -   library, if any.  A unique symbol can never be bound locally.  */
>> -#define SYMBOLIC_BIND(INFO, H) \
>> -    (!(H)->unique_global \
>> -     && ((INFO)->symbolic || ((INFO)->dynamic && !(H)->dynamic)))
>> +/* Will a symbol be bound to the definition within the PIC object, if
>> +   any.  A unique symbol can never be bound locally.  Symbols are always
>> +   bound locally in PIE, similar to -shared -Bsymbolic.  */
>> +#define SYMBOLIC_BIND(INFO, H)                                               \
>> +    (!(H)->unique_global                                             \
>> +     && ((INFO)->symbolic                                            \
>> +      || ((INFO)->dynamic && !(H)->dynamic)                          \
>> +      || bfd_link_pie (INFO)))
>
> This probably should be bfd_link_executable rather than bfd_link_pie.

SYMBOLIC_BIND was intended for shared library and is used only when
PIC is true.  It is never applied to bfd_link_hash_undefweak.  That is why
I added bfd_link_pie.  Change it to bfd_link_executable is a good start if
we want to extend it to executable where bfd_link_hash_undefweak is a
special case.  bfd_link_hash_undefweak always binds locally in executable
and never binds locally in shared library.   Should we extend SYMBOLIC_BIND
to cover all cases so that we don't have check PIC nor undefweak before
using it?


>> diff --git a/bfd/elflink.c b/bfd/elflink.c
>> index ac03ce5..10dece3 100644
>> --- a/bfd/elflink.c
>> +++ b/bfd/elflink.c
>> @@ -632,7 +632,7 @@ bfd_elf_record_link_assignment (bfd *output_bfd,
>>
>>    if ((h->def_dynamic
>>         || h->ref_dynamic
>> -       || bfd_link_pic (info)
>> +       || bfd_link_dll (info)
>>         || (bfd_link_pde (info)
>>          && elf_hash_table (info)->is_relocatable_executable))
>
> Should the bfd_link_pde test be removed here?

I think so.

>> @@ -2679,6 +2679,16 @@ _bfd_elf_adjust_dynamic_symbol (struct elf_link_hash_entry *h, void *data)
>>             && (h->u.weakdef == NULL || h->u.weakdef->dynindx == -1))))
>>      {
>>        h->plt = elf_hash_table (eif->info)->init_plt_offset;
>> +      /* Remove undefined weak symbol from the dynamic symbol table
>> +      in executable.  */
>> +      if (h->root.type == bfd_link_hash_undefweak
>> +       && bfd_link_executable (eif->info)
>> +       && h->dynindx != -1)
>> +     {
>> +       h->dynindx = -1;
>> +       _bfd_elf_strtab_delref (elf_hash_table (eif->info)->dynstr,
>> +                               h->dynstr_index);
>> +     }
>>        return TRUE;
>>      }
>>
>
> This change is incorrect.  Some targets (even x86_64 with -fPIC)
> support
>   if (fun)
>     fun ();
> for an undefined weak fun at link time, making fun dynamic and
> emitting dynamic relocs against fun.  So when the executable is linked
> against a new shared library that provides fun, fun will be called.

Yes, it should be moved before

 /* Remove undefined weak symbol from the dynamic symbol table
     in executable.  */
  if (h->root.type == bfd_link_hash_undefweak
      && bfd_link_executable (eif->info)
      && h->dynindx != -1)
    {
      h->dynindx = -1;
      _bfd_elf_strtab_delref (elf_hash_table (eif->info)->dynstr,
                              h->dynstr_index);
    }


> Other than the above comments, the patch looks good.
>

It exposed more issues in x86 backends :-).


-- 
H.J.