Date: Thu, 1 Sep 2011 12:00:24 GMT From: Kostik Belousov <kostikbel@gmail.com> To: freebsd-bugs@FreeBSD.org Subject: Re: misc/159654: 46 kernel headers use register_t but don't #include <sys/types.h> Message-ID: <201109011200.p81C0OqX049235@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR misc/159654; it has been noted by GNATS. 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> Date: Thu, 1 Sep 2011 14:52:25 +0300 --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?201109011200.p81C0OqX049235>