public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
From: Fangrui Song <maskray@google.com>
To: Alan Modra <amodra@gmail.com>, Joseph Myers <josmyers@redhat.com>,
	 Alexandre Oliva <oliva@adacore.com>,
	binutils@sourceware.org
Cc: Fangrui Song <maskray@gcc.gnu.org>
Subject: [PATCH v2] sh: Make protected symbols local for FDPIC -shared
Date: Mon,  4 Mar 2024 16:43:39 -0800	[thread overview]
Message-ID: <20240305004339.1353851-1-maskray@google.com> (raw)

From: Fangrui Song <maskray@gcc.gnu.org>

    __attribute__((visibility("protected"))) void protected_fun() {}
    void *get_protected_fun() { return protected_fun; }

links fine in the non-FDPIC mode but not in the FDPIC mode:

    R_SH_GOTOFFFUNCDESC relocation against external symbol "protected_fun"

The issue is similar to the x86 issue fixed by commit
d19a265487eda186b6977d9d15648cda9fad3298 ("x86: Make protected symbols
local for -shared").

SYMBOL_FUNCDESC_LOCAL in the R_SH_GOTOFFFUNCDESC check unnecessarily
considers protected symbols non-local.  elf32-frv.c:FRVFDPIC_SYM_LOCAL
avoids the issue by setting local_protected.  Set local_protected for sh
as well.

Technically, the condition `! elf_hash_table
(INFO)->dynamic_sections_created` does not belong to the meaning implied
by SYMBOL_FUNCDESC_LOCAL, but a few `!bfd_link_pic (info) &&
SYMBOL_FUNCDESC_LOCAL (info, h)` uses expect that SYMBOL_FUNCDESC_LOCAL
returns true when the dynamic section is not created.

In addition, remove the `SYMBOL_CALLS_LOCAL (info, h) && !
SYMBOL_FUNCDESC_LOCAL (info, h)` code path, which is now dead.
Nowadays, we should try setting `local_protected` to 1 everywhere.
---
changes from v1:
* keep dynamic_sections_created since it is needed. it could be removed,
  which would require extensive cleanup, so I decide not to do it.
* use SYMBOL_CALLS_LOCAL and remove a dead code block as Alan suggested.
---
 bfd/elf32-sh.c                                | 22 ++++---------------
 .../ld-sh/fdpic-gotofffuncdesc-protected.d    |  6 +++++
 .../ld-sh/fdpic-gotofffuncdesc-protected.s    | 13 +++++++++++
 3 files changed, 23 insertions(+), 18 deletions(-)
 create mode 100644 ld/testsuite/ld-sh/fdpic-gotofffuncdesc-protected.d
 create mode 100644 ld/testsuite/ld-sh/fdpic-gotofffuncdesc-protected.s

diff --git a/bfd/elf32-sh.c b/bfd/elf32-sh.c
index bf4cb2a8242..d25b2b5808a 100644
--- a/bfd/elf32-sh.c
+++ b/bfd/elf32-sh.c
@@ -63,10 +63,10 @@ static bfd_vma tpoff
 /* Decide whether a reference to a symbol can be resolved locally or
    not.  If the symbol is protected, we want the local address, but
    its function descriptor must be assigned by the dynamic linker.  */
-#define SYMBOL_FUNCDESC_LOCAL(INFO, H) \
-  (SYMBOL_REFERENCES_LOCAL (INFO, H) \
-   || ! elf_hash_table (INFO)->dynamic_sections_created)
-\f
+#define SYMBOL_FUNCDESC_LOCAL(INFO, H)                                        \
+  (SYMBOL_CALLS_LOCAL (INFO, H)                                               \
+   || !elf_hash_table (INFO)->dynamic_sections_created)
+
 #define SH_PARTIAL32 true
 #define SH_SRC_MASK32 0xffffffff
 #define SH_ELF_RELOC sh_elf_reloc
@@ -4279,20 +4279,6 @@ sh_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
 	      /* Undefined weak symbol which will not be dynamically
 		 resolved later; leave it at zero.  */
 	      goto funcdesc_leave_zero;
-	    else if (SYMBOL_CALLS_LOCAL (info, h)
-		     && ! SYMBOL_FUNCDESC_LOCAL (info, h))
-	      {
-		/* If the symbol needs a non-local function descriptor
-		   but binds locally (i.e., its visibility is
-		   protected), emit a dynamic relocation decayed to
-		   section+offset.  This is an optimization; the dynamic
-		   linker would resolve our function descriptor request
-		   to our copy of the function anyway.  */
-		dynindx = elf_section_data (h->root.u.def.section
-					    ->output_section)->dynindx;
-		relocation += h->root.u.def.section->output_offset
-		  + h->root.u.def.value;
-	      }
 	    else if (! SYMBOL_FUNCDESC_LOCAL (info, h))
 	      {
 		/* If the symbol is dynamic and there will be dynamic
diff --git a/ld/testsuite/ld-sh/fdpic-gotofffuncdesc-protected.d b/ld/testsuite/ld-sh/fdpic-gotofffuncdesc-protected.d
new file mode 100644
index 00000000000..a8d0803cccb
--- /dev/null
+++ b/ld/testsuite/ld-sh/fdpic-gotofffuncdesc-protected.d
@@ -0,0 +1,6 @@
+#as: --isa=sh4 --fdpic
+#ld: -EL -mshlelf_fd -shared
+#readelf: -sW
+#...
+ +[0-9]+: +[0-9a-f]+ +[0-9a-f]+ +FUNC +GLOBAL +PROTECTED +[0-9]+ +foo
+#pass
diff --git a/ld/testsuite/ld-sh/fdpic-gotofffuncdesc-protected.s b/ld/testsuite/ld-sh/fdpic-gotofffuncdesc-protected.s
new file mode 100644
index 00000000000..14463149b07
--- /dev/null
+++ b/ld/testsuite/ld-sh/fdpic-gotofffuncdesc-protected.s
@@ -0,0 +1,13 @@
+	.text
+	.globl	f
+	.type	f,@function
+f:
+	mov.l .L1, r1
+	add r12, r1
+	.align 2
+.L1:
+	.long	foo@GOTOFFFUNCDESC
+	.globl foo
+	.protected foo
+	.type	foo,@function
+foo:
-- 
2.44.0.rc1.240.g4c46232300-goog


             reply	other threads:[~2024-03-05  0:43 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-05  0:43 Fangrui Song [this message]
2024-03-06 20:12 ` Alexandre Oliva
2024-03-07  8:32   ` Fangrui Song
2024-03-09  5:07     ` Alexandre Oliva

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240305004339.1353851-1-maskray@google.com \
    --to=maskray@google.com \
    --cc=amodra@gmail.com \
    --cc=binutils@sourceware.org \
    --cc=josmyers@redhat.com \
    --cc=maskray@gcc.gnu.org \
    --cc=oliva@adacore.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).