[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240408144107.GV4163@brightrain.aerifal.cx>
Date: Mon, 8 Apr 2024 10:41:07 -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 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
View attachment "bfd-sh.diff" of type "text/plain" (1418 bytes)
View attachment "gcc-sh.diff" of type "text/plain" (586 bytes)
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.
