Date: Thu, 1 Sep 2011 14:52:25 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: Robert Millan <rmh@debian.org>, freebsd-gnats-submit@freebsd.org, freebsd-bugs@freebsd.org Subject: Re: misc/159654: 46 kernel headers use register_t but don't #include <sys/types.h> Message-ID: <20110901115224.GM17489@deviant.kiev.zoral.com.ua> In-Reply-To: <20110901192547.R892@besplex.bde.org> References: <201108101917.p7AJHin7006109@red.freebsd.org> <20110811103923.F926@besplex.bde.org> <20110828163516.GS17489@deviant.kiev.zoral.com.ua> <20110901192547.R892@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <machine/ucontext.h> and <machine/fpu.h> for
> >- * the following fields.
> >+ * See <machine/ucontext.h> and <machine/fpu.h> 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 <machine/trap.h> /* 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 <machine/ucontext.h> and <machine/fpu.h> for
- * the following fields.
+ * See <machine/ucontext.h> and <machine/fpu.h> 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 <machine/trap.h> /* 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 <machine/ucontext.h> and <machine/npx.h> for
- * the following fields.
+ * See <machine/ucontext.h> and <machine/npx.h> 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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110901115224.GM17489>