misc/159654: 46 kernel headers use register_t but don't #include <sys/types.h>

Kostik Belousov kostikbel at gmail.com
Thu Sep 1 11:52:36 UTC 2011


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.

> 
> >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.
> > */
> 
> 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...
> 
> >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.
> >	 */
> 
> The formatting could be further improved by joining the lines.
> 
> >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.
> >	 */
> 
> I prefer "must" to "shall".  It sounds stricter to me, while "shall" sounds
> too formal and/or Englishman-English.  I like the rules about must/may/
> should used in network RFCs.
> 
> 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".
> 
> There are even larger bugs in the organization of the comments in
> i386/include/signal.h:
> 
> % /*
> %  * Only the kernel should need these old type definitions.
> %  */
> 
> 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.
> 
> % #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 {
> 
> 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.

> 
> % ...
> % 
> % /*
> %  * The sequence of the fields/registers in struct sigcontext should match
> %  * those in mcontext_t.
> %  */
> % struct sigcontext {
> 
> 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 */
 
 /*
- * 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 @@
 
 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 */
 
 /*
- * 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 {
 
 /*
  * 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 @@
 
 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) */
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-bugs/attachments/20110901/e51dff25/attachment.pgp


More information about the freebsd-bugs mailing list