From owner-svn-src-all@FreeBSD.ORG Wed Feb 29 05:01:40 2012 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 87E43106564A; Wed, 29 Feb 2012 05:01:40 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail13.syd.optusnet.com.au (mail13.syd.optusnet.com.au [211.29.132.194]) by mx1.freebsd.org (Postfix) with ESMTP id 07CC88FC1A; Wed, 29 Feb 2012 05:01:39 +0000 (UTC) Received: from c211-30-171-136.carlnfd1.nsw.optusnet.com.au (c211-30-171-136.carlnfd1.nsw.optusnet.com.au [211.30.171.136]) by mail13.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q1T51aSK019498 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 29 Feb 2012 16:01:37 +1100 Date: Wed, 29 Feb 2012 16:01:36 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Tijl Coosemans In-Reply-To: <201202282217.q1SMHrIk094780@svn.freebsd.org> Message-ID: <20120229151223.K2273@besplex.bde.org> References: <201202282217.q1SMHrIk094780@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r232275 - in head/sys: amd64/include i386/include pc98/include x86/include X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Feb 2012 05:01:40 -0000 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 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 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. Bruce