* Re: [PATCH] powerpc: make ucontext_t API match glibc
2018-02-21 20:19 [PATCH] powerpc: make ucontext_t API match glibc Matthias Schiffer
@ 2018-02-22 18:28 ` Rich Felker
2018-02-22 21:19 ` A. Wilcox
0 siblings, 1 reply; 3+ messages in thread
From: Rich Felker @ 2018-02-22 18:28 UTC (permalink / raw)
To: musl
On Wed, Feb 21, 2018 at 09:19:39PM +0100, Matthias Schiffer wrote:
> In glibc, ucontext_t's uc_mcontext contains the uc_regs field, pointing at
> the actual mcontext_t at the end of the structure. Bring our definition in
> line with what glibc does to allow builing libunwind.
> ---
>
> Note that this fixes only one of the two problems arising when building
> libunwind on PPC with musl. The other is that libunwind needs register
> definitions from Linux's asm/ptrace.h, which is pulled in through
> sys/user.h in glibc. As asm/ptrace.h contains a conflicting pt_regs
> definition, I've patched this into libunwind itself now in my OpenWrt tree:
>
> https://git.openwrt.org/?p=openwrt/staging/neoraider.git;a=blob;f=package/libs/libunwind/patches/004-ppc-registers-musl.patch
This patch isn't acceptable. In short, glibc's definition is wrong; it
doesn't and can't be made to conform to POSIX, much less to both POSIX
and the kernel's ABI for ucontext/mcontext. POSIX requires the member
uc_mcontext to have type mcontext_t, not some other struct type, and
if you defined mcontext_t as the struct containing the uc_regs
pointer, then uc_regs could not have type mcontext_t* and still point
to an object of type mcontext_t matching the kernel's register layout.
Basically, what seems to have happened is that very early powerpc
kernels ignored the fact that ucontext_t/mcontext_t are permanent ABI
with userspace and made the definition of mcontext_t model-specific.
The people doing glibc's powerpc port, who weren't really savvy about
conformance requirements, responded by making glibc's userspace
definition of mcontext_t simply be (a dummy struct containing) the
uc_regs pointer to the real mcontext, rather than the kernel's
mcontex_t/sigcontext.
Modern kernels (anything capable of running modern glibc or even
capable of running musl) do not have any such issue; the location of
mcontext_t in ucontext_t and its base layout are fixed ABI. musl
simply uses the kernel API/ABI directly, conforming to POSIX, rather
than the outdated glibc hack.
More information on the issue can be found in this thread:
http://www.openwall.com/lists/musl/2016/04/28/2
Message-ID: <20160428020751.GX21636@brightrain.aerifal.cx>
As for applications affected, they need patching; fortunately it's a
fairly benign/trivial patch. It should be relatively easy to add
configure tests to make this conditional so that the patches are
acceptable upstream.
The above text should probably be made into an FAQ item.
Rich
> arch/powerpc/bits/signal.h | 7 ++++---
> arch/powerpc/pthread_arch.h | 2 +-
> 2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/bits/signal.h b/arch/powerpc/bits/signal.h
> index 06efb11cf271..f842f5b82985 100644
> --- a/arch/powerpc/bits/signal.h
> +++ b/arch/powerpc/bits/signal.h
> @@ -64,10 +64,11 @@ typedef struct __ucontext {
> struct __ucontext *uc_link;
> stack_t uc_stack;
> int uc_pad[7];
> - mcontext_t *uc_regs;
> + struct {
> + mcontext_t *uc_regs;
> + } uc_mcontext;
> sigset_t uc_sigmask;
> - int uc_pad2[3];
> - mcontext_t uc_mcontext;
> + char uc_pad2[12 + sizeof(mcontext_t)];
> } ucontext_t;
>
> #define SA_NOCLDSTOP 1U
> diff --git a/arch/powerpc/pthread_arch.h b/arch/powerpc/pthread_arch.h
> index 7c5c4fadb211..8791ec5deb56 100644
> --- a/arch/powerpc/pthread_arch.h
> +++ b/arch/powerpc/pthread_arch.h
> @@ -17,6 +17,6 @@ static inline struct pthread *__pthread_self()
>
> // the kernel calls the ip "nip", it's the first saved value after the 32
> // GPRs.
> -#define MC_PC gregs[32]
> +#define MC_PC uc_regs->gregs[32]
>
> #define CANARY canary_at_end
> --
> 2.16.2
^ permalink raw reply [flat|nested] 3+ messages in thread