Re: [musl] [RFC v2 0/2] xtensa FDPIC port

[Rich Felker <dalias@libc.org>]

[-- Attachment #1: Type: text/plain, Size: 2660 bytes --]

On Wed, Apr 03, 2024 at 04:55:56PM -0400, Rich Felker wrote:
> On Tue, Apr 02, 2024 at 07:30:57PM -0700, Max Filippov wrote:
> > On Thu, Mar 28, 2024 at 6:48 PM Rich Felker <dalias@libc.org> wrote:
> > > On Thu, Mar 28, 2024 at 05:48:50PM -0700, Max Filippov wrote:
> > > > On Thu, Mar 28, 2024 at 4:00 PM Rich Felker <dalias@libc.org> wrote:
> > > > > On Thu, Mar 28, 2024 at 01:03:17PM -0700, Max Filippov wrote:
> > > > > > functional/dlopen fails with the
> > > > > >   src/functional/dlopen.c:39: dlsym main failed: (null)
> > > > > > There's no failure in the dlsym call, but the pointers don't match.
> > > > >
> > > > > Is this something related to canonical function descriptors? Is it
> > > > > musl's fault or a bug in the tooling? I suspect the latter.
> > > >
> > > > Yes, dlsym() returns the pointer into def.dso->funcdescs,
> > > > but (void *)main returns the pointer to the canonical function
> > > > descriptor. I understand that the linker must use the
> > > > R_XTENSA_FUNCDESC relocation for the locally defined
> > > > global symbol instead of the .rofixup entries.
> > >
> > > If the xtensa FDPIC ABI is going to be that the linker makes canonical
> > > function descriptors, I think that's workable, but the dynamic linker
> > > would need a way to find and usee them. I'm not sure how that would
> > > work.
> > >
> > > The simple (but probably less efficient) way is to copy what SH did
> > > and have the dynamic linker always be responsible for them (load
> > > descriptor address from GOT).
> > 
> > I've built and tested SH FDPIC toolchain, it fails this test in exactly
> > the same way: pointer loaded directly does not match the pointer
> > returned by dlsym().
> 
> Yes, I've been able to reproduce this and it's a clear bug. There does
> not seem to be any way the dynamic linker could find these GOTFUNCDESC
> slots to use them as a canonical address for the function, and
> moreover, they're not even unique; there would be one per library.
> 
> The code path for legitimize_pic_address in sh.c that emits
> GOTFUNCDESC has the wrong logic. A simple fix would be just making
> that path never be taken, but I'm not sure if that would break use of
> GOTFUNCDESC for pure-call purposes.
> 
> The condition should probably be something like: if it's just used for
> a call (if this is even needed; pure call is probably handled
> elsewhere) or if the function is static or hidden, use GOTFUNCDESC;
> otherwise, use GOT.
> 
> I might try patching it and see what happens.

Attached patch seems to fix it. I'm not sure if this is the most
idiomatic way to write the predicate in gcc sources, but it should be
correct.

Rich

[-- Attachment #2: fdpic-patch.diff --]
[-- Type: text/plain, Size: 1008 bytes --]

--- gcc-11.4.0/gcc/config/sh/sh.c.orig	2024-04-04 05:52:42.125373614 +0900
+++ gcc-11.4.0/gcc/config/sh/sh.c	2024-04-04 06:22:11.737423661 +0900
@@ -9143,7 +9143,8 @@
 	reg = gen_reg_rtx (Pmode);
 
       if (TARGET_FDPIC
-	  && GET_CODE (orig) == SYMBOL_REF && SYMBOL_REF_FUNCTION_P (orig))
+	  && GET_CODE (orig) == SYMBOL_REF && SYMBOL_REF_FUNCTION_P (orig)
+	  && (!TREE_PUBLIC(SYMBOL_REF_DECL(orig)) || DECL_VISIBILITY (SYMBOL_REF_DECL(orig)) == VISIBILITY_HIDDEN))
 	{
 	  /* Weak functions may be NULL which doesn't work with
 	     GOTOFFFUNCDESC because the runtime offset is not known.  */
@@ -9169,7 +9170,8 @@
       if (reg == NULL_RTX)
 	reg = gen_reg_rtx (Pmode);
 
-      if (TARGET_FDPIC && SYMBOL_REF_FUNCTION_P (orig))
+      if (TARGET_FDPIC && SYMBOL_REF_FUNCTION_P (orig)
+	  && (!TREE_PUBLIC(SYMBOL_REF_DECL(orig)) || DECL_VISIBILITY (SYMBOL_REF_DECL(orig)) == VISIBILITY_HIDDEN))
 	emit_insn (gen_symGOTFUNCDESC2reg (reg, orig));
       else
 	emit_insn (gen_symGOT2reg (reg, orig));