From owner-freebsd-bugs@FreeBSD.ORG Thu Sep 1 11:52:35 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 CBA8C106566B; Thu, 1 Sep 2011 11:52:35 +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 1F0008FC17; Thu, 1 Sep 2011 11:52:34 +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 p81BqPtD003515 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 1 Sep 2011 14:52:25 +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 p81BqPCu006029; Thu, 1 Sep 2011 14:52:25 +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 p81BqPVc006028; Thu, 1 Sep 2011 14:52:25 +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: Thu, 1 Sep 2011 14:52:25 +0300 From: Kostik Belousov To: Bruce Evans Message-ID: <20110901115224.GM17489@deviant.kiev.zoral.com.ua> References: <201108101917.p7AJHin7006109@red.freebsd.org> <20110811103923.F926@besplex.bde.org> <20110828163516.GS17489@deviant.kiev.zoral.com.ua> <20110901192547.R892@besplex.bde.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="mE6eiWAW7MIqbbCC" Content-Disposition: inline In-Reply-To: <20110901192547.R892@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: Thu, 01 Sep 2011 11:52:36 -0000 --mE6eiWAW7MIqbbCC Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Sep 01, 2011 at 08:57:07PM +1000, Bruce Evans wrote: > On Sun, 28 Aug 2011, Kostik Belousov wrote: > kib is working on AVX support on amd64 and i386. AVX needs changing > the mcontext layout yet again, although we thought we left space > for expansion of SSE structures. AVX has a much larger context, > so optimizing away copying of it is a more important unpessimization.) I intend to work, but cannot buy Intel motherboard for LGA 1155 with the serial port. >=20 > >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 mat= ch > >- * 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. > > */ >=20 > I could clean this up a bit more if you want. Mainly English fixes. > Some of the "should"s should be "must"s since not matching is not an > option... >=20 > >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. > > */ >=20 > The formatting could be further improved by joining the lines. >=20 > >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 @@ > > > >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. > > */ >=20 > I prefer "must" to "shall". It sounds stricter to me, while "shall" soun= ds > too formal and/or Englishman-English. I like the rules about must/may/ > should used in network RFCs. >=20 > The second sentence is still non-grammatical here. It should be a > clause in the first sentence (just change ". So" to ", so"), or start > with something like "This is so". >=20 > There are even larger bugs in the organization of the comments in > i386/include/signal.h: >=20 > % /* > % * Only the kernel should need these old type definitions. > % */ >=20 > This comment only applies to osigcontext, but is outside the scope > of the ifdef for that. It applies to 1 declararion but claims to > appy to (multiple) definitions (sic). If it applied to multiple > ones, then it would be normal to separate it from the first one > with a blank line, but this is not done. So the scope of this > comment is hard to see. >=20 > % #if defined(_KERNEL) && defined(COMPAT_43) > % /* > % * Information pushed on stack when a signal is delivered. > % * This is used by the kernel to restore state following > % * execution of the signal handler. It is also made available > % * to the handler to allow it to restore state properly if > % * a non-standard exit is performed. > % */ > % struct osigcontext { >=20 > All of this comment is the same as in 4.4BSD-Lite. It now only > applies to osigcontext so its placement within the ifdef is > almost correct, but this leaves no similar comment about ordinary > sigcontext. On 386, I moved the 'Only the kernel should need' near the osigcontext declaration. On amd64, the sentence is removed at all, there is currently no osigcontext for 64bit. >=20 > % ... > %=20 > % /* > % * The sequence of the fields/registers in struct sigcontext should mat= ch > % * those in mcontext_t. > % */ > % struct sigcontext { >=20 > This comment is correct, but doesn't say anything about what sigcontext > actually is. It is of course just an alias for parts of mcontext, and > may be used by userland, but isn't used directly by the kernel. This > contrasts with osigcontext, which is used by both the kernel and > userland if userland requests an "old" signal context. The 'Information pushed on stack' is now the header for the whole file. diff --git a/sys/amd64/include/signal.h b/sys/amd64/include/signal.h index 228e2d9..0374339 100644 --- a/sys/amd64/include/signal.h +++ b/sys/amd64/include/signal.h @@ -47,18 +47,14 @@ typedef long sig_atomic_t; #include /* codes for SIGILL, SIGFPE */ =20 /* - * Only the kernel should need these old type definitions. - */ -/* * Information pushed on stack when a signal is delivered. * This is used by the kernel to restore state following * execution of the signal handler. It is also made available * to the handler to allow it to restore state properly if * 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 must match those in mcontext_t and struct trapframe. */ struct sigcontext { struct __sigset sc_mask; /* signal mask to restore */ @@ -93,8 +89,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..75b7bd2 100644 --- a/sys/amd64/include/ucontext.h +++ b/sys/amd64/include/ucontext.h @@ -41,12 +41,13 @@ =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 must match the layout of + * struct sigcontext after the sc_mask member. This is 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..c636c2c 100644 --- a/sys/i386/include/signal.h +++ b/sys/i386/include/signal.h @@ -46,16 +46,17 @@ typedef int sig_atomic_t; #include /* codes for SIGILL, SIGFPE */ =20 /* - * Only the kernel should need these old type definitions. - */ -#if defined(_KERNEL) && defined(COMPAT_43) -/* * Information pushed on stack when a signal is delivered. * This is used by the kernel to restore state following * execution of the signal handler. It is also made available * to the handler to allow it to restore state properly if * a non-standard exit is performed. */ + +#if defined(_KERNEL) && defined(COMPAT_43) +/* + * Only the kernel should need these old type definitions. + */ struct osigcontext { int sc_onstack; /* sigstack state to restore */ osigset_t sc_mask; /* signal mask to restore */ @@ -83,7 +84,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 +110,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) */ --mE6eiWAW7MIqbbCC Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (FreeBSD) iEYEARECAAYFAk5fcfgACgkQC3+MBN1Mb4hutwCgvjNunjTUKcfZ1058T52wPFU4 8UMAoKWdfaUNnVeMpT9o1I5sHCHyn2Dc =XXqF -----END PGP SIGNATURE----- --mE6eiWAW7MIqbbCC--