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

[H.J. Lu]

On Tue, Jan 30, 2018 at 6:14 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> 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:

The code looks odd, but works correctly since bfd_elf_define_start_stop
is called after all input objects have been seen.  All def_dynamic have
been turned into ref_dynamic.

>>   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)))

We should check if __start and __stop symbols are referenced
by shared objects, not defined by shared objects.

>>      {
>> +      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?
>

Here is a patch to check ref_dynamic instead.  I added a testcase to verify
that __start/__stop symbols aren't made dynamic when there is no reference
from shared objects.

OK for master?

-- 
H.J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Check-if-__start-__stop-symbols-are-referenced-by-sh.patch
Type: text/x-patch
Size: 2931 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20180130/b3af4dd9/attachment.bin>