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

Message ID 20240630071825.11172-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 30, 2024, 7:18 a.m. UTC
  On LoongArch there is no reason to treat STV_PROTECTED STT_FUNC symbols
as preemptible.  See the comment above LARCH_REF_LOCAL for detailed
explanation.

Signed-off-by: Xi Ruoyao <xry111@xry111.site>
---
 bfd/elfnn-loongarch.c                         | 76 ++++++++++++++-----
 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, 81 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
  

Patch

diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
index bcb144357ff..fa8ed1feb29 100644
--- a/bfd/elfnn-loongarch.c
+++ b/bfd/elfnn-loongarch.c
@@ -181,6 +181,44 @@  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 a PLT entry when someone attempts to la.pcrel an external
+   function.  But we never really implemented "R_LARCH_COPY", thus we've
+   never supported la.pcrel an external symbol unless the loaded address is
+   only used for locating a function to be called.  Thus the PLT entry is
+   a normal PLT entry, not intended to be a so-called "canonical PLT entry"
+   on the ports supporting copy relocation.  So attempting to la.pcrel an
+   external function will just break pointer equality, even it's 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=. -fpie -pie
+   $ ./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 +750,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)
     {
@@ -1221,7 +1259,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))))
 	{
@@ -1739,14 +1777,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,
@@ -1774,7 +1812,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.  */
 
@@ -2687,7 +2724,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,
@@ -2813,7 +2849,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;
 	    }
@@ -2902,7 +2938,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)
@@ -3252,7 +3288,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
@@ -3397,7 +3433,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
@@ -3408,7 +3444,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))
@@ -3441,7 +3477,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))
@@ -3643,7 +3679,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;
@@ -4184,7 +4220,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);
@@ -4896,7 +4932,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;
 	}
@@ -5033,12 +5069,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;
@@ -5049,7 +5085,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;
@@ -5137,7 +5173,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);
@@ -5174,7 +5210,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 6cff117e29b..9a668fb7164 100644
--- a/ld/testsuite/ld-loongarch-elf/ld-loongarch-elf.exp
+++ b/ld/testsuite/ld-loongarch-elf/ld-loongarch-elf.exp
@@ -134,6 +134,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..8f28f9254a4
--- /dev/null
+++ b/ld/testsuite/ld-loongarch-elf/protected-func.s
@@ -0,0 +1,17 @@ 
+# protected function should be non-preemptible 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