Date: Tue, 04 Nov 2014 14:49:36 -0700 From: Ian Lepore <ian@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r274088 - head/sys/kern Message-ID: <1415137776.1200.113.camel@revolution.hippie.lan> In-Reply-To: <20141105071445.O2183@besplex.bde.org> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 2014-11-05 at 07:46 +1100, Bruce Evans wrote: > 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. > Yep, they do. There are instructions on newer 32-bit arm that operate on 64-bit memory operands and adjacent pairs of registers, and they require 64-bit memory alignment to avoid a fault. Also (unrelated to the ALIGN stuff), the compiler will generate such instructions when operating on pairs of 32-bit values, when it knows the alignment of the memory operand makes it safe to do so. Last year we embraced the newer EABI spec. It requires the stack to be 8-byte aligned at all times except within a leaf function, and the compiler is aware of that. > > 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 > So bottom line, define _ALIGNBYTES as (8UL - 1) to minimize the magic? -- Ian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1415137776.1200.113.camel>