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

[Bruce Evans]

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