Re: [musl] Initial xtensa/fdpic port review

[Rich Felker <dalias@libc.org>]

On Wed, Feb 28, 2024 at 11:41:30AM -0800, Max Filippov wrote:
> On Wed, Feb 28, 2024 at 11:34 AM Max Filippov <jcmvbkbc@gmail.com> wrote:
> >
> > On Wed, Feb 28, 2024 at 10:36 AM Rich Felker <dalias@libc.org> wrote:
> > > On Wed, Feb 28, 2024 at 01:30:32PM -0500, Rich Felker wrote:
> > > > On Wed, Feb 28, 2024 at 09:20:33AM -0800, Max Filippov wrote:
> > > > >
> > > > > > >               p->relocated = 1;
> > > > > > >       }
> > > > > > > @@ -1485,7 +1487,7 @@ void __libc_exit_fini()
> > > > > > >               if (dyn[0] & (1<<DT_FINI_ARRAY)) {
> > > > > > >                       size_t n = dyn[DT_FINI_ARRAYSZ]/sizeof(size_t);
> > > > > > >                       size_t *fn = (size_t *)laddr(p, dyn[DT_FINI_ARRAY])+n;
> > > > > > > -                     while (n--) ((void (*)(void))*--fn)();
> > > > > > > +                     while (n--) fpaddr(p, *--fn)();
> > > > > >
> > > > > > If this is fixable on the tooling side it really should be fixed
> > > > > > there. init/fini arrays should have actual language-level function
> > > > > > addresses (descriptor addresses on fdpic), not instruction addresses.
> > > > >
> > > > > I read libgcc code at
> > > > >   https://github.com/jcmvbkbc/gcc-xtensa/blob/xtensa-14-8789-fdpic/libgcc/crtstuff.c#L498-L503
> > > > > and the way it's written suggests that this was done on purpose.
> > > > > I put it into the WIP pile to figure out later what the purpose was..
> > > > > I thought that SH might not have this issue because it just didn't
> > > > > use the .array_init/.array_fini.
> > > >
> > > > I'm pretty sure we're using it -- musl-cross-make always forces it on
> > > > via the gcc configure command line -- but it's possible there's some
> > > > override disabling it for sh. I'll try some test cases and confirm
> > > > whether sh is doing it right. Maybe the arm folks will have input on
> > > > this too..?
> > >
> > > Confirmed both that it works, and that it's working via init_array.
> > > GCC emits:
> > >
> > >         .section        .init_array,"aw"
> > >         .align 2
> > >         .long   foo@FUNCDESC
> > >
> > > for
> > >
> > >         __attribute__((__constructor__))
> > >         void foo() { ... }
> > >
> >
> > Oh, no doubt that that C code generates a function descriptor, it
> > works for xtensa too. But the piece of libgcc quoted above specifically
> > puts a pointer to an object, not to a function into the .init_array.
> 
> It was introduced to gcc by the ARM FDPIC series:
> https://github.com/jcmvbkbc/gcc-xtensa/commit/11189793b6ef60645d5d1126d0bd9d0dd83e6583
> 
> This is the second change that I find made by the ARM FDPIC
> series that appears to be not right for other FDPIC ports, first
> being this change to the C++ unwinding code:
> https://github.com/jcmvbkbc/gcc-xtensa/commit/67b0605494f32811364e25328d3522467aaf0638

OK, so the arm folks put explicitly wrong/broken code here. That needs
to be reverted, and they can work out the mess they created on glibc.

There is probably wrong arm target code too whereby gcc is generating
instruction addresses for __attribute__((__constructor__)) rather than
function addresses.

If they have compat to worry about with glibc binaries, that's going
to be a mess for them to fix, but we can just patch it out for musl
target regardless of what they do since we have no existing broken
binaries.

Rich