[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Mon, 8 Apr 2024 11:32:44 -0400
From: Rich Felker <dalias@...c.org>
To: Max Filippov <jcmvbkbc@...il.com>
Cc: musl@...ts.openwall.com
Subject: Re: [RFC v2 0/2] xtensa FDPIC port
On Mon, Apr 08, 2024 at 10:41:07AM -0400, Rich Felker wrote:
> 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@...c.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@...c.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@...c.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
> --- 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)
> .
> #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
>
> --- 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));
>
Binutils bug report:
https://sourceware.org/bugzilla/show_bug.cgi?id=31619
GCC bug report:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114641
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
