kern/151304: patch - definitions of variables tested by ASSERT_ATOMIC_LOAD_PTR

[Bruce Evans]

The following reply was made to PR kern/151304; it has been noted by GNATS.

From: Bruce Evans <brde at optusnet.com.au>
To: Svatopluk Kraus <onwahe at gmail.com>
Cc: freebsd-gnats-submit at freebsd.org, freebsd-bugs at freebsd.org
Subject: Re: kern/151304: patch - definitions of variables tested by
 ASSERT_ATOMIC_LOAD_PTR
Date: Sat, 9 Oct 2010 07:16:43 +1100 (EST)

 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