Bug 159654

Comment 1 Bruce Evans 2011-08-11 03:57:33 UTC

On Wed, 10 Aug 2011, Robert Millan wrote:

>> Description:
> The following headers use register_t but don't #include <sys/types.h>:

This mostly intentional.  <sys/types.h>, or more often both <sys/param.h>
and <sys/systm.h>, is a prerequisite for all kernel headers.

> arm/include/frame.h
> arm/include/profile.h

<machine/profile.h> is not a user header.  It is an error to include
this header directly (except in the kernel).  The user interface is
<sys/gmon.h>, which also doesn't include <sys/types.h>.  The latter
should be fixed someday, but <sys/types.h> has always been a documented
prerequisite for gmon (see moncontrol(3)).

> arm/include/proc.h

Just an appendage of <sys/proc.h>, so it should and does assume that
<sys/proc.h> has done the necessary including (except for pure MD things).
<sys/proc.h> is a kernel header and is polluted with includes of almost
everything _except_ <sys/types.h> (except possibly by indirect pollution).

> powerpc/include/ucontext.h

<machine/ucontext.h> 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 <ucontext.h> in
applications and via <sys/ucontext.h> in the kernel (<ucontext.h>
is just a copy or symlink of <sys/ucontext.h>).

The amd64 and i386 <machine/ucontext.h>'s spell register_t as __register_t,
but they don't include <machine/_types.h>, so they still have
<sys/ucontext.h> as a prerequisite (<sys/_types.h> or any header that
is documented to include that would work too, but only accidentally
since no types declared in <sys/ucontext.h are used in <machine/ucontext.h>).

The i386 <machine/ucontext.h> 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.

The i386 <machine/ucontext.h> 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.

The i386 <machine/signal.h> 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).

The amd64 <machine/ucontext.h> 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.

The amd64 <machine/signal.h> 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.

kib@ should look at this.

> powerpc/include/pcb.h
> powerpc/include/spr.h
> powerpc/include/reg.h

