From owner-freebsd-bugs@FreeBSD.ORG Sun Aug 28 16:57:23 2011 Return-Path: Delivered-To: freebsd-bugs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 307BC106566B for ; Sun, 28 Aug 2011 16:57:23 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (mx0.zoral.com.ua [91.193.166.200]) by mx1.freebsd.org (Postfix) with ESMTP id 95CB58FC18 for ; Sun, 28 Aug 2011 16:57:22 +0000 (UTC) Received: from deviant.kiev.zoral.com.ua (root@deviant.kiev.zoral.com.ua [10.1.1.148]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id p7SGZHgS011677 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 28 Aug 2011 19:35:17 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.4/8.14.4) with ESMTP id p7SGZHfN059733; Sun, 28 Aug 2011 19:35:17 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.4/8.14.4/Submit) id p7SGZG2x059731; Sun, 28 Aug 2011 19:35:16 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Sun, 28 Aug 2011 19:35:16 +0300 From: Kostik Belousov To: Bruce Evans Message-ID: <20110828163516.GS17489@deviant.kiev.zoral.com.ua> References: <201108101917.p7AJHin7006109@red.freebsd.org> <20110811103923.F926@besplex.bde.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Z12CilHQyxEpxhFF" Content-Disposition: inline In-Reply-To: <20110811103923.F926@besplex.bde.org> User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: clamav-milter 0.95.2 at skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-3.3 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00, DNS_FROM_OPENWHOIS autolearn=no version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua 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 X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 28 Aug 2011 16:57:23 -0000 --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--