[gold][aarch64]Patch to support TLS

[Jing Yu]

On Wed, Sep 17, 2014 at 10:51 AM, Cary Coutant <ccoutant@google.com> wrote:
>> elfcpp/ChangeLog:
>> 2014-09-11  Han Shen  <shenhan@google.com>
>>         * aarch64.h (R_AARCH64_TLS_DTPREL64): Switch enum value with ...
>>         (R_AARCH64_TLS_DTPMOD64): ... enum value.
>>
>> gold/ChangeLog:
>> 2014-09-11  Han Shen  <shenhan@google.com>
>>             Jing Yu  <jingyu@google.com>
>
> Looks good. I found a few typos and made some minor comments. The
> patch is OK with these fixes. Thanks!
>
> -cary
>
>
> +// Above if from Table 4-19, TLS descriptor relocations, 560-569.
>
> s/if/is/
Done.
>
> + tls::Tls_optimization tlsopt =Target_aarch64<size, big_endian>::
> +  optimize_tls_reloc(!parameters->options().shared(), r_type);
>
> Space after '='. (Two locations)
>
Done.
> +    // Create preserved PLT and GOT entries for the resolver.
>
> s/preserved/reserved/
Done.
> @@ -2537,22 +3412,39 @@ Target_aarch64<size, big_endian>::Relocate::relocate(
> ...
>        gold_error_at_location(relinfo, relnum, rela.get_r_offset(),
> +     _("unsupported reloc during scan global %u"),
>
> This is not during the scan pass -- it's in relocate().
> Anyway, telling the user what pass we're in is unlikely to mean
> anything. It would help to print the name of the symbol here:
> "unsupported reloc against global symbol xxx".
Changed the message.
> + gold_error_at_location(relinfo, relnum, rela.get_r_offset(),
> +       _("unsupport gd_to_ie relaxation on %u"),
> +       r_type);
>
> s/unsupport/unsupported/
Done.
>
> +  // The origin sequence is -
>
> s/origin/original/
Done.
>
> +  // Unlike tls_ie_to_le, we change the 3 insns in one funcation call when we
>
> "function"
Done.
>
> +  // relocation type to process. So before proceed, we need to make sure
>
> "before proceeding"
Done.
>
> +  if(!(insn1 == 0x90000000
> +       && insn2 == 0x91000000
> +       && insn3 == 0x94000000))
>
> It would help the reader if you used macros for the various instruction
> bit patterns here and below.
These instruction bit patterns are used once. So I just added comment
describing what the instruction is beside each pattern.
>
> +      gold_error(_("unexpected reloc insn sequence while relaxing "
> +   "tls gd to le for reloc %u."), r_type);
>
> Does this need to be an error? Can it be a warning instead?
The comment above this line was misleading. I changed that.
When the instruction sequence match failed, initially we wanted to
just issue a warning and fall back to do general-dynamic access.
However we realized that we made the gd-to-le decision quite early at
the scan stage, where we did not allocate any GOT or PLT entries for
this relocation. Therefore, we could not do general-dynamic access if
gd-to-le failed. I checked the x86_64 backend, which also fails fast
if gd-to-le check fails.

I updated the misleading comment and inserted new comment describing
the situation to above the gold_error line.

>
> +    default:
> +      gold_error(_("Don't support tlsdesc gd_to_ie optimization on reloc %u"),
> + r_type);
>
> For consistency with other messages, this should say "unsupported"
> instead of "don't support". (And we don't capitalize the first word
> of error messages.)
Done.

Thanks for the review. I updated the code and pushed it to master.
I attached the final patch that I pushed in.

Thanks!
Jing
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tls.patch
Type: text/x-patch
Size: 87336 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20140917/d06a76b6/attachment.bin>