misc/159654: 46 kernel headers use register_t but don't #include
<sys/types.h>
Bruce Evans
brde at optusnet.com.au
Thu Sep 1 11:00:21 UTC 2011
The following reply was made to PR misc/159654; it has been noted by GNATS.
From: Bruce Evans <brde at optusnet.com.au>
To: Kostik Belousov <kostikbel at gmail.com>
Cc: Bruce Evans <brde at optusnet.com.au>, Robert Millan <rmh at debian.org>,
freebsd-gnats-submit at FreeBSD.org, freebsd-bugs at 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
More information about the freebsd-bugs
mailing list