Date: Sat, 9 Oct 2010 07:16:43 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Svatopluk Kraus <onwahe@gmail.com> Cc: freebsd-bugs@freebsd.org, freebsd-gnats-submit@freebsd.org Subject: Re: kern/151304: patch - definitions of variables tested by ASSERT_ATOMIC_LOAD_PTR Message-ID: <20101009064440.Q1138@besplex.bde.org> In-Reply-To: <201010081226.o98CQbBI024222@www.freebsd.org> References: <201010081226.o98CQbBI024222@www.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 8 Oct 2010, Svatopluk Kraus wrote:
>> Description:
> A macro ASSERT_ATOMIC_LOAD_PTR() hits in colfire port I work on. It is possibly due to use of GNU GCC (4.5.1) compiler -Os option (size optimalization). The macro is applied on four places:
Perhaps gcc-4.5.1 -Os is doing invalid packing of structs, or FreeBSD has
broken packing of structs using the __packed mistake and gcc-4.5.1 -Os
exposes the brokenness by exploiting __packed more. Probably the latter.
BTW, gcc-4.2.1 -Os is still completely broken on kernels. It fails to
compile some files due to problems with -Wuninitialized, and when this
is worked around it produces a kernel that is about 50% larger than one
produced by gcc-4.2.1 -O. A few years ago I thought the problem was
excessive inlining, but when I tried to fix it a few weeks ago, reducing
the inlining caused larger problems and still gave large negative
optimizations.
>> Fix:
> I patch the entries definitions in structures by align attribute, I believe it is better than to do nothing. Moreover, it solved my problem.
>
> Patch attached with submission follows:
>
> Index: sys/sys/_rwlock.h
> ===================================================================
> --- sys/sys/_rwlock.h (revision 213567)
> +++ sys/sys/_rwlock.h (working copy)
> @@ -37,7 +37,7 @@
> */
> struct rwlock {
> struct lock_object lock_object;
> - volatile uintptr_t rw_lock;
> + volatile uintptr_t rw_lock __aligned(4);
> };
>
> #endif /* !_SYS__RWLOCK_H_ */
> ...
This does nothing good on arches with 64-bit pointers. With gcc-4.2.1 on
amd64, it seems to do nothing -- the natural alignment of a uintptr_t on
amd64 is 8, and asking for a smaller alignment using __aligned(4) apparently
makes no difference. It takes __aligned(more_than_8) or __packed to make a
difference.
Here is an example of creating an alignment bug due using __packed and just
gcc-4.2.1 on amd64:
% #include <sys/cdefs.h>
% #include <stddef.h>
%
% struct foo {
% int x;
% long y __aligned(4);
% };
%
% struct bar {
% char c;
% struct foo d;
% } __packed;
%
% int z = offsetof(struct foo, y);
% int t = sizeof (struct bar);
'y' has the correct offset of 8, but `struct bar' has size 17, so the `y'
nested in it cannot possibly be aligned properly, at least if there is an
array of `struct bar' with at least 2 elements. I think this is a compiler
bug -- the explicit __aligned(4) in the nested struct should force alignment
of container structs (but only to 4 here, not the 8 that is required).
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20101009064440.Q1138>