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) */
pgpuYSZyARhNQ.pgp
Description: PGP signature
