From owner-freebsd-bugs@FreeBSD.ORG Sun Aug 28 16:40:14 2011 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 5D4D1106564A for ; Sun, 28 Aug 2011 16:40:14 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 4BC108FC08 for ; Sun, 28 Aug 2011 16:40:14 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.4/8.14.4) with ESMTP id p7SGeE5Y027439 for ; Sun, 28 Aug 2011 16:40:14 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.4/8.14.4/Submit) id p7SGeErI027438; Sun, 28 Aug 2011 16:40:14 GMT (envelope-from gnats) Date: Sun, 28 Aug 2011 16:40:14 GMT Message-Id: <201108281640.p7SGeErI027438@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: Kostik Belousov Cc: Subject: Re: misc/159654: 46 kernel headers use register_t but don't #include X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Kostik Belousov List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 28 Aug 2011 16:40:14 -0000 The following reply was made to PR misc/159654; it has been noted by GNATS. From: Kostik Belousov To: Bruce Evans Cc: Robert Millan , freebsd-gnats-submit@freebsd.org, freebsd-bugs@freebsd.org Subject: Re: misc/159654: 46 kernel headers use register_t but don't #include Date: Sun, 28 Aug 2011 19:35:16 +0300 --Z12CilHQyxEpxhFF Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Aug 11, 2011 at 12:57:33PM +1000, Bruce Evans wrote: > is not a kernel header, but it is not a user > header either. It is an error to include this header directly. The > only supported includes of it are indirectly via in > applications and via in the kernel ( > is just a copy or symlink of ). >=20 > The amd64 and i386 's spell register_t as __register_= t, > but they don't include , so they still have > as a prerequisite ( or any header that > is documented to include that would work too, but only accidentally > since no types declared in ). >=20 > The i386 used to spell register_t as int, and it > now has a rotted comment that depends on this spelling for easy checking. > It says that the first 20 of __mcontext fields MUST match the definition > of sigcontext, but for sigcontext the fields are spelled with an int. > Also, the number '20' is confusing and rotted. It is the first 20 fields > of __mcontext that used to have to match the not the first 20 fields of > sigcontext, but the second through 21st fields of sigcontext (since the > first field of mcontext and sigcontext is not in __mcontext). Both > structures have been expanded, and binary compatibility seems to have > been perfected, so there are now 28 fields in __mcontext that all seem > to be binary compatible with sigcontext, giving perfect binary > compatibility of mcontext with sigcontext. >=20 > The i386 also declares struct mcontext4. The magic > 20 is correct for it, since binary compatibility is lost at the "new" > 21st field (there is mc_len but no sc_len, and the FP data structures > have different sizes). But the comment is only attached to struct > __mcontext where it doesn't apply at all -- it was easy to implement > perfect compatibility of the whole structs when the ABI was changed. >=20 > The i386 also declares struct osigcontext. This is > even older than struct mcontext4. The magic 20 applies to it to, even > more so (osigcontext ends after the 1+20'th field in it, where > mcontext4 just becomes incompatitible after the 1+20'th field). >=20 > The amd64 has even larger bugs in the comment. > amd64 has at least 8 more registers, so it should have at least 8 more > fields, but the comment only says 24. The code seems to be correct, > giving perfect binary compatibility for all 36 fields. >=20 > The amd64 spells register_t as long where the i386 > version spells it as int. This difference is of course necessary if > a basic type is used, but it gives larger diffs than if [__]register_t > is used. Another annoying lexical style bug in these files is that > the fields are hard to count and hard to see all at once due to > vertical whitespace being used to separate blocks that are only of > historical interest, but with much the same bugs as the comment -- the > 20 special fields used to be nicely blocked off, but now they are > nastily blocked off because the magic 20 has no significance in the > current ABI. >=20 > kib@ should look at this. I suspect that struct sigcontext is unused at all. I understand that it is a user-mode interface, but the kernel uses mcontext_t almost exclusively, except for the compat layouts. This made especially confusing by packing sigmask into sigcontext as the first member, but using a separate field in ucontext_t right before uc_mcontext (I hope this can be deciphered). And, the driving force beyond the layout is struct trapframe, that is well-known but not obvious until you start to remember sigsend() code. I tried to do some minimal comment cleanup. diff --git a/sys/amd64/include/signal.h b/sys/amd64/include/signal.h index 228e2d9..9e9c9ad 100644 --- a/sys/amd64/include/signal.h +++ b/sys/amd64/include/signal.h @@ -57,8 +57,8 @@ typedef long sig_atomic_t; * a non-standard exit is performed. */ /* - * The sequence of the fields/registers in struct sigcontext should match - * those in mcontext_t. + * The sequence of the fields/registers after sc_mask in struct + * sigcontext should match those in mcontext_t and struct trapframe. */ struct sigcontext { struct __sigset sc_mask; /* signal mask to restore */ @@ -93,8 +93,8 @@ struct sigcontext { long sc_ss; long sc_len; /* sizeof(mcontext_t) */ /* - * XXX - See and for - * the following fields. + * See and for + * the following fields. */ long sc_fpformat; long sc_ownedfp; diff --git a/sys/amd64/include/ucontext.h b/sys/amd64/include/ucontext.h index c5bbd65..0341527 100644 --- a/sys/amd64/include/ucontext.h +++ b/sys/amd64/include/ucontext.h @@ -41,12 +41,12 @@ =20 typedef struct __mcontext { /* - * The first 24 fields must match the definition of - * sigcontext. So that we can support sigcontext - * and ucontext_t at the same time. + * The definition of mcontext_t shall match the layout of + * struct sigcontext after the sc_mask member. So that we can + * support sigcontext and ucontext_t at the same time. */ - __register_t mc_onstack; /* XXX - sigcontext compat. */ - __register_t mc_rdi; /* machine state (struct trapframe) */ + __register_t mc_onstack; /* XXX - sigcontext compat. */ + __register_t mc_rdi; /* machine state (struct trapframe) */ __register_t mc_rsi; __register_t mc_rdx; __register_t mc_rcx; diff --git a/sys/i386/include/signal.h b/sys/i386/include/signal.h index 7a5f876..47035de 100644 --- a/sys/i386/include/signal.h +++ b/sys/i386/include/signal.h @@ -83,7 +83,7 @@ struct osigcontext { =20 /* * The sequence of the fields/registers in struct sigcontext should match - * those in mcontext_t. + * those in mcontext_t and struct trapframe. */ struct sigcontext { struct __sigset sc_mask; /* signal mask to restore */ @@ -109,8 +109,8 @@ struct sigcontext { int sc_ss; int sc_len; /* sizeof(mcontext_t) */ /* - * XXX - See and for - * the following fields. + * See and for + * the following fields. */ int sc_fpformat; int sc_ownedfp; diff --git a/sys/i386/include/ucontext.h b/sys/i386/include/ucontext.h index d8657d3..d9f8344 100644 --- a/sys/i386/include/ucontext.h +++ b/sys/i386/include/ucontext.h @@ -33,9 +33,9 @@ =20 typedef struct __mcontext { /* - * The first 20 fields must match the definition of - * sigcontext. So that we can support sigcontext - * and ucontext_t at the same time. + * The definition of mcontext_t shall match the layout of + * struct sigcontext after the sc_mask member. So that we can + * support sigcontext and ucontext_t at the same time. */ __register_t mc_onstack; /* XXX - sigcontext compat. */ __register_t mc_gs; /* machine state (struct trapframe) */ --Z12CilHQyxEpxhFF Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (FreeBSD) iEYEARECAAYFAk5abkMACgkQC3+MBN1Mb4jBKgCdH1d8YiFe9Htjy30Vw5dvlWer o/YAoPbaQVbjkI9/sclHewUTGrD06r/c =SDSz -----END PGP SIGNATURE----- --Z12CilHQyxEpxhFF--