Date: Wed, 5 Nov 2014 07:46:17 +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: <20141105071445.O2183@besplex.bde.org> In-Reply-To: <1415123429.1200.75.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>
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 03:19 +1100, Bruce Evans wrote: >> [...] >> Another unsuitable alignment is by the MD ALIGNBYTES macro. This is >> (sizeof(register_t) - 1) on all arches except mips 32-bit where it is 7 >> sparc64 where it is 15. On arm, it is 3, but arm also has >> STACKALIGNBYTES = 7. The register size is really too small to use for >> malloc() on 32-bit arches. Its technical correctness depends on no >> C objects having more than 32-bit alignment. On i386, int64_t and >> double should have 64-bit alignment, but this is not required unless >> CFLAGS includes -malign-double which breaks the ABI in userland and >> is irrelevant for the kernel. >> > > In arm/include/_align.h we have: > > /* > * Round p (pointer or byte index) up to a correctly-aligned value > * for all data types (int, long, ...). [more words snipped] > */ > #define _ALIGNBYTES (sizeof(int) - 1) > > So that's clearly wrong, because int64_t and double types require 8-byte > alignment. Do they really, except to be optimal? arm64 doesn't seem to be in -current yet. I know little about arm, but seem to remember that it uses 4-byte alignment to a fault and pads out arrays of chars to that. That would make even less sense if some types actually require 8-byte alignment. > When it comes to fixing it, I could: > > * include _types.h and use sizeof(__int64_t) > * use sizeof(long long) > * use sizeof(double) > * just hardcode '7' with a comment that says why > > What are the pros and cons of the various options? Is one of them > preferable? Is there something even better I overlooked? Just hard-code '7' as '(8 - 1)' without even a comment :-). Except most arches (not arm) already have an excessively verbose comment that has been been blindly copied so that it is now wrong in some cases. You would get this by copying bugs from other arches. For sparc64, it is: % * from: @(#)param.h 5.8 (Berkeley) 6/28/91 % * $FreeBSD: head/sys/sparc64/include/_align.h 196994 2009-09-08 20:45:40Z phk $ % */ The headers are convoluted and pessimized by splitting off this tiny header for namespace reasons. When I fixed the namespace bugs for i386, I intentionally didn't split off this header, but used ifdefs instead. (machine/_align.h is also used in sys/socket.h. Keeping the 2 definitions in it in a separate file just allows sys/socket.h to include this file. It used to direct machine/param.h to define only these 2 things.) % % #ifndef _SPARC64_INCLUDE__ALIGN_H_ % #define _SPARC64_INCLUDE__ALIGN_H_ % % /* % * Round p (pointer or byte index) up to a correctly-aligned value % * for all data types (int, long, ...). The result is unsigned int % * and must be cast to any desired pointer type. % */ % #define _ALIGNBYTES 0xf % #define _ALIGN(p) (((u_long)(p) + _ALIGNBYTES) & ~_ALIGNBYTES) Note that the result type is not unsigned int as claimed in the comment. It is only that on 32-bit arches. Also, the type of _ALIGNEDBYTES is a bit fragile. It is signed 32-bit int. So ~ALIGNBYTES is 32 bits. It has value -16, not 0xFFFFFFF0 (the latter is large and unsigned. Then promotion of ~ALIGNBYTES to match the other operand changes its type to u_long and its value to 0xFFFFFFFFFFFFFFF0. The original type had to be signed int and the arithmetic normal 2's complement for the promotion to give the right mask. Even forming ~_ALIGNBYTES requires magic for toggling the sign bit. Since this is the implementation, it can know what happens, but using magic makes its correctness harder to understand. _ALIGNBYTES should probably have type u_long to begin with to reduce the magic. % % #endif /* !_SPARC64_INCLUDE__ALIGN_H_ */ Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141105071445.O2183>