Re: [PATCH 3/3] add s390x port

[Bobby Bingham <koorogi@koorogi.info>]

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