Date: Sun, 28 Aug 2011 19:35:16 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: 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> Message-ID: <20110828163516.GS17489@deviant.kiev.zoral.com.ua> In-Reply-To: <20110811103923.F926@besplex.bde.org> References: <201108101917.p7AJHin7006109@red.freebsd.org> <20110811103923.F926@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--Z12CilHQyxEpxhFF
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
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>).
>=20
> 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=20
> <machine/ucontext.h>).
>=20
> 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.
>=20
> 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.
>=20
> 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).
>=20
> 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.
>=20
> 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.
>=20
> 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 @@
=20
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 {
=20
/*
* 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 @@
=20
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) */
--Z12CilHQyxEpxhFF
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (FreeBSD)
iEYEARECAAYFAk5abkMACgkQC3+MBN1Mb4jBKgCdH1d8YiFe9Htjy30Vw5dvlWer
o/YAoPbaQVbjkI9/sclHewUTGrD06r/c
=SDSz
-----END PGP SIGNATURE-----
--Z12CilHQyxEpxhFF--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110828163516.GS17489>