This is not a kernel header, but I think it is not a user header either.
Applications should use <sys/procfs.h> or <sys/ptrace.h> (preferably the
latter) and never include this directly.  These user headers include
<sys/types.h> and much more (all of <sys/param.h> :-().

> powerpc/include/_align.h

No way including this directly is supported.  It doesn't even use
register_t, since it only defines macros that use register_t.  OTOH,
it should use __register_t in these macros.

The existence of these _align.h headers is another bug.  Old versions
of FreeBSD implemented the alignment macros correctly using ifdefs in
<machine/param.h> (<machine/param.h> normally gives lots of pollution,
but ifdefs were used to prevent that when only the alignment macros
are needed.  It's stupid to have lots of little headers like
<machine/_align.h> when there are nearby enormous kitchen sink headers
like <sys/cdefs.h> and <machine/_types.h> that are almost always
needed.  The messy ifdefs for this in <machine/param.h were only needed
because <machine/param.h> is not properly structured and there is no
nearby kitchen sink header to use.  When <machine/_types.h was restructured
and de-polluted, we lost the nearby <machine/ansi.h> which was more suitable.
<machine/ansi.h> mostly got moved to definitions of _FOO_DECLARED in many
headers.  This move went a bit too far).

> powerpc/include/profile.h
> powerpc/include/pcpu.h
> powerpc/include/pmap.h
> powerpc/include/proc.h
> sparc64/include/smp.h
> sparc64/include/profile.h
> sparc64/include/cpufunc.h

Mostly as for arm and powerpc.

<machine/cpufunc.h> is not supported as a user header, but abusing it
as a user header is quite useful.  Anyone abusing it must know that it
is a kernel header and has a prerequisite of <sys/types.h>.  Its APIs
are undocumented even in the kernel, so this prerequisite is undocmented
too, but all kernel headers have it (or more -- see above).

> sparc64/include/proc.h
> ia64/include/profile.h
> ia64/include/proc.h
> mips/include/sigframe.h
> mips/include/ucontext.h
> mips/include/pcb.h
> mips/include/db_machdep.h

This is an appendage of <ddb/ddb.h>, but I once cleaned up the ddb headers
on i386, so they don't have so many inter-dependencies.  This unfortunately
gave lots of MD pollution in the i386 db_machdep.h, but not the MI
<sys/types.h> pollution (that is intentionally left out of ddb.h).

> mips/include/reg.h
> mips/include/frame.h
> mips/include/proc.h
> mips/include/trap.h
> ofed/include/linux/sched.h
> x86/include/_align.h
> i386/include/sigframe.h
> i386/include/apicvar.h
> i386/include/profile.h
> i386/include/cpufunc.h
> i386/include/proc.h
> amd64/include/pcb.h
> amd64/include/reg.h
> amd64/include/apicvar.h
> amd64/include/frame.h
> amd64/include/intr_machdep.h
> amd64/include/profile.h
> amd64/include/cpufunc.h
> amd64/include/pcpu.h
> amd64/include/proc.h
> sys/sysproto.h

Kernel-only, and also machine-generated.  The machine already generates
lots of pollution.  About half of it is just wrong (e.g., use of
siginfo_t instead of a forward declaration of struct __siginfo) and most
of it is avoided in my version.  But the pollution intentionally doesn't
include <sys/types.h>.

> sys/sysent.h
> sys/proc.h
> sys/ktrace.h
>
>> How-To-Repeat:
>
>> Fix:
> Please consider attached patch to add the missing #include.

Too much pollution for me.

BTW, I have a 42K shell script for checking that headers don't grow
_new_ dependencies.  It essentially just lists all the dependencies
of all important user headers.  There are far too many of these --
<sys/types.h> is the least of the problems.  Unfortunately, this rewards
pollution instead of detecting it, and bugs grep faster than I could
fix them, so I stopped maintaining this in 1999.  Many improvements
were made in core POSIX headers mainly by mike@ in ~2002, but this stalled
when mike departed (?) in ~2003.

The mess in the i386 <machine> directory in 1999 can be read off from the
script:

% 	test $i != ./machine/atomic.h || echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/bootinfo.h || echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/bus.h || echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/cpu.h || echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/cpufunc.h || echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/db_machdep.h || echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/dvcfg.h || echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/elf.h || echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/frame.h || echo "#include <sys/types.h>" >>z.c

Almost everything depends on <sys/types.h>, as is its right.  `test'
is a function that many compiles the header after including it to z.c
(after the above has echoed some #include's there) with strict CFLAGS.
If <sys/types.h> is not needed, then it is left out of the above echo;
this happens rarely.  The echo list was built up startng with no echos
and adding includes until z.c compiles.  Nested pollution makes it very
difficult to see which includes are actually needed or actually used.
It is easier but more disgusting after fixing thousands of failed compiles
when building up the list.  <sys/param.h> is often needed, but my test
doesn't cover many files that need it.

% 	test $i != ./machine/globaldata.h ||
% 	    echo "#include <sys/time.h>
% 		  #include <machine/segments.h>
% 		  #include <machine/tss.h>" >>z.c

In the kernel, it is a style bug to include <sys/time.h> directly, so not
doing it here is correct.  <sys/time.h> is standard pollution in
<sys/param.h>.

% 	test $i != ./machine/i4b_debug.h || echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/i4b_ioctl.h || echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/i4b_rbch_ioctl.h ||
% 	    echo "#include <sys/types.h>
% 		  #include <machine/i4b_ioctl.h>" >>z.c
% 	test $i != ./machine/i4b_trace.h || echo "#include <sys/time.h>" >>z.c
% 	test $i != ./machine/if_wavelan_ieee.h ||
% 	    echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/iic.h || echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/in_cksum.h ||
% 	    echo "#include <sys/types.h>
% 		  #include <netinet/in.h>
% 		  #include <netinet/in_systm.h>
% 		  #include <netinet/ip.h>" >>z.c

Messier headers depend on more things.  Networking headers are especially
bad.

% 	test $i != ./machine/ioctl_bt848.h ||
% 	    echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/md_var.h || echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/npx.h || echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/pc/bios.h || echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/pc/vesa.h || echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/pcb.h || echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/pcb_ext.h || echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/pmap.h ||
% 	    echo "#include <sys/types.h>
% 		  #include <vm/vm.h>
% 		  #include <vm/pmap.h>" >>z.c
% 	test $i != ./machine/profile.h || echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/si.h ||
% 	    echo "#include <sys/types.h>
% 		  #include <sys/tty.h>" >>z.c
% 	test $i != ./machine/signal.h ||
% 	    echo "#include <sys/types.h>
% 		  #include <sys/signal.h>" >>z.c
% 	test $i != ./machine/smb.h || echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/specialreg.h ||
% 	    echo "#include <sys/types.h>
% 		  #include <machine/cpufunc.h>" >>z.c
% 	test $i != ./machine/types.h || echo "#include <sys/types.h>" >>z.c
% 	test $i != ./machine/vm86.h || echo "#include <sys/types.h>" >>z.c

Example of a bad networking header.

% 	test $i != ./netinet/icmp_var.h ||
% 	    echo "#include <sys/types.h>
% 		  #include <netinet/in.h>
% 		  #include <netinet/in_systm.h>
% 		  #include <netinet/ip.h>
% 		  #include <netinet/ip_icmp.h>" >>z.c

Although core POSIX headers have been cleaned up, the pollution and
prerequisites for kernel headers have been cleaned down.  Everything is
more complicated and bloated now.  Almost everything uses mutexes,
so needs <sys/mutex.h>.  Mutexes sometimes use locking of a slightly
different type, or lock profiling, so almost everything needs <sys/lock.h>
too...  However, this complexity is mostly in <sys> and networking
headers and .c files, since <machine> files are mostly too primitive to
need mutexes.  For networking headers, mainly <net/if_var.h> is polluted
with an include of <sys/mutex.h>.  This header is otherwise disgustingly
polluted, and lots of the pollution including kernel mutexes escapes
to userland.  There are new complexities and pollution involving rwlocks
which I haven't kept track of.

Bruce

On Thu, Aug 11, 2011 at 12:57:33PM +1000, Bruce Evans wrote:
> <machine/ucontext.h> 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 <ucontext.h> in
> applications and via <sys/ucontext.h> in the kernel (<ucontext.h>
> is just a copy or symlink of <sys/ucontext.h>).
> 
> The amd64 and i386 <machine/ucontext.h>'s spell register_t as __register_t,
> but they don't include <machine/_types.h>, so they still have
> <sys/ucontext.h> as a prerequisite (<sys/_types.h> or any header that
> is documented to include that would work too, but only accidentally
> since no types declared in <sys/ucontext.h are used in 
> <machine/ucontext.h>).
> 
> The i386 <machine/ucontext.h> 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.
> 
> The i386 <machine/ucontext.h> 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.
> 
> The i386 <machine/signal.h> 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).
> 
> The amd64 <machine/ucontext.h> 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.
> 
> The amd64 <machine/signal.h> 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.
> 
> 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 <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..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.
 	 */
-	__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 {
 
 /*
  * 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 <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) */

Comment 3 Bruce Evans 2011-09-01 11:57:07 UTC

On Sun, 28 Aug 2011, Kostik Belousov wrote:

> On Thu, Aug 11, 2011 at 12:57:33PM +1000, Bruce Evans wrote:
>> <machine/ucontext.h> 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 <ucontext.h> in
>> applications and via <sys/ucontext.h> in the kernel (<ucontext.h>
>> is just a copy or symlink of <sys/ucontext.h>).
>>
>> [... mainly about out of date comments]
>>
>> kib@ should look at this.

He did :-).

> 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).

Yes, sigcontext is the old BSD interface for signal handlers, and
-current only supports it by making it essentially the same as
mcontext via type puns.

> 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 thought that this was more obvious in old versions, since the
interface between the trap frame and the signal frame was rawer,
but history shows otherwise.  The i386 sigcontext in 4.4BSD-Lite
is amusingly simple:

% struct	sigcontext {
% 	int	sc_onstack;	/* sigstack state to restore */
% 	int	sc_mask;	/* signal mask to restore */
% 	int	sc_sp;		/* sp to restore */
% 	int	sc_fp;		/* fp to restore */
% 	int	sc_ap;		/* ap to restore */
% 	int	sc_pc;		/* pc to restore */
% 	int	sc_ps;		/* psl to restore */
% };

This is almost useless in userland (not enough registers), and is just
enough for simple signal handling to work.

386BSD was weirdly different according to what is left of it in FreeBSD-1.
Apparently it had to flesh out the above.  It added (or obtained from Net/2)
the following layering violations and weirdness:
- struct sigcontext is declared in <sys/signal.h> with an i386 ifdef for
   the registers.  It looks more like it is today.
- <machine/signal.h> doesn't exist.  Only <machine/frame.h> exists.  This
   declares a mess of frames: different ones for syscalls, traps,
   interrupts and signals.  Translating between these was painful
   (especially for the syscall and trap frames in ptrace and gdb).
   FreeBSD-1 removed the special syscall frame by making it the same
   as the trap frame.  The others still exist in some form.
- the painful translations include copyng registers 1 at a time from the
   trap frame to the sigcontext in sendsig(), because the struct layouts
   are only vaguely related.  There is now too much copying to make the
   layouts of other things match, so having to rearrange all the registers
   wouldn't take much longer or cost much more, at least on arches with
   only a few registers like i386.
     (sendsig() and sigreturn() touch memory 2-3 times as much as
     they need to.  This starts with copying all the registers from
     the trap frame to the signal frame so that only 1 copyout()
     is needed.  With a better organization, there would be a few
     more copyout()s and no copying within the kernel.

     Also, large areas of the mcontext like unused FP registers should
     be sparsely mapped and not copied out when they are inactive.
     Currently, copying the FP state on amd64 and i386 is always done
     3-4 times per signal.  This mostly defeats the optimizations to
     avoid copying the FP state between the FPU and memory when it is
     not used.  The optimizations prevent copying by FPU, but there
     are 2-4 copies by software instead.  sendsig() copies out the
     unused state, and then to avoid a security hole it has to
     bzero() the unused state first.  That gives 1 and a half copies.
     Then sigreturn() copies in the unused state.  I think it manages
     to not unbzero() the unused state or copy it to the FPU (the
     optimizations avoid always restoring what is in the mcontext to
     the FPU).

     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 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.

% ...
% 
% /*
%  * 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.

Bruce

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) */

Comment 5 Robert Millan 2013-07-02 00:18:35 UTC

State Changed
From-To: open->closed

This PR was filed based on missunderstanding on FreeBSD header pollution 
policies. Closing!