Re: [PATCH] [aarch64] Fix handling of stubs to weak and section symbols

["Han Shen via binutils" <binutils at sourceware dot org>]

On Mon, Jun 19, 2017 at 5:49 PM, Eric Christopher <echristo@gmail.com> wrote:
>
> Hi Cary, Han,
>
> This patch has 3 notable changes:
>
> -         symval.set_output_value(this->plt_section()->address()
> -                                 + gsym->plt_offset());
> +         symval.set_output_value(this->plt_address_for_global(gsym));
>
> which is just a boring fix to update to use the correct API. Looks
> like it didn't get updated with the others.
>

Yeah, good catch. This seems affect handling STT_GNU_IFUNC.

> +  // We don't create stubs for undefined symbols, but do for weak.
> +  if (gsym
> +      && !gsym->use_plt_offset(arp->reference_flags())
> +      && gsym->is_undefined())
>
> which is a small fix on top of my previous one for undefined symbols
> to match the associated code in scan_reloc_for_stub by including stubs
> for undefined weak symbols.
>
> and finally:
>
> -      // If symbol is a section symbol, we don't know the actual type of
> -      // destination.  Give up.
> -      if (psymval->is_section_symbol())
> -       continue;
>
> which appears to have been pulled from the arm port when the aarch64
> port was created. I looked and this was put into the arm port when
> stub creation was first put in with no other comments. I can't think
> of a reason why we wouldn't want to be able to have stubs to section
> symbols - at least not as long as they're STT_FUNC or in separate
> sections (these are all likely to be both). I think the original idea
> was the return if we didn't know if the symbol was STT_FUNC, but not
> entirely positive (cc'ing Doug in case someone sees this discussion in
> the future and he wants to chime in). It might be possible to put a
> suitable assert or verification into the stub creation in order to
> verify that all of the bits of the ABI are satisfied:
>
> The target symbol has type STT_FUNC.
> Or, the target symbol and relocated place are in separate sections
> input to the linker.
> Or, the target symbol is undefined (external to the link unit).
>
> but it's a little harder to figure out the best when/where since a lot
> of this code is spread out.

Yeah, if Doug wrote the lines, he must know it better :)

>
>
> At any rate, I'm convinced that this is safe given that we're now able
> to link and execute a bunch of code that wouldn't work before on some
> fairly extensive internal testing.
>
> Thoughts? OK?

LGTM, thanks.

>
> -eric
>
> 2017-06-19  Eric Christopher  <echristo@gmail.com>
>
> * aarch64.cc (scan_reloc_for_stub): Use plt_address_for_global to
> calculate the symbol value.
> (scan_reloc_section_for_stubs): Allow stubs to be created for
> section symbols.
> (maybe_apply_stub): Handle creating stubs for weak symbols to
> match the code in scan_reloc_for_stub.



-Han Shen