[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160428013819.GA4337@dora.lan>
Date: Wed, 27 Apr 2016 20:38:19 -0500
From: Bobby Bingham <koorogi@...rogi.info>
To: musl@...ts.openwall.com
Subject: Re: [PATCH v2] add powerpc64 port
On Sat, Apr 16, 2016 at 01:09:34PM -0400, Rich Felker wrote:
> On Fri, Apr 15, 2016 at 10:38:33PM +0200, Szabolcs Nagy wrote:
> > * Szabolcs Nagy <nsz@...t70.net> [2016-04-14 15:42:13 +0200]:
> > > * Bobby Bingham <koorogi@...rogi.info> [2016-04-14 03:01:38 -0500]:
> > > >
> > > > How important is it to match glibc here?
> > > >
> > >
> > > i think we don't care about abi compat
> > > (but it might be interesting to check how much abi
> > > difference there is between glibc and musl, i can
> > > do this if i can build a musl+glibc toolchain)
> > >
> >
> > attached some c++ abi comparisions
>
> Thanks!
>
> > (the comparision was not entirely clean, it involved
> > various manual hacks, but most types and functions
> > should be compared correctly glibc vs musl)
> >
> > two interesting gcc warnings:
> >
> > powerpc64 elf_vrreg_t (both musl and glibc):
> > note: the ABI of passing aggregates with 16-byte alignment has changed in GCC 5
>
> This could be mildly problematic, but it's in a junk interface that's
> unlikely to be used cross-library anyway.
>
> > x86_64 struct inotify_event (musl only):
> > note: the ABI of passing struct with a flexible array member has changed in GCC 4.4
>
> This looks irrelevant; passing the struct by value does not make sense.
>
> > --- abi_type.powerpc64le.glibc 2016-04-15 21:00:27.432246225 +0100
> > +++ abi_type.powerpc64le.musl 2016-04-15 20:57:39.000149371 +0100
> > @@ -97 +96 @@
> > -fexcept_t: unsigned int, unsigned int*, size (*) [4], align (*) [4]
> > +fexcept_t: unsigned long, unsigned long*, size (*) [8], align (*) [8]
>
> Is this right?
I've fixed this locally. I'll submit a new patch soon.
>
> > @@ -100,2 +99,2 @@
> > -fpos_t: _G_fpos_t, _G_fpos_t*, size (*) [16], align (*) [8]
> > +fpos_t: _G_fpos64_t, _G_fpos64_t*, size (*) [16], align (*) [8]
>
> You should probably be testing with -D_FILE_OFFSET_BITS=64. musl
> implements that ABI, and it matters in a few places even on 64-bit
> archs, like here.
>
> > @@ -130 +130 @@
> > -jmp_buf: __jmp_buf_tag*, __jmp_buf_tag (*) [1], size (*) [656], align (*) [16]
> > +jmp_buf: __jmp_buf_tag*, __jmp_buf_tag (*) [1], size (*) [664], align (*) [8]
>
> Let's fix at least alignment and hopefully size.
Ok
>
> > @@ -141 +141 @@
> > -mcontext_t: mcontext_t, mcontext_t*, size (*) [1272], align (*) [8]
> > +mcontext_t: sigcontext, sigcontext*, size (*) [1528], align (*) [8]
>
> IIRC on other archs we made an effort to make the tag here match ABI
> (duplicating the struct def if needed). Not sure if it matters.
I can duplicate the structure if you want. But it looks like glibc used
to do 'typedef struct sigcontext mcontext_t' as well:
https://sourceware.org/git/?p=glibc.git;a=blobdiff;f=sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h;h=a499a80ef9994e541b866202ee8440843b004fdd;hp=b75e25a3c84c852b22e1690ff530d3ddb8dff257;hb=5ef6ae4bdb;hpb=39b04aa39823faf1cc414e7f3eca4f43e01426e4
>
> > @@ -183,2 +183,2 @@
> > -pthread_rwlockattr_t: pthread_rwlockattr_t, pthread_rwlockattr_t*, size (*) [8], align (*) [8]
> > +pthread_rwlockattr_t: pthread_rwlockattr_t, pthread_rwlockattr_t*, size (*) [8], align (*) [4]
>
> Alignment difference was unintentional here but seems bad to try to
> fix for existing archs and I don't want to make the pthread types
> arch-specific; actually I want to move them to the shared
> alltypes.h.in or new arch/generic* with just a dependency on 32/64
> bit.
>
> > -pthread_spinlock_t: int, int volatile*, size (*) [4], align (*) [4]
> > +pthread_spinlock_t: int, int*, size (*) [4], align (*) [4]
>
> Did glibc add volatile here? IIRC it was not there to begin with. If
> so they broke their own C++ ABI. I'd like to change this too, and if
> glibc did change it without anyone noticing/caring, we probably could
> too.
>
> > @@ -195 +195 @@
> > -sem_t: sem_t, sem_t*, size (*) [32], align (*) [8]
> > +sem_t: sem_t, sem_t*, size (*) [32], align (*) [4]
>
> > @@ -229,2 +229,2 @@
> > -cmsghdr: cmsghdr, cmsghdr*, size (*) [16], align (*) [8]
> > +cmsghdr: cmsghdr, cmsghdr*, size (*) [16], align (*) [4]
>
> This is likely going to hit the same issue we're trying to debug on
> mips64.
The mips64 issue ended up not being alignment related. Do you still
want me to do something about this? And if so, do you have a suggestion?
>
> > @@ -416 +417 @@
> > -ucontext_t: ucontext, ucontext*, size (*) [1440], align (*) [8]
> > +ucontext_t: ucontext, ucontext*, size (*) [1696], align (*) [8]
>
> This may be a real problem. ucontext_t is ABI between kernel and
> userspace and if it's wrong cancellation won't work right.
Kernel commit ce48b2100785 expanded the vmx_reserve member of mcontext_t
by 256 bytes. The glibc headers haven't been updated for this expansion.
>
> Rich
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.
