[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20161022001519.GA26769@dora.lan>
Date: Fri, 21 Oct 2016 19:15:19 -0500
From: Bobby Bingham <koorogi@...rogi.info>
To: musl@...ts.openwall.com
Subject: Re: [PATCH 3/3] add s390x port
On Fri, Oct 21, 2016 at 04:27:02PM -0400, Rich Felker wrote:
> Sorry for taking so long to give this the attention it deserves. I
> think it looks good and is mostly ready to commit. A few
> questions/comments though:
Thanks for the review.
>
> On Mon, Jul 25, 2016 at 10:53:00PM -0500, Bobby Bingham wrote:
> > diff --git a/arch/s390x/bits/alltypes.h.in b/arch/s390x/bits/alltypes.h.in
> > new file mode 100644
> > index 0000000..1a83846
> > --- /dev/null
> > +++ b/arch/s390x/bits/alltypes.h.in
> > @@ -0,0 +1,26 @@
> > +#define _Addr long
> > +#define _Int64 long
> > +#define _Reg long
> > +
> > +TYPEDEF __builtin_va_list va_list;
> > +TYPEDEF __builtin_va_list __isoc_va_list;
> > +
> > +#ifndef __cplusplus
> > +TYPEDEF int wchar_t;
> > +#endif
> > +
> > +TYPEDEF double float_t;
>
> Reportedly this is wrong (a bug copied from glibc) and floats are
> actually evaluated as float on s390. Can you check and confirm?
It's evaluating as double here on gcc 4.9.
printf("%f", FLT_MAX * 2.0f) outputs
680564693277057719623408366969033850880.000000 rather than inf.
>
> > diff --git a/arch/s390x/bits/ipc.h b/arch/s390x/bits/ipc.h
> > new file mode 100644
> > index 0000000..4710c12
> > --- /dev/null
> > +++ b/arch/s390x/bits/ipc.h
> > @@ -0,0 +1,14 @@
> > +struct ipc_perm {
> > + key_t __ipc_perm_key;
> > + uid_t uid;
> > + gid_t gid;
> > + uid_t cuid;
> > + gid_t cgid;
> > + mode_t mode;
> > + unsigned short __pad1;
> > + unsigned short __ipc_perm_seq;
> > + unsigned long __pad2;
> > + unsigned long __pad3;
> > +};
>
> __ipc_perm_seq is int on other archs, without the padding. It's not a
> standard type so I don't think we're forced to have matching type, but
> I wonder why this is. OTOH maybe some current archs are buggy because
> of this..
>
Strange. FWIW, it is unsigned short on aarch64 already as well. And the
kernel type is int for ppc(64), so that architecture is probably fine.
> > diff --git a/arch/s390x/syscall_arch.h b/arch/s390x/syscall_arch.h
> > [...]
> > +#define SYSCALL_USE_SOCKETCALL
>
> Is this intentional? s390x does not (or did not always) have dedicated
> syscalls for socket ops?
The dedicated syscalls were only added a year ago in kernel 4.3
(kernel commit 977108f89c989b1eeb5c8d938e1e71913391eb5f)
>
> > diff --git a/src/thread/s390x/__tls_get_offset.s b/src/thread/s390x/__tls_get_offset.s
> > new file mode 100644
> > index 0000000..8ee92de
> > --- /dev/null
> > +++ b/src/thread/s390x/__tls_get_offset.s
> > @@ -0,0 +1,17 @@
> > + .global __tls_get_offset
> > + .type __tls_get_offset,%function
> > +__tls_get_offset:
> > + stmg %r14, %r15, 112(%r15)
> > + aghi %r15, -160
> > +
> > + la %r2, 0(%r2, %r12)
> > + brasl %r14, __tls_get_addr
> > +
> > + ear %r1, %a0
> > + sllg %r1, %r1, 32
> > + ear %r1, %a1
> > +
> > + sgr %r2, %r1
> > +
> > + lmg %r14, %r15, 272(%r15)
> > + br %r14
>
> Can you explain this function and why a tail-call to __tls_get_addr
> doesn't work? Based on the (albeit 32-bit) info in
> https://www.akkadia.org/drepper/tls.pdf it seems like it could but
> maybe I'm missing something. Is there a special contract about
> clobbers?
A tail call doesn't work because the return values aren't the same. From
the linked document:
> The return value of __tls_get_offset is an offset to the thread pointer.
> To get the address of the requested variable the thread pointer needs to
> be added to the return value.
This function is essentially:
size_t __tls_get_offset(size_t offset, char *got)
{
return (char*)__tls_get_addr(got + offset) - (char*)__pthread_self();
}
Except that the second parameter is passed via r12 instead of the usual r3,
which is why it's written in assembly.
--
Bobby
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.
