[PATCH v2] sh: Make protected symbols local for FDPIC -shared

[PATCH v2] sh: Make protected symbols local for FDPIC -shared

[Fangrui Song]

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

Re: [PATCH v2] sh: Make protected symbols local for FDPIC -shared

[Alexandre Oliva]

On Mar  4, 2024, Fangrui Song <maskray@google.com> wrote:

>     __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"

This appears to be in line with the comment over SYMBOL_FUNCDESC_LOCAL:

>  /* 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.  */

If the comment is wrong as your proposed change suggests, it ought to be
fixed along with the patch that changes this behavior.


But I'd also consider the possibility that it is wrong for the compiler
to expect a GOTOFF-accessible FD, when the FD is only expected to be
created at run time by the DL.

A protected symbol is visible by name outside its loadable object, so
other objects may refer to it and get a dynamic linker-created function
descriptor that needs to be the canonical one for that symbol.  Which
implies that the relocation to the function descriptor cannot be at a
link-time constant GOT offset.

OTOH, it might seem to make sense to create a local noncanonical FD for
the locally-bound function, to be used only to call the function rather
than as the function address, like a non-canonical PLT entry, but then,
if it's only for local calls, we don't need a FD for calls, we could
just call the function directly.


But the quoted C testcase above makes it clear that what is expected is
indeed the canonical function descriptor, so I think this patch is
papering over a bug in the compiler, that should be generating code to
load the canonical FD address from the GOT rather than assuming the
canonical FD is local to the module just because the symbol binds
locally.


Am I making any sense?

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: [PATCH v2] sh: Make protected symbols local for FDPIC -shared

[Fangrui Song]

On 2024-03-06, Alexandre Oliva wrote:
>On Mar  4, 2024, Fangrui Song <maskray@google.com> wrote:
>
>>     __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"
>
>This appears to be in line with the comment over SYMBOL_FUNCDESC_LOCAL:
>
>>  /* 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.  */
>
>If the comment is wrong as your proposed change suggests, it ought to be
>fixed along with the patch that changes this behavior.

The comment looks correct to me, so I did not update it.
However, I may be confused by the meaning of "local address"... See
below.

>But I'd also consider the possibility that it is wrong for the compiler
>to expect a GOTOFF-accessible FD, when the FD is only expected to be
>created at run time by the DL.
>
>A protected symbol is visible by name outside its loadable object, so
>other objects may refer to it and get a dynamic linker-created function
>descriptor that needs to be the canonical one for that symbol.  Which
>implies that the relocation to the function descriptor cannot be at a
>link-time constant GOT offset.
>
>OTOH, it might seem to make sense to create a local noncanonical FD for
>the locally-bound function, to be used only to call the function rather
>than as the function address, like a non-canonical PLT entry, but then,
>if it's only for local calls, we don't need a FD for calls, we could
>just call the function directly.
>
>
>But the quoted C testcase above makes it clear that what is expected is
>indeed the canonical function descriptor, so I think this patch is
>papering over a bug in the compiler, that should be generating code to
>load the canonical FD address from the GOT rather than assuming the
>canonical FD is local to the module just because the symbol binds
>locally.
>
>
>Am I making any sense?

I have some notes about the protected visibility at
https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected#stv_protected
There were some old x86 discussions around GCC 5 and GNU ld 2.26. The
gist is that people use protected visibility for performance. The use
case is typically about -fvisibility=protected that applies to
definitions, serving as a superior alternative to -Wl,-Bsymbolic. If
taking the address of a protected visibility symbol uses GOT-indirect
addressing in -fpic mode (GCC flag_shlib), as slow as the default
visibility, then there would be no point to use the protected visibility
in the first place. (Therefore, I don't know what purpose
`local_protected==0` serves. I think all ports should just use 1.)

There have been some debates what to do when copy relocations/canonical
PLT entries are used together with protected symbol. gold, lld, and
certain BFD ports' interpretation is that copy relocations are simply
incompatible with protected visibility. Thankfully, FDPIC codegen
just forces PIC and avoids copy relocations.

I think __attribute__((visibility("protected"))) is for a definition
within a component, not for a cross-DSO API. Let's say a.so defines
protected `foo`, which is referenced by b.so using the default
visibility symbol. The references within a.so use GOTOFF relocations,
which are resolved to the canonical function descriptor. The references
from b.so use GOT relocations, which will be resolved by the dynamic loader
to a.so's canonical function descriptor. Everything should just work.
If b.so tries a protected undefined symbol, it will lead to a linker
error.

     ld.bfd: a.out: protected symbol `foo' isn't defined
     ld.lld: error: undefined protected symbol: foo

Re: [PATCH v2] sh: Make protected symbols local for FDPIC -shared

[Alexandre Oliva]

On Mar  7, 2024, Fangrui Song <maskray@google.com> wrote:

>>> /* 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.  */
>> 
>> If the comment is wrong as your proposed change suggests, it ought to be
>> fixed along with the patch that changes this behavior.

> The comment looks correct to me, so I did not update it.

The point was about having the descriptor assigned by the dynamic
linker.  You seem to be arguing against it to enable GOTOFF references
to the descriptor.  You can only use GOTOFF for descriptors that are
local to the module, I hope you'll agree.

I understand the motivations for performance, but ISTM that twisting the
meaning of protected to satisfy those motivations is not a desirable
path to take; hidden aliases seem to satisfy that goal without deviating
from any standard behaviors.

Now, I'm not aware of any special provisions in SH FDPIC specs that
would justify this change, so I'd rather stick to the overall standard.

> Let's say a.so defines protected `foo`, which is referenced by b.so
> using the default visibility symbol.

That is unclear to me.  Are we talking about two different symbols, each
with different visibility, or about two different views of the same
global symbol?

> The references within a.so use GOTOFF relocations, which are resolved
> to the canonical function descriptor.

That can't be.  The comments in place (quoted above) state that it is
the dynamic linker that assigns the descriptor for the symbol, so you
can't possibly refer to it with GOTOFF, even if the symbol otherwise
binds locally.


Now, one could conceive of using a local descriptor as the canonical
one.  FDPIC has never had provisions for that AFAIK: the dynamic loader
has no visibility into local descriptors or their associations to local
symbols.  IMHO some means to enable that would have to be added before
this kind of change could be considered.

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive