Re: Define __start/__stop symbols when there is only a dynamic def

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

On Tue, Jan 30, 2018 at 6:03 AM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Tue, 30 Jan 2018, Alan Modra wrote:
>
>> On Mon, Jan 29, 2018 at 03:36:05PM -0800, H.J. Lu wrote:
>> > There is a reason why I have been keeping asking for a testcase.
>> > I will update my patch with the above testcase.
>>
>> The last few emails in this thread illustrate just why changes based
>> on testcases can be suspect.
>
> Only when it's the wrong testcase ;-)
>
>> I don't believe that ld 2.28 or earlier defined dynamic __start and
>> __stop symbols without a dynamic reference.
>
> If you include dynamic defs (from linked shared libs) as dynamic refs,
> then you're right.  Old binutils did define dynamic __start in app when
> there was a dynamic def in some linked shared lib.
> I.e. the situation of the test in
>   https://sourceware.org/ml/binutils/2018-01/msg00432.html
> That worked in ld 2.26 and is the situation pacemaker wants.
>
>> In fact, you need to use
>> -E or some other way of making symbols dynamic in an executable to
>> support dlsym on *normal* symbols.  I'm going to commit the extra test
>> I posted earlier without -E, plus this patch.
>>
>>       * elflink.c (bfd_elf_define_start_stop): Make __start and __stop
>>       symbols dynamic.
>>
>> diff --git a/bfd/elflink.c b/bfd/elflink.c
>> index e81f6c6..f1ec880 100644
>> --- a/bfd/elflink.c
>> +++ b/bfd/elflink.c
>> @@ -14354,8 +14354,13 @@ bfd_elf_define_start_stop (struct bfd_link_info *info,
>>         bed = get_elf_backend_data (info->output_bfd);
>>         (*bed->elf_backend_hide_symbol) (info, h, TRUE);
>>       }
>> -      else if (ELF_ST_VISIBILITY (h->other) == STV_DEFAULT)
>> -     h->other = (h->other & ~ELF_ST_VISIBILITY (-1)) | STV_PROTECTED;
>> +      else
>> +     {
>> +       if (ELF_ST_VISIBILITY (h->other) == STV_DEFAULT)
>> +         h->other = (h->other & ~ELF_ST_VISIBILITY (-1)) | STV_PROTECTED;
>> +       if (h->ref_dynamic || h->def_dynamic)
>> +         bfd_elf_link_record_dynamic_symbol (info, h);
>> +     }
>
> This doesn't work as you seemingly intended (testcase above fails).
> See the larger context:
>
>   if (h != NULL
>       && (h->root.type == bfd_link_hash_undefined
>     ...
>       h->def_dynamic = 0;
>     ...
>          if (h->ref_dynamic || h->def_dynamic)
>             bfd_elf_link_record_dynamic_symbol (info, h);
>
> It does work after moving the overriding of def_dynamic downwards or
> remembering the old status:
>
> diff --git a/bfd/elflink.c b/bfd/elflink.c
> index f1ec880..3fe4555 100644
> --- a/bfd/elflink.c
> +++ b/bfd/elflink.c
> @@ -14340,6 +14340,7 @@ bfd_elf_define_start_stop (struct bfd_link_info
> *info,
>           || h->root.type == bfd_link_hash_undefweak
>           || ((h->ref_regular || h->def_dynamic) && !h->def_regular)))
>      {
> +      bfd_boolean was_dynamic = h->ref_dynamic || h->def_dynamic;
>        h->root.type = bfd_link_hash_defined;
>        h->root.u.def.section = sec;
>        h->root.u.def.value = 0;
> @@ -14358,7 +14359,7 @@ bfd_elf_define_start_stop (struct bfd_link_info
> *info,
>         {
>           if (ELF_ST_VISIBILITY (h->other) == STV_DEFAULT)
>             h->other = (h->other & ~ELF_ST_VISIBILITY (-1)) | STV_PROTECTED;
> -         if (h->ref_dynamic || h->def_dynamic)
> +         if (was_dynamic)
>             bfd_elf_link_record_dynamic_symbol (info, h);
>         }
>        return &h->root;
>

Can you include your testcase with this patch?

Thanks.

-- 
H.J.