This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] PR ld/19636: pie changes program behavior and generate unnecessary dynamic symbols
- From: "H.J. Lu" <hjl dot tools at gmail dot com>
- To: Alan Modra <amodra at gmail dot com>
- Cc: Binutils <binutils at sourceware dot org>
- Date: Wed, 17 Feb 2016 20:24:06 -0800
- Subject: Re: [PATCH] PR ld/19636: pie changes program behavior and generate unnecessary dynamic symbols
- Authentication-results: sourceware.org; auth=none
- References: <20160215150032 dot GA26273 at gmail dot com> <20160217052322 dot GA14941 at gmail dot com> <CAMe9rOoOKPsrxf51pzBzouREZYP0TUyFNq2XTZGY61p3kzrEgA at mail dot gmail dot com> <20160218004245 dot GG31757 at bubble dot grove dot modra dot org>
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.