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

[Rich Felker <dalias@libc.org>]

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

On Thu, Apr 04, 2024 at 11:00:54AM -0400, Rich Felker wrote:
> On Thu, Apr 04, 2024 at 10:01:35AM -0400, Rich Felker wrote:
> > On Thu, Apr 04, 2024 at 01:44:13AM -0700, Max Filippov wrote:
> > > On Wed, Apr 3, 2024 at 2:45 PM Rich Felker <dalias@libc.org> wrote:
> > > >
> > > > 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.
> > > 
> > > It's not what I observe. On my side it doesn't change the result of the
> > > dlopen test, and it also breaks building of all statically-linked tests.
> > > 
> > > There are no relocations against the symbol 'main' neither in the test
> > > built with the original gcc nor in the test built with the patched one.
> > > dlopen.o built with the original gcc had R_SH_GOTOFFFUNCDESC
> > > relocation against the symbol 'main', dlopen.o built with the patched
> > > gcc has R_SH_GOTOFF instead. The code generated with the patched
> > > gcc:
> > > 
> > >        if (dlsym(g, "main") != (void*)main) {
> > > 28c:   50 d1           mov.l   3d0 <main+0x3d0>,r1     ! 0 <main>
> > > 28e:   8c 31           add     r8,r1
> > > 290:   12 61           mov.l   @r1,r1
> > > 292:   13 62           mov     r1,r2
> > > 294:   8f 91           mov.w   3b6 <main+0x3b6>,r1     ! 1e0
> > > 296:   ec 31           add     r14,r1
> > > 298:   23 65           mov     r2,r5
> > > 29a:   1c 54           mov.l   @(48,r1),r4
> > > 29c:   83 6c           mov     r8,r12
> > > 29e:   4d d6           mov.l   3d4 <main+0x3d4>,r6     ! 130
> > > 2a0:   03 06           bsrf    r6
> > > 2a2:   09 00           nop
> > > 2a4:   03 61           mov     r0,r1
> > > 2a6:   4c d2           mov.l   3d8 <main+0x3d8>,r2     ! 0 <main>
> > > 2a8:   8c 32           add     r8,r2
> > > 2aa:   20 31           cmp/eq  r2,r1
> > > 2ac:   27 89           bt      2fe <main+0x2fe>
> > > ....
> > > 3d8:   00 00           .word 0x0000
> > >                        3d8: R_SH_GOTOFF        main
> > > 
> > > doesn't look right to me at all. Using R_SH_GOTOFF
> > > for the symbol in text doesn't make sense. Using R_SH_GOT
> > > (AFAIU that's what you meant it to be) doesn't make sense
> > > to me as well, as the value stored in the GOT would be the
> > > address of the main() entry point, not of its descriptor.
> > > 
> > > I believe that gcc need to generate R_SH_GOTFUNCDESC
> > > instead of R_SH_GOTOFFFUNCDESC for this test to work
> > > correctly, and that the linker need to put R_SH_FUNCDESC
> > > relocation against that GOT entry, so that the dynamic linker
> > > could put there the address of the function descriptor associated
> > > with the symbol.
> > 
> > Indeed I messed this up. Ignore that patch. You're right that use of
> > R_SH_GOTOFFFUNCDESC is wrong -- it's "Used for the FDPIC-relative
> > offset to the function descriptor itself", which is never a constant
> > except for static/hidden functions or when static linking.
> > 
> > However, you can test switching it to R_SH_GOTFUNCDESC just by using
> > -fPIC, and when you link, you still get the same problem, because the
> > linker is breaking it by binding at ld-time. So there are at least two
> > bugs in play here.
> 
> The offending logic in bfd linker is SYMBOL_FUNCDESC_LOCAL macro in
> bfd/elf32-sh.c. SYMBOL_REFERENCES_LOCAL is not sufficient to conclude
> that the FUNCDESC can be expanded at ld time. I'm not sure how to
> write it yet, but the condition is:
> 
>     !dynamic_linking || (SYMBOL_REFERENCES_LOCAL && !visible_outside_dso)
> 
> The existing elf_hash_table (INFO)->dynamic_sections_created clause
> covers !dynamic_linking mostly except that it will give the wrong
> result for static PIE (which is largely gratuitous for FDPIC, but
> should still be expected to work).

I have what seem to be working patches for the linker and gcc.

The bfd link code for sh/fdpic was doing two things wrong: using the
condition "locally defined" rather than "externally visible" for
deciding how to handle function descriptors, and "optimizing" out the
symbol reference even once that was fixed. The patch corrects both
behaviors.

GCC was wrongly using GOTOFFFUNCDESC rather than GOTFUNCDESC for
locally defined but externally visible symbols. This could be worked
around with -fPIC, but the patch fixes it. (Probably should do
internal too, but nobody uses internal visibility because it's
underspecified and broken.)

Following the same logic changes should fix the problem on xtensa.

Rich

[-- Attachment #2: bfd-sh.diff --]
[-- Type: text/plain, Size: 1418 bytes --]

--- binutils-2.33.1/bfd/elf32-sh.c.orig	2024-04-04 23:11:28.739136261 +0900
+++ binutils-2.33.1/bfd/elf32-sh.c	2024-04-08 23:14:24.496915074 +0900
@@ -61,7 +61,7 @@
    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) \
+  (!(H) || (H)->dynindx < 0 || (H)->forced_local \
    || ! elf_hash_table (INFO)->dynamic_sections_created)
 \f
 #define SH_PARTIAL32 TRUE
@@ -4405,20 +4405,6 @@
 	      /* 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


[-- Attachment #3: gcc-sh.diff --]
[-- Type: text/plain, Size: 585 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 22:54:01.875106654 +0900
@@ -9147,7 +9147,7 @@
 	{
 	  /* Weak functions may be NULL which doesn't work with
 	     GOTOFFFUNCDESC because the runtime offset is not known.  */
-	  if (SYMBOL_REF_WEAK (orig))
+	  if (SYMBOL_REF_WEAK (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_symGOTOFFFUNCDESC2reg (reg, orig));