svn commit: r232275 - in head/sys: amd64/include i386/include pc98/include x86/include

Thu Mar 1 22:47:38 UTC 2012

On Wednesday 29 February 2012 06:01:36 Bruce Evans wrote:
> On Tue, 28 Feb 2012, Tijl Coosemans wrote:
> 
>> Log:
>>  Copy amd64 setjmp.h to x86 and replace amd64/i386/pc98 setjmp.h with stubs.
> 
> This may be correct (except for comment), but it is confusing.
> 
>> Added:
>>  head/sys/x86/include/setjmp.h
>>     - copied unchanged from r232268, head/sys/amd64/include/setjmp.h
>> Modified:
>>  head/sys/amd64/include/setjmp.h
>>  head/sys/i386/include/setjmp.h
>>  head/sys/pc98/include/setjmp.h
>>
>> Modified: head/sys/amd64/include/setjmp.h
>> ==============================================================================
>> --- head/sys/amd64/include/setjmp.h	Tue Feb 28 22:15:46 2012	(r232274)
>> +++ head/sys/amd64/include/setjmp.h	Tue Feb 28 22:17:52 2012	(r232275)
>> @@ -1,50 +1,6 @@
>> ...
>> -#define	_JBLEN	12		/* Size of the jmp_buf on AMD64. */
>> -
>> -/*
>> - * jmp_buf and sigjmp_buf are encapsulated in different structs to force
>> - * compile-time diagnostics for mismatches.  The structs are the same
>> - * internally to avoid some run-time errors for mismatches.
>> - */
>> -#if __BSD_VISIBLE || __POSIX_VISIBLE || __XSI_VISIBLE
>> -typedef	struct _sigjmp_buf { long _sjb[_JBLEN]; } sigjmp_buf[1];
>> -#endif
>> -
>> -typedef	struct _jmp_buf { long _jb[_JBLEN]; } jmp_buf[1];
> 
> On amd64, the comment was specific to amd64 (but with amd64 misspelled
> as AMD64), and actually matched the code.
> 
>> Modified: head/sys/i386/include/setjmp.h
>> ==============================================================================
>> --- head/sys/i386/include/setjmp.h	Tue Feb 28 22:15:46 2012	(r232274)
>> +++ head/sys/i386/include/setjmp.h	Tue Feb 28 22:17:52 2012	(r232275)
>> @@ -1,50 +1,6 @@
>> -#define	_JBLEN	11		/* Size of the jmp_buf on x86. */
>> -
>> -/*
>> - * jmp_buf and sigjmp_buf are encapsulated in different structs to force
>> - * compile-time diagnostics for mismatches.  The structs are the same
>> - * internally to avoid some run-time errors for mismatches.
>> - */
>> -#if __BSD_VISIBLE || __POSIX_VISIBLE || __XSI_VISIBLE
>> -typedef	struct _sigjmp_buf { int _sjb[_JBLEN + 1]; } sigjmp_buf[1];
>> -#endif
>> -
>> -typedef	struct _jmp_buf { int _jb[_JBLEN + 1]; } jmp_buf[1];
> 
> On i386, the comment had a differently wrong name for the old code, and
> mismatched the old code, but is correct for the new code.
> 
> _JBLEN was apparently 1 less than what it was claimed to be on i386.
> I forget what the extra 1 is for.
> 
>> Copied: head/sys/x86/include/setjmp.h (from r232268, head/sys/amd64/include/setjmp.h)
>> ==============================================================================
>> --- /dev/null	00:00:00 1970	(empty, because file is newly added)
>> +++ head/sys/x86/include/setjmp.h	Tue Feb 28 22:17:52 2012	(r232275, copy of r232268, head/sys/amd64/include/setjmp.h)
>> @@ -0,0 +1,50 @@
>> ...
>> +#define	_JBLEN	12		/* Size of the jmp_buf on AMD64. */
> 
> This code looks wrong, since _JBLEN was only 11 for i386, but binary
> compatibility of the jmp_buf is preserved by the extra 1 in it.
> 
> This comment is wrong, since the code is no longer just for AMD64 (sic),
> and the comment makes it look more like the code has an off-by-1 error
> for !AMD64.
> 
>> +
>> +/*
>> + * jmp_buf and sigjmp_buf are encapsulated in different structs to force
>> + * compile-time diagnostics for mismatches.  The structs are the same
>> + * internally to avoid some run-time errors for mismatches.
>> + */
>> +#if __BSD_VISIBLE || __POSIX_VISIBLE || __XSI_VISIBLE
>> +typedef	struct _sigjmp_buf { long _sjb[_JBLEN]; } sigjmp_buf[1];
>> +#endif
>> +
>> +typedef	struct _jmp_buf { long _jb[_JBLEN]; } jmp_buf[1];
> 
> No extra 1 for either now.
> 
> The extra 1 was apparently a placeholder for the signal mask and became
> obsolete 15-20 years ago.  4.4BSD-Lite2 claims not to support the signal
> mask, and doesn't have space for it in the plain jmp_buf.  setjmp.h
> was in src/include and was moved to MD files to clean up its ifdefs.
> Now we're barely escaping putting the ifdefs back in the x86 version,
> sigh.  Ifdefs are really needed, since the i386 version is too small
> to have space for mxcsr, and mxcsr is only hacked into the amd64 version
> by packing it using the doubled space given by long entries.
> 
> From the 4.4BSD-Lite2 version:
> % #if defined(hp300) || defined(__hp300__) || defined(luna68k) || defined(__luna68k__)
> % #define _JBLEN	17
> % #endif
> % 
> % #if defined(i386) || defined(__i386__)
> % #define _JBLEN	10
> % #endif
> % 
> % #if defined(mips) || defined(__mips__)
> % #define _JBLEN	83
> % #endif
> % 
> % #if defined(sparc) || defined(__sparc__)
> % #define _JBLEN	10
> % #endif
> % 
> % #if defined(tahoe) || defined(__tahoe__)
> % #define _JBLEN	10
> % #endif
> % 
> % #if defined(vax) || defined(__vax__)
> % #define _JBLEN	10
> % #endif
> % 
> % #ifndef _ANSI_SOURCE
> % /*
> %  * WARNING: sigsetjmp() isn't supported yet, this is a placeholder.
> %  */
> % typedef int sigjmp_buf[_JBLEN + 1];
> % #endif /* not ANSI */
> % 
> % typedef int jmp_buf[_JBLEN];
> 
> The extra 1 seems to have been nonsense there too.  BSD has always (?)
> saved the signal mask in the ordinary jmp_buf.  4.4BSD-Lite2 seems to
> do this, and could hardly have worked without it.  So _JBLEN was already
> large enough and didn't need a placeholder for the signal mask.  But
> I now vaguely remember stealing this extra 1 for the FP control word.
> It was sloppy not to update the comment.  Later, jb left the extra 1
> and the comment alone.  The commit log says that _JBLEN was left in
> case something (mis)uses it.  I think it should be removed.  It should
> only be used in the 2 jmp_buf structs, and now that sigsetjmp() has
> been standard for so long, and since making the structure layouts
> identical is best for any system which saves the signal mask in setjmp()
> and probably also on any system that doesn't, I don't see any need for
> separate jmp_buf structs (just use separate typedefs using the same
> struct).
> 
> Next, I don't see why <setjmp.h> can't be MI except for the definition
> of _JBLEN.  _JBLEN can be declared in some harmless MD header that is
> already usually included.  Since we removed machine/ansi.h,
> machine/types.h seems to be the only suitable one.
> 
> Here is what current arches have in their machine/setjmp.h:
> 
> amd64, i386: not much
> arm: has lots of comments and register offsets.  These are defined as
>       _JB_REG_* so they aren't pollution, but there is no reason to
>       export them to the application <setjmp.h> either.  The actual
>       structs are the usual 2 arrays of ints, with the extra 1 for
>       both the comment not matching the code, as on i386.  The extra
>       1 is unused, or at least has no _JB_REG_* for it.
> ia64: has lots of namespace-pollution definitions under a __BSD_VISIBLE
>       ifdef.  The structs are arrays of long doubles!  This defeats
>       my idea of using a MI array of register_t's.  _JBLEN could be
>       expanded for long doubles, but __align() would be required too,
>       and it gets messier than a separate file.
> mips: just the usual extra 1 (now 4 instances for 32/64 doubling) and
>      the usual comment not matching the code.
> powerpc: like x86
> sparc64: just the usual extra 1.  The comment is fixed by removing it.
> 
> So the extra 1 seems to be just a ~20-year old mistake, faithfully
> propagated to all arches except amd64 i386, with unfaithful propagation
> just fixed for i386.

If we could add the returns_twice attribute to setjmp() then the
compiler makes sure all registers are dead before calling it and
jmp_buf wouldn't have to be that big.

Also, from ISO C: "All accessible objects have values, and all other
components of the abstract machine [249] have state, as of the time the
longjmp function was called"

"[249] This includes, but is not limited to, the floating-point status
flags and the state of open files."

So I think storing mxcsr in jmp_buf is incorrect.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 228 bytes
Desc: This is a digitally signed message part.
Url : http://lists.freebsd.org/pipermail/svn-src-head/attachments/20120301/52a364e3/attachment.pgp