Date: Thu, 6 Nov 2014 08:04:04 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ian Lepore <ian@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r274088 - head/sys/kern Message-ID: <20141106063913.V1296@besplex.bde.org> In-Reply-To: <1415137776.1200.113.camel@revolution.hippie.lan> References: <201411041129.sA4BTnwX030600@svn.freebsd.org> <20141104114041.GA21297@dft-labs.eu> <20141105023323.O1105@besplex.bde.org> <1415123429.1200.75.camel@revolution.hippie.lan> <20141105071445.O2183@besplex.bde.org> <1415137776.1200.113.camel@revolution.hippie.lan>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 4 Nov 2014, Ian Lepore wrote: > On Wed, 2014-11-05 at 07:46 +1100, Bruce Evans wrote: >> ... >> _ALIGNBYTES should probably have type u_long to begin with to reduce the >> magic. >> ... > > So bottom line, define _ALIGNBYTES as (8UL - 1) to minimize the magic? 8UL on 64-bit arches and 8U on 32-bit arches. The second is required to avoid surprises and not break the comment which says that _ALIGN() has type unsigned int. Except the comment is often broken. All cases are now broken in some way, including some that were correct except for style bugs in FreeBSD-[4-5] :-(, The less-broken cases use 'unsigned long' or 'unsigned int' in the definition of _ALIGN() so as to avoid namespace pollution. This type should be identical to uintptr_t, and must be equivalent to uintptr_t. The documentation of the type in the comment should match this. The current (and some previous) brokenness is: amd64: This was broken in FreeBSD-[5-8] by spelling uintptr_t as u_long in the code and comments. Using u_long gives namespace pollution and thus defeats the reason for existence of _align.h, but FreeBSD-[5-8] didn't use _align.h. The namespace pollution was moved by merging with the i386 _align.h. The u_long in the code is now spelled uintptr_t. The long in the code is now spelled register_t. The u_long in the comment is now just wrong for amd64. It is now misspelled unsigned int, after copying the comment from i386 where it was correct. arm: Correct, except for the style bug of spelling the type verbosely as unsigned int in the comment. Only 32 bits is supported, and uintptr_t is always uint32_t == unsigned, so there are no complications. i386, pc98: This was correct in FreeBSD[4-8] except for the same style bug as arm. (I changed tyhe comment to say uintptr_t in my version. This spelling doesn't cause any namespace problems when it is in a comment.) i386 now uses the same code as amd64. The type in the comment remains correct, but the types in the code give namespace pollution. mips This still uses the old alpha/NetBSD (arch) code and spells the type as u_long in the comment and the code. This is more broken than before, since u_long is incompatible with the type of uintptr_t in the 32-bit case. uintptr_t == uint32_t == plain unsigned then. It gives the same namespace pollution as on old amd64 (which was also closer to alpha than to i386). More details on the pollution: u_long is not even defined when in the case where _align.h does something useful (when sys/socket.h is compiled with options for strict POSIX conformance). powerpc: Same as amd64. More details on the namsespace pollution: types ending in _t are in the POSIX namespace in some cases, so uintptr_t and register_t are not quite as polluting as u_long. sparc64: this uses u_long in the code but unsigned int in the comment. It is like old alpha except for the wrong comment. ia64: Now dead. In FreeBSD-10, it was like old alpha except for having ALIGNBYTES = 15 instead of 7. This signed type gives the magic previously discussed. Another archaism from old alpha is spelling '& ~ALIGNBYTES' as '&~ ALIGNBYTES'. All arches except mips and powerpc now avoid the signed magic by spelling _ALIGNBYTES using sizeof(sizeof(foo_t)). sizeof() gives type size_t without spelling it as such, thus has no namespace problems in itself, and also gives the correct type (u_int or u_long) on all supported arches. However, using foo_t gives namespace problems. The namespace problems can also be avoided by including <machine/_types.h> and using __foo_t instead of foo_t. I don't like that much, though its overhead is small since almost all headers end up including <machine/_types.h>. Using literal constants also has the advantage of making _ALIGNBYTES usable in cpp expressions. Since it is an implementation detail, this is not very important. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141106063913.V1296>