Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 1 Sep 2011 11:00:20 GMT
From:      Bruce Evans <brde@optusnet.com.au>
To:        freebsd-bugs@FreeBSD.org
Subject:   Re: misc/159654: 46 kernel headers use register_t but don't #include <sys/types.h>
Message-ID:  <201109011100.p81B0KO9091539@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: Bruce Evans <brde@optusnet.com.au>
To: Kostik Belousov <kostikbel@gmail.com>
Cc: Bruce Evans <brde@optusnet.com.au>, 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 20:57:07 +1000 (EST)

 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



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201109011100.p81B0KO9091539>