[3/5] LoongArch: Make protected function symbols local for -shared

Message ID 20240622100307.9498-4-xry111@xry111.site
State New
Headers
Series LoongArch: Add DT_RELR (packing relative relocs) support |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Xi Ruoyao June 22, 2024, 10:03 a.m. UTC
  On LoongArch there is no reason to treat STV_PROTECTED STT_FUNC symbols
as preemptable.  See the comment above LARCH_REF_LOCAL for detailed
explanation.

Signed-off-by: Xi Ruoyao <xry111@xry111.site>
---
 bfd/elfnn-loongarch.c                         | 78 ++++++++++++++-----
 ld/testsuite/ld-loongarch-elf/ifunc-reloc.d   |  2 +-
 .../ld-loongarch-elf/ld-loongarch-elf.exp     |  1 +
 .../ld-loongarch-elf/protected-func.d         |  6 ++
 .../ld-loongarch-elf/protected-func.s         | 17 ++++
 5 files changed, 83 insertions(+), 21 deletions(-)
 create mode 100644 ld/testsuite/ld-loongarch-elf/protected-func.d
 create mode 100644 ld/testsuite/ld-loongarch-elf/protected-func.s
  

Comments

Fangrui Song June 22, 2024, 5:37 p.m. UTC | #1
On Sat, Jun 22, 2024 at 3:03 AM Xi Ruoyao <xry111@xry111.site> wrote:
>
> On LoongArch there is no reason to treat STV_PROTECTED STT_FUNC symbols
> as preemptable.  See the comment above LARCH_REF_LOCAL for detailed
> explanation.
>
> Signed-off-by: Xi Ruoyao <xry111@xry111.site>
> ---
>  bfd/elfnn-loongarch.c                         | 78 ++++++++++++++-----
>  ld/testsuite/ld-loongarch-elf/ifunc-reloc.d   |  2 +-
>  .../ld-loongarch-elf/ld-loongarch-elf.exp     |  1 +
>  .../ld-loongarch-elf/protected-func.d         |  6 ++
>  .../ld-loongarch-elf/protected-func.s         | 17 ++++
>  5 files changed, 83 insertions(+), 21 deletions(-)
>  create mode 100644 ld/testsuite/ld-loongarch-elf/protected-func.d
>  create mode 100644 ld/testsuite/ld-loongarch-elf/protected-func.s
>
> diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
> index e0b3e9f5ee6..2acb5c7fa95 100644
> --- a/bfd/elfnn-loongarch.c
> +++ b/bfd/elfnn-loongarch.c
> @@ -181,6 +181,46 @@ struct loongarch_elf_link_hash_table
>      } \
>      while (0)
>
> +/* TL;DR always use it in this file instead when you want to type
> +   SYMBOL_REFERENCES_LOCAL.
> +
> +   It's like SYMBOL_REFERENCES_LOCAL, but it returns true for local
> +   protected functions.  It happens to be same as SYMBOL_CALLS_LOCAL but
> +   let's not reuse SYMBOL_CALLS_LOCAL or "CALLS" may puzzle people.
> +
> +   We do generate the so-called "canonical PLT entry" when someone attempts
> +   to la.pcrel an external function.  But this is only an unwanted side-
> +   effect from using R_LARCH_PCALA_{HI20,LO12} for medium and extreme
> +   code model function call: we have the same problem as i386 where
> +   R_386_PC32 is used for both call and lea (for medium code model new code
> +   should use R_LARCH_CALL36 instead), but unlike i386 we never really
> +   implemented "R_LARCH_COPY" thus attempting to la.pcrel an external
> +   symbol is always considered a programming error unless it's a part of
> +   a extreme code model function call, and pointer equality will be broken
> +   even with a STV_DEFAULT function:
> +
> +   $ cat t.c
> +   #include <assert.h>
> +   void check(void *p) {assert(p == check);}
> +   $ cat main.c
> +   extern void check(void *);
> +   int main(void) { check(check); }
> +   $ cc t.c -fPIC -shared -o t.so
> +   $ cc main.c -mdirect-extern-access t.so -Wl,-rpath=.
> +   $ ./a.out
> +   a.out: t.c:2: check: Assertion `p == check' failed.
> +   Aborted
> +
> +   Thus handling STV_PROTECTED function specially just fixes nothing:
> +   adding -fvisibility=protected compiling t.c will not magically fix
> +   the inequality.  The only possible and correct fix is not to use
> +   -mdirect-extern-access.
> +
> +   So we should remove this special handling, because it's only an
> +   unsuccessful workaround for invalid code and it's penalizing valid
> +   code.  */
> +#define LARCH_REF_LOCAL(info, h) \
> +  (_bfd_elf_symbol_refs_local_p ((h), (info), true))
>
>  /* Generate a PLT header.  */
>
> @@ -712,7 +752,7 @@ loongarch_tls_transition_without_check (struct bfd_link_info *info,
>                                         struct elf_link_hash_entry *h)
>  {
>    bool local_exec = bfd_link_executable (info)
> -                   && SYMBOL_REFERENCES_LOCAL (info, h);
> +                   && LARCH_REF_LOCAL (info, h);
>
>    switch (r_type)
>      {
> @@ -1196,7 +1236,7 @@ loongarch_elf_adjust_dynamic_symbol (struct bfd_link_info *info,
>      {
>        if (h->plt.refcount <= 0
>           || (h->type != STT_GNU_IFUNC
> -             && (SYMBOL_REFERENCES_LOCAL (info, h)
> +             && (LARCH_REF_LOCAL (info, h)
>                   || (ELF_ST_VISIBILITY (h->other) != STV_DEFAULT
>                       && h->root.type == bfd_link_hash_undefweak))))
>         {
> @@ -1714,14 +1754,14 @@ elfNN_allocate_ifunc_dynrelocs (struct elf_link_hash_entry *h,
>       here if it is defined and referenced in a non-shared object.  */
>    if (h->type == STT_GNU_IFUNC && h->def_regular)
>      {
> -      if (ref_local && SYMBOL_REFERENCES_LOCAL (info, h))
> +      if (ref_local && LARCH_REF_LOCAL (info, h))
>         return local_allocate_ifunc_dyn_relocs (info, h,
>                                                 &h->dyn_relocs,
>                                                 PLT_ENTRY_SIZE,
>                                                 PLT_HEADER_SIZE,
>                                                 GOT_ENTRY_SIZE,
>                                                 false);
> -      else if (!ref_local && !SYMBOL_REFERENCES_LOCAL (info, h))
> +      else if (!ref_local && !LARCH_REF_LOCAL (info, h))
>         return _bfd_elf_allocate_ifunc_dyn_relocs (info, h,
>                                                    &h->dyn_relocs,
>                                                    PLT_ENTRY_SIZE,
> @@ -1749,7 +1789,6 @@ elfNN_allocate_ifunc_dynrelocs_ref_global (struct elf_link_hash_entry *h,
>                                          false);
>  }
>
> -
>  /* Allocate space in .plt, .got and associated reloc sections for
>     ifunc dynamic relocs.  */
>
> @@ -2662,7 +2701,6 @@ tlsoff (struct bfd_link_info *info, bfd_vma addr)
>    return addr - elf_hash_table (info)->tls_sec->vma;
>  }
>
> -
>  static int
>  loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>                                 bfd *input_bfd, asection *input_section,
> @@ -2788,7 +2826,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>             {
>               defined_local = !unresolved_reloc && !ignored;
>               resolved_local =
> -               defined_local && SYMBOL_REFERENCES_LOCAL (info, h);
> +               defined_local && LARCH_REF_LOCAL (info, h);
>               resolved_dynly = !resolved_local;
>               resolved_to_const = !resolved_local && !resolved_dynly;
>             }
> @@ -2877,7 +2915,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>                       outrel.r_addend = 0;
>                     }
>
> -                 if (SYMBOL_REFERENCES_LOCAL (info, h))
> +                 if (LARCH_REF_LOCAL (info, h))
>                     {
>
>                       if (htab->elf.splt != NULL)
> @@ -3246,7 +3284,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>                   if (!WILL_CALL_FINISH_DYNAMIC_SYMBOL (is_dyn,
>                                                         bfd_link_pic (info), h)
>                       && ((bfd_link_pic (info)
> -                          && SYMBOL_REFERENCES_LOCAL (info, h))))
> +                          && LARCH_REF_LOCAL (info, h))))
>                     {
>                       /* This is actually a static link, or it is a
>                          -Bsymbolic link and the symbol is defined
> @@ -3391,7 +3429,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>                 asection *srel = htab->elf.srelgot;
>                 bfd_vma tls_block_off = 0;
>
> -               if (SYMBOL_REFERENCES_LOCAL (info, h))
> +               if (LARCH_REF_LOCAL (info, h))
>                   {
>                     BFD_ASSERT (elf_hash_table (info)->tls_sec);
>                     tls_block_off = relocation
> @@ -3402,7 +3440,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>                   {
>                     rela.r_offset = sec_addr (got) + got_off;
>                     rela.r_addend = 0;
> -                   if (SYMBOL_REFERENCES_LOCAL (info, h))
> +                   if (LARCH_REF_LOCAL (info, h))
>                       {
>                         /* Local sym, used in exec, set module id 1.  */
>                         if (bfd_link_executable (info))
> @@ -3435,7 +3473,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>                 if (tls_type & GOT_TLS_IE)
>                   {
>                     rela.r_offset = sec_addr (got) + got_off + ie_off;
> -                   if (SYMBOL_REFERENCES_LOCAL (info, h))
> +                   if (LARCH_REF_LOCAL (info, h))
>                       {
>                         /* Local sym, used in exec, set module id 1.  */
>                         if (!bfd_link_executable (info))
> @@ -3637,7 +3675,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>                                                             bfd_link_pic (info),
>                                                             h)
>                           && bfd_link_pic (info)
> -                         && SYMBOL_REFERENCES_LOCAL (info, h))
> +                         && LARCH_REF_LOCAL (info, h))
>                         {
>                           Elf_Internal_Rela rela;
>                           rela.r_offset = sec_addr (got) + got_off;
> @@ -4178,7 +4216,7 @@ loongarch_tls_perform_trans (bfd *abfd, asection *sec,
>  {
>    unsigned long insn;
>    bool local_exec = bfd_link_executable (info)
> -                     && SYMBOL_REFERENCES_LOCAL (info, h);
> +                     && LARCH_REF_LOCAL (info, h);
>    bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;
>    unsigned long r_type = ELFNN_R_TYPE (rel->r_info);
>    unsigned long r_symndx = ELFNN_R_SYM (rel->r_info);
> @@ -4890,7 +4928,7 @@ loongarch_elf_relax_section (bfd *abfd, asection *sec,
>           else
>             continue;
>
> -         if (h && SYMBOL_REFERENCES_LOCAL (info, h))
> +         if (h && LARCH_REF_LOCAL (info, h))
>             local_got = true;
>           symtype = h->type;
>         }
> @@ -5027,12 +5065,12 @@ loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
>        if (htab->elf.splt)
>         {
>           BFD_ASSERT ((h->type == STT_GNU_IFUNC
> -                      && SYMBOL_REFERENCES_LOCAL (info, h))
> +                      && LARCH_REF_LOCAL (info, h))
>                       || h->dynindx != -1);
>
>           plt = htab->elf.splt;
>           gotplt = htab->elf.sgotplt;
> -         if (h->type == STT_GNU_IFUNC && SYMBOL_REFERENCES_LOCAL (info, h))
> +         if (h->type == STT_GNU_IFUNC && LARCH_REF_LOCAL (info, h))
>             relplt = htab->elf.srelgot;
>           else
>             relplt = htab->elf.srelplt;
> @@ -5043,7 +5081,7 @@ loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
>        else /* if (htab->elf.iplt) */
>         {
>           BFD_ASSERT (h->type == STT_GNU_IFUNC
> -                     && SYMBOL_REFERENCES_LOCAL (info, h));
> +                     && LARCH_REF_LOCAL (info, h));
>
>           plt = htab->elf.iplt;
>           gotplt = htab->elf.igotplt;
> @@ -5131,7 +5169,7 @@ loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
>               if (htab->elf.splt == NULL)
>                 srela = htab->elf.irelplt;
>
> -             if (SYMBOL_REFERENCES_LOCAL (info, h))
> +             if (LARCH_REF_LOCAL (info, h))
>                 {
>                   asection *sec = h->root.u.def.section;
>                   rela.r_info = ELFNN_R_INFO (0, R_LARCH_IRELATIVE);
> @@ -5168,7 +5206,7 @@ loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
>               return true;
>             }
>         }
> -      else if (bfd_link_pic (info) && SYMBOL_REFERENCES_LOCAL (info, h))
> +      else if (bfd_link_pic (info) && LARCH_REF_LOCAL (info, h))
>         {
>           asection *sec = h->root.u.def.section;
>           rela.r_info = ELFNN_R_INFO (0, R_LARCH_RELATIVE);
> diff --git a/ld/testsuite/ld-loongarch-elf/ifunc-reloc.d b/ld/testsuite/ld-loongarch-elf/ifunc-reloc.d
> index cb592874b1e..968e7564b49 100644
> --- a/ld/testsuite/ld-loongarch-elf/ifunc-reloc.d
> +++ b/ld/testsuite/ld-loongarch-elf/ifunc-reloc.d
> @@ -8,6 +8,7 @@
>  .* R_LARCH_IRELATIVE .*
>  .* R_LARCH_IRELATIVE .*
>  .* R_LARCH_IRELATIVE .*
> +.* R_LARCH_IRELATIVE .*
>  #...
>  .*'\.rela\.plt'.*
>  #...
> @@ -16,4 +17,3 @@
>  .* R_LARCH_JUMP_SLOT .*
>  .* R_LARCH_JUMP_SLOT .*
>  .* R_LARCH_JUMP_SLOT .*
> -.* R_LARCH_JUMP_SLOT .*
> diff --git a/ld/testsuite/ld-loongarch-elf/ld-loongarch-elf.exp b/ld/testsuite/ld-loongarch-elf/ld-loongarch-elf.exp
> index b8721f0eaff..3172a0ab4b5 100644
> --- a/ld/testsuite/ld-loongarch-elf/ld-loongarch-elf.exp
> +++ b/ld/testsuite/ld-loongarch-elf/ld-loongarch-elf.exp
> @@ -141,6 +141,7 @@ if [istarget "loongarch64-*-*"] {
>      run_dump_test "reloc_abs_with_shared"
>      run_dump_test "r_larch_32_elf64"
>      run_dump_test "ifunc-reloc"
> +    run_dump_test "protected-func"
>    }
>
>    if [check_pie_support] {
> diff --git a/ld/testsuite/ld-loongarch-elf/protected-func.d b/ld/testsuite/ld-loongarch-elf/protected-func.d
> new file mode 100644
> index 00000000000..501c7cb5f8c
> --- /dev/null
> +++ b/ld/testsuite/ld-loongarch-elf/protected-func.d
> @@ -0,0 +1,6 @@
> +#ld: -shared
> +#readelf: -Wr
> +
> +#...
> +.* R_LARCH_RELATIVE .*
> +.* R_LARCH_RELATIVE .*
> diff --git a/ld/testsuite/ld-loongarch-elf/protected-func.s b/ld/testsuite/ld-loongarch-elf/protected-func.s
> new file mode 100644
> index 00000000000..347e371a777
> --- /dev/null
> +++ b/ld/testsuite/ld-loongarch-elf/protected-func.s
> @@ -0,0 +1,17 @@
> +# protected function should be unpreemptable and relocated with

LGTM.

unpreemptable => non-preemptible.

Regarding "preemptible" vs "preemptable": the former is used much more
than the latter.

> +# R_LARCH_RELATIVE in shared library, for both GOT and pointer data
> +
> +.globl x
> +.protected x
> +.type x, @function
> +x:
> +  ret
> +
> +.globl _start
> +_start:
> +  la.got $a0, x
> +  ret
> +
> +.data
> +p:
> +  .quad x
> --
> 2.45.2
>
  
mengqinggang June 25, 2024, 2:39 a.m. UTC | #2
在 2024/6/22 下午6:03, Xi Ruoyao 写道:
> On LoongArch there is no reason to treat STV_PROTECTED STT_FUNC symbols
> as preemptable.  See the comment above LARCH_REF_LOCAL for detailed
> explanation.
>
> Signed-off-by: Xi Ruoyao <xry111@xry111.site>
> ---
>   bfd/elfnn-loongarch.c                         | 78 ++++++++++++++-----
>   ld/testsuite/ld-loongarch-elf/ifunc-reloc.d   |  2 +-
>   .../ld-loongarch-elf/ld-loongarch-elf.exp     |  1 +
>   .../ld-loongarch-elf/protected-func.d         |  6 ++
>   .../ld-loongarch-elf/protected-func.s         | 17 ++++
>   5 files changed, 83 insertions(+), 21 deletions(-)
>   create mode 100644 ld/testsuite/ld-loongarch-elf/protected-func.d
>   create mode 100644 ld/testsuite/ld-loongarch-elf/protected-func.s
>
> diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
> index e0b3e9f5ee6..2acb5c7fa95 100644
> --- a/bfd/elfnn-loongarch.c
> +++ b/bfd/elfnn-loongarch.c
> @@ -181,6 +181,46 @@ struct loongarch_elf_link_hash_table
>       } \
>       while (0)
>   
> +/* TL;DR always use it in this file instead when you want to type
> +   SYMBOL_REFERENCES_LOCAL.
> +
> +   It's like SYMBOL_REFERENCES_LOCAL, but it returns true for local
> +   protected functions.  It happens to be same as SYMBOL_CALLS_LOCAL but
> +   let's not reuse SYMBOL_CALLS_LOCAL or "CALLS" may puzzle people.
> +
> +   We do generate the so-called "canonical PLT entry" when someone attempts
> +   to la.pcrel an external function.  But this is only an unwanted side-
> +   effect from using R_LARCH_PCALA_{HI20,LO12} for medium and extreme
> +   code model function call: we have the same problem as i386 where


Extreme code model function call use GOT not PLT.

   10:   1a00000d        pcalau12i       $t1, 0
                         10: R_LARCH_GOT_PC_HI20 f
   14:   02c0000c        li.d            $t0, 0
                         14: R_LARCH_GOT_PC_LO12 f
   18:   1600000c        lu32i.d         $t0, 0
                         18: R_LARCH_GOT64_PC_LO20       f
   1c:   0300018c        lu52i.d         $t0, $t0, 0
                         1c: R_LARCH_GOT64_PC_HI12       f
   20:   380c31ac        ldx.d           $t0, $t1, $t0
   24:   4c000181        jirl            $ra, $t0, 0

> +   R_386_PC32 is used for both call and lea (for medium code model new code
> +   should use R_LARCH_CALL36 instead), but unlike i386 we never really
> +   implemented "R_LARCH_COPY" thus attempting to la.pcrel an external
> +   symbol is always considered a programming error unless it's a part of
> +   a extreme code model function call, and pointer equality will be broken
> +   even with a STV_DEFAULT function:
> +
> +   $ cat t.c
> +   #include <assert.h>
> +   void check(void *p) {assert(p == check);}
> +   $ cat main.c
> +   extern void check(void *);
> +   int main(void) { check(check); }
> +   $ cc t.c -fPIC -shared -o t.so
> +   $ cc main.c -mdirect-extern-access t.so -Wl,-rpath=.
> +   $ ./a.out
> +   a.out: t.c:2: check: Assertion `p == check' failed.
> +   Aborted
> +
> +   Thus handling STV_PROTECTED function specially just fixes nothing:
> +   adding -fvisibility=protected compiling t.c will not magically fix
> +   the inequality.  The only possible and correct fix is not to use
> +   -mdirect-extern-access.
> +
> +   So we should remove this special handling, because it's only an
> +   unsuccessful workaround for invalid code and it's penalizing valid
> +   code.  */
> +#define LARCH_REF_LOCAL(info, h) \
> +  (_bfd_elf_symbol_refs_local_p ((h), (info), true))
>   
>   /* Generate a PLT header.  */
>   
> @@ -712,7 +752,7 @@ loongarch_tls_transition_without_check (struct bfd_link_info *info,
>   					struct elf_link_hash_entry *h)
>   {
>     bool local_exec = bfd_link_executable (info)
> -		    && SYMBOL_REFERENCES_LOCAL (info, h);
> +		    && LARCH_REF_LOCAL (info, h);
>   
>     switch (r_type)
>       {
> @@ -1196,7 +1236,7 @@ loongarch_elf_adjust_dynamic_symbol (struct bfd_link_info *info,
>       {
>         if (h->plt.refcount <= 0
>   	  || (h->type != STT_GNU_IFUNC
> -	      && (SYMBOL_REFERENCES_LOCAL (info, h)
> +	      && (LARCH_REF_LOCAL (info, h)
>   		  || (ELF_ST_VISIBILITY (h->other) != STV_DEFAULT
>   		      && h->root.type == bfd_link_hash_undefweak))))
>   	{
> @@ -1714,14 +1754,14 @@ elfNN_allocate_ifunc_dynrelocs (struct elf_link_hash_entry *h,
>        here if it is defined and referenced in a non-shared object.  */
>     if (h->type == STT_GNU_IFUNC && h->def_regular)
>       {
> -      if (ref_local && SYMBOL_REFERENCES_LOCAL (info, h))
> +      if (ref_local && LARCH_REF_LOCAL (info, h))
>   	return local_allocate_ifunc_dyn_relocs (info, h,
>   						&h->dyn_relocs,
>   						PLT_ENTRY_SIZE,
>   						PLT_HEADER_SIZE,
>   						GOT_ENTRY_SIZE,
>   						false);
> -      else if (!ref_local && !SYMBOL_REFERENCES_LOCAL (info, h))
> +      else if (!ref_local && !LARCH_REF_LOCAL (info, h))
>   	return _bfd_elf_allocate_ifunc_dyn_relocs (info, h,
>   						   &h->dyn_relocs,
>   						   PLT_ENTRY_SIZE,
> @@ -1749,7 +1789,6 @@ elfNN_allocate_ifunc_dynrelocs_ref_global (struct elf_link_hash_entry *h,
>   					 false);
>   }
>   
> -
>   /* Allocate space in .plt, .got and associated reloc sections for
>      ifunc dynamic relocs.  */
>   
> @@ -2662,7 +2701,6 @@ tlsoff (struct bfd_link_info *info, bfd_vma addr)
>     return addr - elf_hash_table (info)->tls_sec->vma;
>   }
>   
> -
>   static int
>   loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   				bfd *input_bfd, asection *input_section,
> @@ -2788,7 +2826,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   	    {
>   	      defined_local = !unresolved_reloc && !ignored;
>   	      resolved_local =
> -		defined_local && SYMBOL_REFERENCES_LOCAL (info, h);
> +		defined_local && LARCH_REF_LOCAL (info, h);
>   	      resolved_dynly = !resolved_local;
>   	      resolved_to_const = !resolved_local && !resolved_dynly;
>   	    }
> @@ -2877,7 +2915,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   		      outrel.r_addend = 0;
>   		    }
>   
> -		  if (SYMBOL_REFERENCES_LOCAL (info, h))
> +		  if (LARCH_REF_LOCAL (info, h))
>   		    {
>   
>   		      if (htab->elf.splt != NULL)
> @@ -3246,7 +3284,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   		  if (!WILL_CALL_FINISH_DYNAMIC_SYMBOL (is_dyn,
>   							bfd_link_pic (info), h)
>   		      && ((bfd_link_pic (info)
> -			   && SYMBOL_REFERENCES_LOCAL (info, h))))
> +			   && LARCH_REF_LOCAL (info, h))))
>   		    {
>   		      /* This is actually a static link, or it is a
>   			 -Bsymbolic link and the symbol is defined
> @@ -3391,7 +3429,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   		asection *srel = htab->elf.srelgot;
>   		bfd_vma tls_block_off = 0;
>   
> -		if (SYMBOL_REFERENCES_LOCAL (info, h))
> +		if (LARCH_REF_LOCAL (info, h))
>   		  {
>   		    BFD_ASSERT (elf_hash_table (info)->tls_sec);
>   		    tls_block_off = relocation
> @@ -3402,7 +3440,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   		  {
>   		    rela.r_offset = sec_addr (got) + got_off;
>   		    rela.r_addend = 0;
> -		    if (SYMBOL_REFERENCES_LOCAL (info, h))
> +		    if (LARCH_REF_LOCAL (info, h))
>   		      {
>   			/* Local sym, used in exec, set module id 1.  */
>   			if (bfd_link_executable (info))
> @@ -3435,7 +3473,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   		if (tls_type & GOT_TLS_IE)
>   		  {
>   		    rela.r_offset = sec_addr (got) + got_off + ie_off;
> -		    if (SYMBOL_REFERENCES_LOCAL (info, h))
> +		    if (LARCH_REF_LOCAL (info, h))
>   		      {
>   			/* Local sym, used in exec, set module id 1.  */
>   			if (!bfd_link_executable (info))
> @@ -3637,7 +3675,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   							    bfd_link_pic (info),
>   							    h)
>   			  && bfd_link_pic (info)
> -			  && SYMBOL_REFERENCES_LOCAL (info, h))
> +			  && LARCH_REF_LOCAL (info, h))
>   			{
>   			  Elf_Internal_Rela rela;
>   			  rela.r_offset = sec_addr (got) + got_off;
> @@ -4178,7 +4216,7 @@ loongarch_tls_perform_trans (bfd *abfd, asection *sec,
>   {
>     unsigned long insn;
>     bool local_exec = bfd_link_executable (info)
> -		      && SYMBOL_REFERENCES_LOCAL (info, h);
> +		      && LARCH_REF_LOCAL (info, h);
>     bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;
>     unsigned long r_type = ELFNN_R_TYPE (rel->r_info);
>     unsigned long r_symndx = ELFNN_R_SYM (rel->r_info);
> @@ -4890,7 +4928,7 @@ loongarch_elf_relax_section (bfd *abfd, asection *sec,
>   	  else
>   	    continue;
>   
> -	  if (h && SYMBOL_REFERENCES_LOCAL (info, h))
> +	  if (h && LARCH_REF_LOCAL (info, h))
>   	    local_got = true;
>   	  symtype = h->type;
>   	}
> @@ -5027,12 +5065,12 @@ loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
>         if (htab->elf.splt)
>   	{
>   	  BFD_ASSERT ((h->type == STT_GNU_IFUNC
> -		       && SYMBOL_REFERENCES_LOCAL (info, h))
> +		       && LARCH_REF_LOCAL (info, h))
>   		      || h->dynindx != -1);
>   
>   	  plt = htab->elf.splt;
>   	  gotplt = htab->elf.sgotplt;
> -	  if (h->type == STT_GNU_IFUNC && SYMBOL_REFERENCES_LOCAL (info, h))
> +	  if (h->type == STT_GNU_IFUNC && LARCH_REF_LOCAL (info, h))
>   	    relplt = htab->elf.srelgot;
>   	  else
>   	    relplt = htab->elf.srelplt;
> @@ -5043,7 +5081,7 @@ loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
>         else /* if (htab->elf.iplt) */
>   	{
>   	  BFD_ASSERT (h->type == STT_GNU_IFUNC
> -		      && SYMBOL_REFERENCES_LOCAL (info, h));
> +		      && LARCH_REF_LOCAL (info, h));
>   
>   	  plt = htab->elf.iplt;
>   	  gotplt = htab->elf.igotplt;
> @@ -5131,7 +5169,7 @@ loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
>   	      if (htab->elf.splt == NULL)
>   		srela = htab->elf.irelplt;
>   
> -	      if (SYMBOL_REFERENCES_LOCAL (info, h))
> +	      if (LARCH_REF_LOCAL (info, h))
>   		{
>   		  asection *sec = h->root.u.def.section;
>   		  rela.r_info = ELFNN_R_INFO (0, R_LARCH_IRELATIVE);
> @@ -5168,7 +5206,7 @@ loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
>   	      return true;
>   	    }
>   	}
> -      else if (bfd_link_pic (info) && SYMBOL_REFERENCES_LOCAL (info, h))
> +      else if (bfd_link_pic (info) && LARCH_REF_LOCAL (info, h))
>   	{
>   	  asection *sec = h->root.u.def.section;
>   	  rela.r_info = ELFNN_R_INFO (0, R_LARCH_RELATIVE);
> diff --git a/ld/testsuite/ld-loongarch-elf/ifunc-reloc.d b/ld/testsuite/ld-loongarch-elf/ifunc-reloc.d
> index cb592874b1e..968e7564b49 100644
> --- a/ld/testsuite/ld-loongarch-elf/ifunc-reloc.d
> +++ b/ld/testsuite/ld-loongarch-elf/ifunc-reloc.d
> @@ -8,6 +8,7 @@
>   .* R_LARCH_IRELATIVE .*
>   .* R_LARCH_IRELATIVE .*
>   .* R_LARCH_IRELATIVE .*
> +.* R_LARCH_IRELATIVE .*
>   #...
>   .*'\.rela\.plt'.*
>   #...
> @@ -16,4 +17,3 @@
>   .* R_LARCH_JUMP_SLOT .*
>   .* R_LARCH_JUMP_SLOT .*
>   .* R_LARCH_JUMP_SLOT .*
> -.* R_LARCH_JUMP_SLOT .*
> diff --git a/ld/testsuite/ld-loongarch-elf/ld-loongarch-elf.exp b/ld/testsuite/ld-loongarch-elf/ld-loongarch-elf.exp
> index b8721f0eaff..3172a0ab4b5 100644
> --- a/ld/testsuite/ld-loongarch-elf/ld-loongarch-elf.exp
> +++ b/ld/testsuite/ld-loongarch-elf/ld-loongarch-elf.exp
> @@ -141,6 +141,7 @@ if [istarget "loongarch64-*-*"] {
>       run_dump_test "reloc_abs_with_shared"
>       run_dump_test "r_larch_32_elf64"
>       run_dump_test "ifunc-reloc"
> +    run_dump_test "protected-func"
>     }
>   
>     if [check_pie_support] {
> diff --git a/ld/testsuite/ld-loongarch-elf/protected-func.d b/ld/testsuite/ld-loongarch-elf/protected-func.d
> new file mode 100644
> index 00000000000..501c7cb5f8c
> --- /dev/null
> +++ b/ld/testsuite/ld-loongarch-elf/protected-func.d
> @@ -0,0 +1,6 @@
> +#ld: -shared
> +#readelf: -Wr
> +
> +#...
> +.* R_LARCH_RELATIVE .*
> +.* R_LARCH_RELATIVE .*
> diff --git a/ld/testsuite/ld-loongarch-elf/protected-func.s b/ld/testsuite/ld-loongarch-elf/protected-func.s
> new file mode 100644
> index 00000000000..347e371a777
> --- /dev/null
> +++ b/ld/testsuite/ld-loongarch-elf/protected-func.s
> @@ -0,0 +1,17 @@
> +# protected function should be unpreemptable and relocated with
> +# R_LARCH_RELATIVE in shared library, for both GOT and pointer data
> +
> +.globl x
> +.protected x
> +.type x, @function
> +x:
> +  ret
> +
> +.globl _start
> +_start:
> +  la.got $a0, x
> +  ret
> +
> +.data
> +p:
> +  .quad x
  
mengqinggang June 25, 2024, 6:38 a.m. UTC | #3
在 2024/6/22 下午6:03, Xi Ruoyao 写道:
> On LoongArch there is no reason to treat STV_PROTECTED STT_FUNC symbols
> as preemptable.  See the comment above LARCH_REF_LOCAL for detailed
> explanation.
>
> Signed-off-by: Xi Ruoyao <xry111@xry111.site>
> ---
>   bfd/elfnn-loongarch.c                         | 78 ++++++++++++++-----
>   ld/testsuite/ld-loongarch-elf/ifunc-reloc.d   |  2 +-
>   .../ld-loongarch-elf/ld-loongarch-elf.exp     |  1 +
>   .../ld-loongarch-elf/protected-func.d         |  6 ++
>   .../ld-loongarch-elf/protected-func.s         | 17 ++++
>   5 files changed, 83 insertions(+), 21 deletions(-)
>   create mode 100644 ld/testsuite/ld-loongarch-elf/protected-func.d
>   create mode 100644 ld/testsuite/ld-loongarch-elf/protected-func.s
>
> diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
> index e0b3e9f5ee6..2acb5c7fa95 100644
> --- a/bfd/elfnn-loongarch.c
> +++ b/bfd/elfnn-loongarch.c
> @@ -181,6 +181,46 @@ struct loongarch_elf_link_hash_table
>       } \
>       while (0)
>   
> +/* TL;DR always use it in this file instead when you want to type
> +   SYMBOL_REFERENCES_LOCAL.
> +
> +   It's like SYMBOL_REFERENCES_LOCAL, but it returns true for local
> +   protected functions.  It happens to be same as SYMBOL_CALLS_LOCAL but
> +   let's not reuse SYMBOL_CALLS_LOCAL or "CALLS" may puzzle people.
> +
> +   We do generate the so-called "canonical PLT entry" when someone attempts
> +   to la.pcrel an external function.  But this is only an unwanted side-
> +   effect from using R_LARCH_PCALA_{HI20,LO12} for medium and extreme
> +   code model function call: we have the same problem as i386 where
> +   R_386_PC32 is used for both call and lea (for medium code model new code
> +   should use R_LARCH_CALL36 instead), but unlike i386 we never really
> +   implemented "R_LARCH_COPY" thus attempting to la.pcrel an external
> +   symbol is always considered a programming error unless it's a part of
> +   a extreme code model function call, and pointer equality will be broken
> +   even with a STV_DEFAULT function:
> +
> +   $ cat t.c
> +   #include <assert.h>
> +   void check(void *p) {assert(p == check);}
> +   $ cat main.c
> +   extern void check(void *);
> +   int main(void) { check(check); }
> +   $ cc t.c -fPIC -shared -o t.so
> +   $ cc main.c -mdirect-extern-access t.so -Wl,-rpath=.
> +   $ ./a.out
> +   a.out: t.c:2: check: Assertion `p == check' failed.
> +   Aborted


I have some questions about this example.

This example fail in PIE and success in no-PIE whether has this patch or 
not.


> +
> +   Thus handling STV_PROTECTED function specially just fixes nothing:
> +   adding -fvisibility=protected compiling t.c will not magically fix
> +   the inequality.  The only possible and correct fix is not to use
> +   -mdirect-extern-access.
> +
> +   So we should remove this special handling, because it's only an
> +   unsuccessful workaround for invalid code and it's penalizing valid
> +   code.  */
> +#define LARCH_REF_LOCAL(info, h) \
> +  (_bfd_elf_symbol_refs_local_p ((h), (info), true))
>   
>   /* Generate a PLT header.  */
>   
> @@ -712,7 +752,7 @@ loongarch_tls_transition_without_check (struct bfd_link_info *info,
>   					struct elf_link_hash_entry *h)
>   {
>     bool local_exec = bfd_link_executable (info)
> -		    && SYMBOL_REFERENCES_LOCAL (info, h);
> +		    && LARCH_REF_LOCAL (info, h);
>   
>     switch (r_type)
>       {
> @@ -1196,7 +1236,7 @@ loongarch_elf_adjust_dynamic_symbol (struct bfd_link_info *info,
>       {
>         if (h->plt.refcount <= 0
>   	  || (h->type != STT_GNU_IFUNC
> -	      && (SYMBOL_REFERENCES_LOCAL (info, h)
> +	      && (LARCH_REF_LOCAL (info, h)
>   		  || (ELF_ST_VISIBILITY (h->other) != STV_DEFAULT
>   		      && h->root.type == bfd_link_hash_undefweak))))
>   	{
> @@ -1714,14 +1754,14 @@ elfNN_allocate_ifunc_dynrelocs (struct elf_link_hash_entry *h,
>        here if it is defined and referenced in a non-shared object.  */
>     if (h->type == STT_GNU_IFUNC && h->def_regular)
>       {
> -      if (ref_local && SYMBOL_REFERENCES_LOCAL (info, h))
> +      if (ref_local && LARCH_REF_LOCAL (info, h))
>   	return local_allocate_ifunc_dyn_relocs (info, h,
>   						&h->dyn_relocs,
>   						PLT_ENTRY_SIZE,
>   						PLT_HEADER_SIZE,
>   						GOT_ENTRY_SIZE,
>   						false);
> -      else if (!ref_local && !SYMBOL_REFERENCES_LOCAL (info, h))
> +      else if (!ref_local && !LARCH_REF_LOCAL (info, h))
>   	return _bfd_elf_allocate_ifunc_dyn_relocs (info, h,
>   						   &h->dyn_relocs,
>   						   PLT_ENTRY_SIZE,
> @@ -1749,7 +1789,6 @@ elfNN_allocate_ifunc_dynrelocs_ref_global (struct elf_link_hash_entry *h,
>   					 false);
>   }
>   
> -
>   /* Allocate space in .plt, .got and associated reloc sections for
>      ifunc dynamic relocs.  */
>   
> @@ -2662,7 +2701,6 @@ tlsoff (struct bfd_link_info *info, bfd_vma addr)
>     return addr - elf_hash_table (info)->tls_sec->vma;
>   }
>   
> -
>   static int
>   loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   				bfd *input_bfd, asection *input_section,
> @@ -2788,7 +2826,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   	    {
>   	      defined_local = !unresolved_reloc && !ignored;
>   	      resolved_local =
> -		defined_local && SYMBOL_REFERENCES_LOCAL (info, h);
> +		defined_local && LARCH_REF_LOCAL (info, h);
>   	      resolved_dynly = !resolved_local;
>   	      resolved_to_const = !resolved_local && !resolved_dynly;
>   	    }
> @@ -2877,7 +2915,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   		      outrel.r_addend = 0;
>   		    }
>   
> -		  if (SYMBOL_REFERENCES_LOCAL (info, h))
> +		  if (LARCH_REF_LOCAL (info, h))
>   		    {
>   
>   		      if (htab->elf.splt != NULL)
> @@ -3246,7 +3284,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   		  if (!WILL_CALL_FINISH_DYNAMIC_SYMBOL (is_dyn,
>   							bfd_link_pic (info), h)
>   		      && ((bfd_link_pic (info)
> -			   && SYMBOL_REFERENCES_LOCAL (info, h))))
> +			   && LARCH_REF_LOCAL (info, h))))
>   		    {
>   		      /* This is actually a static link, or it is a
>   			 -Bsymbolic link and the symbol is defined
> @@ -3391,7 +3429,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   		asection *srel = htab->elf.srelgot;
>   		bfd_vma tls_block_off = 0;
>   
> -		if (SYMBOL_REFERENCES_LOCAL (info, h))
> +		if (LARCH_REF_LOCAL (info, h))
>   		  {
>   		    BFD_ASSERT (elf_hash_table (info)->tls_sec);
>   		    tls_block_off = relocation
> @@ -3402,7 +3440,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   		  {
>   		    rela.r_offset = sec_addr (got) + got_off;
>   		    rela.r_addend = 0;
> -		    if (SYMBOL_REFERENCES_LOCAL (info, h))
> +		    if (LARCH_REF_LOCAL (info, h))
>   		      {
>   			/* Local sym, used in exec, set module id 1.  */
>   			if (bfd_link_executable (info))
> @@ -3435,7 +3473,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   		if (tls_type & GOT_TLS_IE)
>   		  {
>   		    rela.r_offset = sec_addr (got) + got_off + ie_off;
> -		    if (SYMBOL_REFERENCES_LOCAL (info, h))
> +		    if (LARCH_REF_LOCAL (info, h))
>   		      {
>   			/* Local sym, used in exec, set module id 1.  */
>   			if (!bfd_link_executable (info))
> @@ -3637,7 +3675,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   							    bfd_link_pic (info),
>   							    h)
>   			  && bfd_link_pic (info)
> -			  && SYMBOL_REFERENCES_LOCAL (info, h))
> +			  && LARCH_REF_LOCAL (info, h))
>   			{
>   			  Elf_Internal_Rela rela;
>   			  rela.r_offset = sec_addr (got) + got_off;
> @@ -4178,7 +4216,7 @@ loongarch_tls_perform_trans (bfd *abfd, asection *sec,
>   {
>     unsigned long insn;
>     bool local_exec = bfd_link_executable (info)
> -		      && SYMBOL_REFERENCES_LOCAL (info, h);
> +		      && LARCH_REF_LOCAL (info, h);
>     bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;
>     unsigned long r_type = ELFNN_R_TYPE (rel->r_info);
>     unsigned long r_symndx = ELFNN_R_SYM (rel->r_info);
> @@ -4890,7 +4928,7 @@ loongarch_elf_relax_section (bfd *abfd, asection *sec,
>   	  else
>   	    continue;
>   
> -	  if (h && SYMBOL_REFERENCES_LOCAL (info, h))
> +	  if (h && LARCH_REF_LOCAL (info, h))
>   	    local_got = true;
>   	  symtype = h->type;
>   	}
> @@ -5027,12 +5065,12 @@ loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
>         if (htab->elf.splt)
>   	{
>   	  BFD_ASSERT ((h->type == STT_GNU_IFUNC
> -		       && SYMBOL_REFERENCES_LOCAL (info, h))
> +		       && LARCH_REF_LOCAL (info, h))
>   		      || h->dynindx != -1);
>   
>   	  plt = htab->elf.splt;
>   	  gotplt = htab->elf.sgotplt;
> -	  if (h->type == STT_GNU_IFUNC && SYMBOL_REFERENCES_LOCAL (info, h))
> +	  if (h->type == STT_GNU_IFUNC && LARCH_REF_LOCAL (info, h))
>   	    relplt = htab->elf.srelgot;
>   	  else
>   	    relplt = htab->elf.srelplt;
> @@ -5043,7 +5081,7 @@ loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
>         else /* if (htab->elf.iplt) */
>   	{
>   	  BFD_ASSERT (h->type == STT_GNU_IFUNC
> -		      && SYMBOL_REFERENCES_LOCAL (info, h));
> +		      && LARCH_REF_LOCAL (info, h));
>   
>   	  plt = htab->elf.iplt;
>   	  gotplt = htab->elf.igotplt;
> @@ -5131,7 +5169,7 @@ loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
>   	      if (htab->elf.splt == NULL)
>   		srela = htab->elf.irelplt;
>   
> -	      if (SYMBOL_REFERENCES_LOCAL (info, h))
> +	      if (LARCH_REF_LOCAL (info, h))
>   		{
>   		  asection *sec = h->root.u.def.section;
>   		  rela.r_info = ELFNN_R_INFO (0, R_LARCH_IRELATIVE);
> @@ -5168,7 +5206,7 @@ loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
>   	      return true;
>   	    }
>   	}
> -      else if (bfd_link_pic (info) && SYMBOL_REFERENCES_LOCAL (info, h))
> +      else if (bfd_link_pic (info) && LARCH_REF_LOCAL (info, h))
>   	{
>   	  asection *sec = h->root.u.def.section;
>   	  rela.r_info = ELFNN_R_INFO (0, R_LARCH_RELATIVE);
> diff --git a/ld/testsuite/ld-loongarch-elf/ifunc-reloc.d b/ld/testsuite/ld-loongarch-elf/ifunc-reloc.d
> index cb592874b1e..968e7564b49 100644
> --- a/ld/testsuite/ld-loongarch-elf/ifunc-reloc.d
> +++ b/ld/testsuite/ld-loongarch-elf/ifunc-reloc.d
> @@ -8,6 +8,7 @@
>   .* R_LARCH_IRELATIVE .*
>   .* R_LARCH_IRELATIVE .*
>   .* R_LARCH_IRELATIVE .*
> +.* R_LARCH_IRELATIVE .*
>   #...
>   .*'\.rela\.plt'.*
>   #...
> @@ -16,4 +17,3 @@
>   .* R_LARCH_JUMP_SLOT .*
>   .* R_LARCH_JUMP_SLOT .*
>   .* R_LARCH_JUMP_SLOT .*
> -.* R_LARCH_JUMP_SLOT .*
> diff --git a/ld/testsuite/ld-loongarch-elf/ld-loongarch-elf.exp b/ld/testsuite/ld-loongarch-elf/ld-loongarch-elf.exp
> index b8721f0eaff..3172a0ab4b5 100644
> --- a/ld/testsuite/ld-loongarch-elf/ld-loongarch-elf.exp
> +++ b/ld/testsuite/ld-loongarch-elf/ld-loongarch-elf.exp
> @@ -141,6 +141,7 @@ if [istarget "loongarch64-*-*"] {
>       run_dump_test "reloc_abs_with_shared"
>       run_dump_test "r_larch_32_elf64"
>       run_dump_test "ifunc-reloc"
> +    run_dump_test "protected-func"
>     }
>   
>     if [check_pie_support] {
> diff --git a/ld/testsuite/ld-loongarch-elf/protected-func.d b/ld/testsuite/ld-loongarch-elf/protected-func.d
> new file mode 100644
> index 00000000000..501c7cb5f8c
> --- /dev/null
> +++ b/ld/testsuite/ld-loongarch-elf/protected-func.d
> @@ -0,0 +1,6 @@
> +#ld: -shared
> +#readelf: -Wr
> +
> +#...
> +.* R_LARCH_RELATIVE .*
> +.* R_LARCH_RELATIVE .*
> diff --git a/ld/testsuite/ld-loongarch-elf/protected-func.s b/ld/testsuite/ld-loongarch-elf/protected-func.s
> new file mode 100644
> index 00000000000..347e371a777
> --- /dev/null
> +++ b/ld/testsuite/ld-loongarch-elf/protected-func.s
> @@ -0,0 +1,17 @@
> +# protected function should be unpreemptable and relocated with
> +# R_LARCH_RELATIVE in shared library, for both GOT and pointer data
> +
> +.globl x
> +.protected x
> +.type x, @function
> +x:
> +  ret
> +
> +.globl _start
> +_start:
> +  la.got $a0, x
> +  ret
> +
> +.data
> +p:
> +  .quad x
  
Xi Ruoyao June 25, 2024, 10:15 a.m. UTC | #4
On Tue, 2024-06-25 at 10:39 +0800, mengqinggang wrote:
> > +   We do generate the so-called "canonical PLT entry" when someone attempts
> > +   to la.pcrel an external function.  But this is only an unwanted side-
> > +   effect from using R_LARCH_PCALA_{HI20,LO12} for medium and extreme
> > +   code model function call: we have the same problem as i386 where
> 
> 
> Extreme code model function call use GOT not PLT.
> 
>    10:   1a00000d        pcalau12i       $t1, 0
>                          10: R_LARCH_GOT_PC_HI20 f
>    14:   02c0000c        li.d            $t0, 0
>                          14: R_LARCH_GOT_PC_LO12 f
>    18:   1600000c        lu32i.d         $t0, 0
>                          18: R_LARCH_GOT64_PC_LO20       f
>    1c:   0300018c        lu52i.d         $t0, $t0, 0
>                          1c: R_LARCH_GOT64_PC_HI12       f
>    20:   380c31ac        ldx.d           $t0, $t1, $t0
>    24:   4c000181        jirl            $ra, $t0, 0

Yes, normally.

But on some rare cases we do extreme code model function call via PLT.
For example in Glibc startup code:

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/loongarch/start.S;hb=HEAD

Otherwise it's very difficult to make it work for both static PIE and
extreme code model.  (For static PIE GOT is not initialized at the time
when the code is executed, and other platforms supporting static PIE
seems not caring the largest code model.)
  
Xi Ruoyao June 25, 2024, 10:44 a.m. UTC | #5
On Tue, 2024-06-25 at 14:38 +0800, mengqinggang wrote:
> > +   $ cat t.c
> > +   #include <assert.h>
> > +   void check(void *p) {assert(p == check);}
> > +   $ cat main.c
> > +   extern void check(void *);
> > +   int main(void) { check(check); }
> > +   $ cc t.c -fPIC -shared -o t.so
> > +   $ cc main.c -mdirect-extern-access t.so -Wl,-rpath=.
> > +   $ ./a.out
> > +   a.out: t.c:2: check: Assertion `p == check' failed.
> > +   Aborted
> 
> 
> I have some questions about this example.
> 
> This example fail in PIE and success in no-PIE whether has this patch or 
> not.

The example just shows the difference between LoongArch and
"traditional" platforms (with copy relocation).  It's not a test case
that we should try to "fix."

With copy relocation, those traditional platforms use PCREL instead of
GOT to refer external symbols from a main executable, thus the example
is supposed to work on these platforms.

But on LoongArch there's no copy relocation, and a dynamically-linked
main executable should always use GOT to refer an external symbol
(unless it's going to call an external function).  Thus the example is
not supposed to work, at all.

So while this change is controversial for other ports (it's just
"controversial", not "impossible": ld.gold is already treating
STV_PROTECTED functions non-preemptible anyway, and GCC has stopped to
use PCREL for referring external functions on at least x86 and AArch64),
on LoongArch it should be perfectly fine: it'll only break already
broken code.

Note that on LoongArch -mdirect-extern-access is not the default and
it's clearly documented as "don't use unless you know there's no dynamic
linking."  In this case it's used with dynamic linking, thus the
produced assembly is just broken code.
  

Patch

diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
index e0b3e9f5ee6..2acb5c7fa95 100644
--- a/bfd/elfnn-loongarch.c
+++ b/bfd/elfnn-loongarch.c
@@ -181,6 +181,46 @@  struct loongarch_elf_link_hash_table
     } \
     while (0)
 
+/* TL;DR always use it in this file instead when you want to type
+   SYMBOL_REFERENCES_LOCAL.
+
+   It's like SYMBOL_REFERENCES_LOCAL, but it returns true for local
+   protected functions.  It happens to be same as SYMBOL_CALLS_LOCAL but
+   let's not reuse SYMBOL_CALLS_LOCAL or "CALLS" may puzzle people.
+
+   We do generate the so-called "canonical PLT entry" when someone attempts
+   to la.pcrel an external function.  But this is only an unwanted side-
+   effect from using R_LARCH_PCALA_{HI20,LO12} for medium and extreme
+   code model function call: we have the same problem as i386 where
+   R_386_PC32 is used for both call and lea (for medium code model new code
+   should use R_LARCH_CALL36 instead), but unlike i386 we never really
+   implemented "R_LARCH_COPY" thus attempting to la.pcrel an external
+   symbol is always considered a programming error unless it's a part of
+   a extreme code model function call, and pointer equality will be broken
+   even with a STV_DEFAULT function:
+
+   $ cat t.c
+   #include <assert.h>
+   void check(void *p) {assert(p == check);}
+   $ cat main.c
+   extern void check(void *);
+   int main(void) { check(check); }
+   $ cc t.c -fPIC -shared -o t.so
+   $ cc main.c -mdirect-extern-access t.so -Wl,-rpath=.
+   $ ./a.out
+   a.out: t.c:2: check: Assertion `p == check' failed.
+   Aborted
+
+   Thus handling STV_PROTECTED function specially just fixes nothing:
+   adding -fvisibility=protected compiling t.c will not magically fix
+   the inequality.  The only possible and correct fix is not to use
+   -mdirect-extern-access.
+
+   So we should remove this special handling, because it's only an
+   unsuccessful workaround for invalid code and it's penalizing valid
+   code.  */
+#define LARCH_REF_LOCAL(info, h) \
+  (_bfd_elf_symbol_refs_local_p ((h), (info), true))
 
 /* Generate a PLT header.  */
 
@@ -712,7 +752,7 @@  loongarch_tls_transition_without_check (struct bfd_link_info *info,
 					struct elf_link_hash_entry *h)
 {
   bool local_exec = bfd_link_executable (info)
-		    && SYMBOL_REFERENCES_LOCAL (info, h);
+		    && LARCH_REF_LOCAL (info, h);
 
   switch (r_type)
     {
@@ -1196,7 +1236,7 @@  loongarch_elf_adjust_dynamic_symbol (struct bfd_link_info *info,
     {
       if (h->plt.refcount <= 0
 	  || (h->type != STT_GNU_IFUNC
-	      && (SYMBOL_REFERENCES_LOCAL (info, h)
+	      && (LARCH_REF_LOCAL (info, h)
 		  || (ELF_ST_VISIBILITY (h->other) != STV_DEFAULT
 		      && h->root.type == bfd_link_hash_undefweak))))
 	{
@@ -1714,14 +1754,14 @@  elfNN_allocate_ifunc_dynrelocs (struct elf_link_hash_entry *h,
      here if it is defined and referenced in a non-shared object.  */
   if (h->type == STT_GNU_IFUNC && h->def_regular)
     {
-      if (ref_local && SYMBOL_REFERENCES_LOCAL (info, h))
+      if (ref_local && LARCH_REF_LOCAL (info, h))
 	return local_allocate_ifunc_dyn_relocs (info, h,
 						&h->dyn_relocs,
 						PLT_ENTRY_SIZE,
 						PLT_HEADER_SIZE,
 						GOT_ENTRY_SIZE,
 						false);
-      else if (!ref_local && !SYMBOL_REFERENCES_LOCAL (info, h))
+      else if (!ref_local && !LARCH_REF_LOCAL (info, h))
 	return _bfd_elf_allocate_ifunc_dyn_relocs (info, h,
 						   &h->dyn_relocs,
 						   PLT_ENTRY_SIZE,
@@ -1749,7 +1789,6 @@  elfNN_allocate_ifunc_dynrelocs_ref_global (struct elf_link_hash_entry *h,
 					 false);
 }
 
-
 /* Allocate space in .plt, .got and associated reloc sections for
    ifunc dynamic relocs.  */
 
@@ -2662,7 +2701,6 @@  tlsoff (struct bfd_link_info *info, bfd_vma addr)
   return addr - elf_hash_table (info)->tls_sec->vma;
 }
 
-
 static int
 loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
 				bfd *input_bfd, asection *input_section,
@@ -2788,7 +2826,7 @@  loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
 	    {
 	      defined_local = !unresolved_reloc && !ignored;
 	      resolved_local =
-		defined_local && SYMBOL_REFERENCES_LOCAL (info, h);
+		defined_local && LARCH_REF_LOCAL (info, h);
 	      resolved_dynly = !resolved_local;
 	      resolved_to_const = !resolved_local && !resolved_dynly;
 	    }
@@ -2877,7 +2915,7 @@  loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
 		      outrel.r_addend = 0;
 		    }
 
-		  if (SYMBOL_REFERENCES_LOCAL (info, h))
+		  if (LARCH_REF_LOCAL (info, h))
 		    {
 
 		      if (htab->elf.splt != NULL)
@@ -3246,7 +3284,7 @@  loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
 		  if (!WILL_CALL_FINISH_DYNAMIC_SYMBOL (is_dyn,
 							bfd_link_pic (info), h)
 		      && ((bfd_link_pic (info)
-			   && SYMBOL_REFERENCES_LOCAL (info, h))))
+			   && LARCH_REF_LOCAL (info, h))))
 		    {
 		      /* This is actually a static link, or it is a
 			 -Bsymbolic link and the symbol is defined
@@ -3391,7 +3429,7 @@  loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
 		asection *srel = htab->elf.srelgot;
 		bfd_vma tls_block_off = 0;
 
-		if (SYMBOL_REFERENCES_LOCAL (info, h))
+		if (LARCH_REF_LOCAL (info, h))
 		  {
 		    BFD_ASSERT (elf_hash_table (info)->tls_sec);
 		    tls_block_off = relocation
@@ -3402,7 +3440,7 @@  loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
 		  {
 		    rela.r_offset = sec_addr (got) + got_off;
 		    rela.r_addend = 0;
-		    if (SYMBOL_REFERENCES_LOCAL (info, h))
+		    if (LARCH_REF_LOCAL (info, h))
 		      {
 			/* Local sym, used in exec, set module id 1.  */
 			if (bfd_link_executable (info))
@@ -3435,7 +3473,7 @@  loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
 		if (tls_type & GOT_TLS_IE)
 		  {
 		    rela.r_offset = sec_addr (got) + got_off + ie_off;
-		    if (SYMBOL_REFERENCES_LOCAL (info, h))
+		    if (LARCH_REF_LOCAL (info, h))
 		      {
 			/* Local sym, used in exec, set module id 1.  */
 			if (!bfd_link_executable (info))
@@ -3637,7 +3675,7 @@  loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
 							    bfd_link_pic (info),
 							    h)
 			  && bfd_link_pic (info)
-			  && SYMBOL_REFERENCES_LOCAL (info, h))
+			  && LARCH_REF_LOCAL (info, h))
 			{
 			  Elf_Internal_Rela rela;
 			  rela.r_offset = sec_addr (got) + got_off;
@@ -4178,7 +4216,7 @@  loongarch_tls_perform_trans (bfd *abfd, asection *sec,
 {
   unsigned long insn;
   bool local_exec = bfd_link_executable (info)
-		      && SYMBOL_REFERENCES_LOCAL (info, h);
+		      && LARCH_REF_LOCAL (info, h);
   bfd_byte *contents = elf_section_data (sec)->this_hdr.contents;
   unsigned long r_type = ELFNN_R_TYPE (rel->r_info);
   unsigned long r_symndx = ELFNN_R_SYM (rel->r_info);
@@ -4890,7 +4928,7 @@  loongarch_elf_relax_section (bfd *abfd, asection *sec,
 	  else
 	    continue;
 
-	  if (h && SYMBOL_REFERENCES_LOCAL (info, h))
+	  if (h && LARCH_REF_LOCAL (info, h))
 	    local_got = true;
 	  symtype = h->type;
 	}
@@ -5027,12 +5065,12 @@  loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
       if (htab->elf.splt)
 	{
 	  BFD_ASSERT ((h->type == STT_GNU_IFUNC
-		       && SYMBOL_REFERENCES_LOCAL (info, h))
+		       && LARCH_REF_LOCAL (info, h))
 		      || h->dynindx != -1);
 
 	  plt = htab->elf.splt;
 	  gotplt = htab->elf.sgotplt;
-	  if (h->type == STT_GNU_IFUNC && SYMBOL_REFERENCES_LOCAL (info, h))
+	  if (h->type == STT_GNU_IFUNC && LARCH_REF_LOCAL (info, h))
 	    relplt = htab->elf.srelgot;
 	  else
 	    relplt = htab->elf.srelplt;
@@ -5043,7 +5081,7 @@  loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
       else /* if (htab->elf.iplt) */
 	{
 	  BFD_ASSERT (h->type == STT_GNU_IFUNC
-		      && SYMBOL_REFERENCES_LOCAL (info, h));
+		      && LARCH_REF_LOCAL (info, h));
 
 	  plt = htab->elf.iplt;
 	  gotplt = htab->elf.igotplt;
@@ -5131,7 +5169,7 @@  loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
 	      if (htab->elf.splt == NULL)
 		srela = htab->elf.irelplt;
 
-	      if (SYMBOL_REFERENCES_LOCAL (info, h))
+	      if (LARCH_REF_LOCAL (info, h))
 		{
 		  asection *sec = h->root.u.def.section;
 		  rela.r_info = ELFNN_R_INFO (0, R_LARCH_IRELATIVE);
@@ -5168,7 +5206,7 @@  loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
 	      return true;
 	    }
 	}
-      else if (bfd_link_pic (info) && SYMBOL_REFERENCES_LOCAL (info, h))
+      else if (bfd_link_pic (info) && LARCH_REF_LOCAL (info, h))
 	{
 	  asection *sec = h->root.u.def.section;
 	  rela.r_info = ELFNN_R_INFO (0, R_LARCH_RELATIVE);
diff --git a/ld/testsuite/ld-loongarch-elf/ifunc-reloc.d b/ld/testsuite/ld-loongarch-elf/ifunc-reloc.d
index cb592874b1e..968e7564b49 100644
--- a/ld/testsuite/ld-loongarch-elf/ifunc-reloc.d
+++ b/ld/testsuite/ld-loongarch-elf/ifunc-reloc.d
@@ -8,6 +8,7 @@ 
 .* R_LARCH_IRELATIVE .*
 .* R_LARCH_IRELATIVE .*
 .* R_LARCH_IRELATIVE .*
+.* R_LARCH_IRELATIVE .*
 #...
 .*'\.rela\.plt'.*
 #...
@@ -16,4 +17,3 @@ 
 .* R_LARCH_JUMP_SLOT .*
 .* R_LARCH_JUMP_SLOT .*
 .* R_LARCH_JUMP_SLOT .*
-.* R_LARCH_JUMP_SLOT .*
diff --git a/ld/testsuite/ld-loongarch-elf/ld-loongarch-elf.exp b/ld/testsuite/ld-loongarch-elf/ld-loongarch-elf.exp
index b8721f0eaff..3172a0ab4b5 100644
--- a/ld/testsuite/ld-loongarch-elf/ld-loongarch-elf.exp
+++ b/ld/testsuite/ld-loongarch-elf/ld-loongarch-elf.exp
@@ -141,6 +141,7 @@  if [istarget "loongarch64-*-*"] {
     run_dump_test "reloc_abs_with_shared"
     run_dump_test "r_larch_32_elf64"
     run_dump_test "ifunc-reloc"
+    run_dump_test "protected-func"
   }
 
   if [check_pie_support] {
diff --git a/ld/testsuite/ld-loongarch-elf/protected-func.d b/ld/testsuite/ld-loongarch-elf/protected-func.d
new file mode 100644
index 00000000000..501c7cb5f8c
--- /dev/null
+++ b/ld/testsuite/ld-loongarch-elf/protected-func.d
@@ -0,0 +1,6 @@ 
+#ld: -shared
+#readelf: -Wr
+
+#...
+.* R_LARCH_RELATIVE .*
+.* R_LARCH_RELATIVE .*
diff --git a/ld/testsuite/ld-loongarch-elf/protected-func.s b/ld/testsuite/ld-loongarch-elf/protected-func.s
new file mode 100644
index 00000000000..347e371a777
--- /dev/null
+++ b/ld/testsuite/ld-loongarch-elf/protected-func.s
@@ -0,0 +1,17 @@ 
+# protected function should be unpreemptable and relocated with
+# R_LARCH_RELATIVE in shared library, for both GOT and pointer data
+
+.globl x
+.protected x
+.type x, @function
+x:
+  ret
+
+.globl _start
+_start:
+  la.got $a0, x
+  ret
+
+.data
+p:
+  .quad x