Date: Wed, 1 Jun 2016 01:27:35 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ed Schouten <ed@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r301037 - head/sys/x86/include Message-ID: <20160601002420.E4290@besplex.bde.org> In-Reply-To: <201605311331.u4VDVJsx026326@repo.freebsd.org> References: <201605311331.u4VDVJsx026326@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 31 May 2016, Ed Schouten wrote: > Log: > Implement _ALIGN() using internal integer types. > > The existing version depends on register_t and uintptr_t, which are only > available when including headers such as <sys/types.h>. As this macro is > used by <sys/socket.h>, for example, it should be written in such a way > that it doesn't depend on those types. It was originally carefully written to not have this dependency. This was broken by merging the amd64 and i386 versions. > Modified: head/sys/x86/include/_align.h > ============================================================================== > --- head/sys/x86/include/_align.h Tue May 31 12:39:54 2016 (r301036) > +++ head/sys/x86/include/_align.h Tue May 31 13:31:19 2016 (r301037) > @@ -46,7 +46,7 @@ > * for all data types (int, long, ...). The result is unsigned int > * and must be cast to any desired pointer type. > */ The comment is still broken. It still hard-codes the i386 type, so is wrong for amd64. The orginaly i386 version spelled "unsigned" verbosely as "unsigned int" in the comment but not in the code. The code now uses __uintptr_t. My version uses the following old fixes: X diff -u2 param.h~ param.h X --- param.h~ Thu Apr 8 23:21:42 2004 X +++ param.h Thu Apr 8 23:21:43 2004 X @@ -40,20 +40,12 @@ X /* X * Round p (pointer or byte index) up to a correctly-aligned value X - * for all data types (int, long, ...). The result is unsigned int X - * and must be cast to any desired pointer type. X + * for all data types (int, long, ...). The result is compatible with X + * uintptr_t and must be cast to any desired pointer type. X */ X -#ifndef _ALIGNBYTES X #define _ALIGNBYTES (sizeof(int) - 1) X -#endif X -#ifndef _ALIGN X #define _ALIGN(p) (((unsigned)(p) + _ALIGNBYTES) & ~_ALIGNBYTES) X -#endif X ... The fixes here are: - don't say what the type is. Only say that it is compatible with something - say it is compatible with uintptr_t and don't just echo the implementation's use of unsigned. -current already uses __uintptr_t in the code. - the verbose spelling goes away as a side effect - remove ifdefs. Redefinition with the same value is harmless, and the ifdefs just break detection of incorrect redefinitions. The original amd64 version was more broken here. It used u_long throughout that looks better, but is broken since u_long is in the application namespace. The comment is also broken on arm64. Plain arm was changed recently. It now has slightly better but unnecessarily different wording in the comment. mips uses u_long consistently, but that is probably a type mismatch with size_t. powerpc has the same 2 bugs as x86 had before this fix. riscv only has the wrong comment like arm64 (if it is 64 bits) sparc64 only has the wrong comment. > -#define _ALIGNBYTES (sizeof(register_t) - 1) > -#define _ALIGN(p) (((uintptr_t)(p) + _ALIGNBYTES) & ~_ALIGNBYTES) > +#define _ALIGNBYTES (sizeof(__register_t) - 1) > +#define _ALIGN(p) (((__uintptr_t)(p) + _ALIGNBYTES) & ~_ALIGNBYTES) > > #endif /* !_X86_INCLUDE__ALIGN_H_ */ The existence of this file is another bug. The original version was carefully written to live in <machine/param.h>, although that requires complications to avoid namespace pollution in <sys/socket.h>. Now with the merged amd64 and i386, it takes 2 extra include files instead of only 1 extra just to define 2 macros. First there is <machine/_align.h> which just points here; then there is <x86/_align.h>. Misimplementations like that are one reason for bloated build times (bloat for kernels with no modules since FreeBSD-4 is approximately 3 times in kernel size, 6 times in .depend/number of #include's, and more than 6 for build times. Over nfs mounted without -cto, build times are almost proportional to the number of #include statements unless you have a fast network and break +cto using large -j. <machine/param.h> is too large and complicated to merge, so with the alignment macros back in it, they have to be duplicated. This is a feature. They can use unsigned [long], so you can see what the types are without decoding 10 layers of nested includes and ifdefs. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160601002420.E4290>