From owner-freebsd-bugs@FreeBSD.ORG Thu Sep 1 10:57:26 2011 Return-Path: Delivered-To: freebsd-bugs@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id AA9E5106564A; Thu, 1 Sep 2011 10:57:26 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail04.syd.optusnet.com.au (mail04.syd.optusnet.com.au [211.29.132.185]) by mx1.freebsd.org (Postfix) with ESMTP id 18CA48FC0A; Thu, 1 Sep 2011 10:57:25 +0000 (UTC) Received: from c122-106-165-191.carlnfd1.nsw.optusnet.com.au (c122-106-165-191.carlnfd1.nsw.optusnet.com.au [122.106.165.191]) by mail04.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id p81Av7cd006057 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 1 Sep 2011 20:57:09 +1000 Date: Thu, 1 Sep 2011 20:57:07 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Kostik Belousov In-Reply-To: <20110828163516.GS17489@deviant.kiev.zoral.com.ua> Message-ID: <20110901192547.R892@besplex.bde.org> References: <201108101917.p7AJHin7006109@red.freebsd.org> <20110811103923.F926@besplex.bde.org> <20110828163516.GS17489@deviant.kiev.zoral.com.ua> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: Robert Millan , freebsd-gnats-submit@FreeBSD.org, freebsd-bugs@FreeBSD.org, Bruce Evans Subject: Re: misc/159654: 46 kernel headers use register_t but don't #include X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 01 Sep 2011 10:57:26 -0000 On Sun, 28 Aug 2011, Kostik Belousov wrote: > On Thu, Aug 11, 2011 at 12:57:33PM +1000, Bruce Evans wrote: >> 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 in >> applications and via in the kernel ( >> is just a copy or symlink of ). >> >> [... 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 with an i386 ifdef for the registers. It looks more like it is today. - doesn't exist. Only 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 and for > - * the following fields. > + * See and 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