Re: [PATCH] Add a do_assignments hook to ldemul

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

On Thu, May 5, 2016 at 9:26 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Sat, Apr 30, 2016 at 4:10 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Sat, Apr 30, 2016 at 7:40 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Fri, Apr 29, 2016 at 5:25 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> On Fri, Apr 29, 2016 at 6:35 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>> On Fri, Apr 29, 2016 at 5:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>> On Thu, Apr 28, 2016 at 11:35 PM, Alan Modra <amodra@gmail.com> wrote:
>>>>>>> On Thu, Apr 28, 2016 at 05:59:55PM -0700, H.J. Lu wrote:
>>>>>>>> On Thu, Apr 28, 2016 at 4:38 PM, Alan Modra <amodra@gmail.com> wrote:
>>>>>>>> > On Thu, Apr 28, 2016 at 02:01:47PM -0700, H.J. Lu wrote:
>>>>>>>> >> +      /* Symbol is defined.  Check if it is also defined in a regular
>>>>>>>> >> +      input file only when it is currently defined in a dynamic
>>>>>>>> >> +      object, since otherwise, it can't be a __start_<name> nor
>>>>>>>> >> +      __stop_<name> symbol.  */
>>>>>>>> >> +      if (!h->def_dynamic)
>>>>>>>> >>       return NULL;
>>>>>>>> > [snip]
>>>>>>>> >> +         if (s != NULL)
>>>>>>>> >> +           {
>>>>>>>> >> +             h->root.u.undef.section = s;
>>>>>>>> >> +             break;
>>>>>>>> >> +           }
>>>>>>>> >
>>>>>>>> > You can't set u.undef here on a defined symbol.  That's just too ugly,
>>>>>>>> > even if you later set it to undefined.  Better to force it
>>>>>>>> > bfd_link_hash_undefined here.
>>>>>>>>
>>>>>>>> Like this?
>>>>>>>
>>>>>>> No, I meant that if you want to make use of the u.undef.section cache,
>>>>>>> then set it to undefined first.  If you don't intend to set
>>>>>>> u.undef.section then set to undefined later.
>>>>>>>
>>>>>>>> > This is getting quite messy, and I'm wondering if we even need
>>>>>>>> > _bfd_elf_is_start_stop, except for gc-sections code.
>>>>>>>
>>>>>>> Note that when I wrote _bfd_elf_is_start_stop I thought it might come
>>>>>>> in useful in check_relocs, to prevent __start_* and __stop_* from
>>>>>>> being seen as dynamic.  Now that you're running both
>>>>>>> find_statement_assignment and lang_do_assignments before check_relocs,
>>>>>>> doing anything special with _bfd_elf_is_start_stop should not be
>>>>>>> necessary.
>>>>>>
>>>>>>  __start_* and __stop_* aren't handled properly with shared library:
>>>>>>
>>>>>> https://sourceware.org/bugzilla/show_bug.cgi?id=20022
>>>>>>
>>>>>> _bfd_elf_is_start_stop needs to check symbols defined in a
>>>>>> shared library.
>>>>>>
>>>>>>> find_statement_assignment has given the bfd backend a look at symbols,
>>>>>>> and you've processed the PROVIDE (__start_sec = .) linker script
>>>>>>> statements in lang_do_assignments.  So it ought to be possible to make
>>>>>>> __start_sec a def_regular normal symbol by that point.  As I said
>>>>>>> before, I'd expect that to work better if find_statement_assignment
>>>>>>> ran before lang_do_assignments.  That allows
>>>>>>> bfd_elf_record_link_assignment to force def_dynamic symbols to
>>>>>>> bfd_link_hash_undefined so that ldexp.c code handling PROVIDE doesn't
>>>>>>> need to know ELF specific details.
>>>>>>>
>>>>>>> However, you said that doing it that way didn't work.  What didn't
>>>>>>> work, and what needs to change in bfd_elf_record_link_assignment to
>>>>>>> make it work?
>>>>>>
>>>>>> The failed testcase is ld-elf/ehdr_start-userdef.  We have
>>>>>>
>>>>>>          /* Only adjust the export class if the symbol was referenced
>>>>>>              and not defined, otherwise leave it alone.  */
>>>>>>           if (h != NULL
>>>>>>               && (h->root.type == bfd_link_hash_new
>>>>>>                   || h->root.type == bfd_link_hash_undefined
>>>>>>                   || h->root.type == bfd_link_hash_undefweak
>>>>>>                   || h->root.type == bfd_link_hash_common))
>>>>>>             {
>>>>>>
>>>>>> Since this is run before  lang_do_assignments, we don't see
>>>>>>
>>>>>> __ehdr_start = 0x12345678;
>>>>>>
>>>>>> in linker script and get
>>>>>>
>>>>>> (gdb) p *h
>>>>>> $2 = {root = {root = {next = 0x0, string = 0x859bad "__ehdr_start",
>>>>>>       hash = 78192523}, type = bfd_link_hash_undefined, non_ir_ref = 0,
>>>>>>     linker_def = 0, u = {undef = {next = 0x0, abfd = 0x857100, section = 0x0},
>>>>>>       def = {next = 0x0, section = 0x857100, value = 0}, i = {next = 0x0,
>>>>>>         link = 0x857100, warning = 0x0}, c = {next = 0x0, p = 0x857100,
>>>>>>         size = 0}}}, indx = -1, dynindx = -1, got = {refcount = 0, offset = 0,
>>>>>>     glist = 0x0, plist = 0x0}, plt = {refcount = 0, offset = 0, glist = 0x0,
>>>>>>     plist = 0x0}, size = 0, type = 0, other = 0, target_internal = 0,
>>>>>>   ref_regular = 1, def_regular = 0, ref_dynamic = 0, def_dynamic = 0,
>>>>>>   ref_regular_nonweak = 1, dynamic_adjusted = 0, needs_copy = 0,
>>>>>>   needs_plt = 0, non_elf = 0, versioned = unversioned, forced_local = 0,
>>>>>>   dynamic = 0, mark = 0, non_got_ref = 0, dynamic_def = 0,
>>>>>>   ref_dynamic_nonweak = 0, pointer_equality_needed = 0, unique_global = 0,
>>>>>>   protected_def = 0, dynstr_index = 0, u = {weakdef = 0x0,
>>>>>>     elf_hash_value = 0}, verinfo = {verdef = 0x0, vertree = 0x0}, vtable = 0x0}
>>>>>>
>>>>>> We get
>>>>>>
>>>>>>      3: 0000000012345678     0 NOTYPE  LOCAL  DEFAULT  ABS __ehdr_start
>>>>>>
>>>>>>
>>>>>> instead of
>>>>>>
>>>>>>      3: 0000000012345678     0 NOTYPE  GLOBAL  DEFAULT  ABS __ehdr_start
>>>>>>
>>>>>> in output.
>>>>>
>>>>> I have a fix for it.  But _bfd_elf_is_start_stop still needs fix.
>>>>
>>>> Here is the updated patch, which doesn't use _bfd_elf_is_start_stop.
>>>>
>>>> OK for master?
>>>>
>>>
>>> Scan over all input files for each start/stop symbol may be too
>>> expensive.  Assuming there are fewer start/stop symbols than
>>> input files,  this patch collects all start/stop symbols first and
>>> only scan over all input files once.
>>>
>>> Comments, feedbacks?
>>>
>>
>> An updated patch.
>
> This patch enables the do_assignments hook only if
> CALL_LDEMUL_DO_ASSIGNMENTS is yes.
>
> Any comments, feedbacks?
>

I withdrew this patch since it also requires to call
_bfd_elf_link_assign_sym_version before check_relocs.

-- 
H.J.