svn commit: r232261 - in head/sys: amd64/include i386/include pc98/include x86/include

[Tijl Coosemans]

On Wednesday 29 February 2012 00:26:43 Bruce Evans wrote:
> On Tue, 28 Feb 2012, Tijl Coosemans wrote:
> 
>> Log:
>>  Copy amd64 _types.h to x86 and merge with i386 _types.h. Replace existing
>>  amd64/i386/pc98 _types.h with stubs.
> 
> I don't like this much.  It gives 2 layers of convolutions for readers
> (humans and compilers) to slowly untangle.  There is another layer of
> include files for compatibility (but both layers are still used by
> default), and lots of ifdefs.  The whole point of 1 file per arch was
> to avoid such ifdefs.  This might be OK if arches were actually identical
> for the APIs in these files, but for types there are lots of differences
> between 32-bit and 64-bit machines.

amd64 isn't just a 64-bit machine. i386 support is one of its core
features. In hindsight, I believe it was a mistake to create a separate
amd64 include directory. There wouldn't be any stub headers if the
original i386 headers had been extended instead, as was done for
powerpc64 and powerpc. But so be it. Eventually, hopefully, x86 will be
installed as the machine directory and then the stub layer will
disappear.

For now, the approach I'm taking is to only merge headers that can be
included by user code. I don't want to stuff too many changes in one
commit, which means the current batch of patches are pure merges and
little else. The resulting headers are bug for bug compatible with the
original headers. I also try to keep the number of ifdefs down but to
keep diffs small and easier to review I don't want to excessively
rearrange files just to eliminate an extra ifdef. The resulting headers
are meant to be similar to existing powerpc and mips headers. Once
that's done we can look into fixing any bugs and structural problems
for all architectures.

Changing some of the type definitions like you suggest below is
something that can be discussed, but it requires a lot more thought and
testing than what I'm currently trying to accomplish which is to make
cc -m32 work and to prevent further deviations between amd64 and i386
such as __clock_t.

I've also been working on mapping out all the ISO C and POSIX library
requirements which should give me a better idea of how to organise the
headers. Currently the C (and C++) standards are done and I've started
with the various POSIX standards. So far it seems a lot can be moved to
sys because many type definitions (and their limits) aren't machine
dependent but LP64/ILP32 dependent. With several architectures (x86,
powerpc, mips and armv8 in the future?) already supporting both,
there's a lot of duplication that can be removed by supporting both
from sys instead. It would remove some of the stubs like _inttypes.h
and _stdint.h, maybe _limits.h.

>    The differences can be reduced by spelling 32/64-bit types as
>    [unsigned] long and by always using the basic type instead of a
>    derived type.  Old code (e.g., FreeBSD-3) did this, but now almost
>    everything is declared using the derived fixed-width types
>    int32_t/int64_t etc., so there is always a spelling difference for
>    32/64-bit types.  The only exceptions are floating point types, and
>    the broken __clock_t type.
>      clock_t is unsigned long on i386, but is int32_t on amd64.  This
>      is backwards at best.  I think this brokenness came from NetBSD.
>      _BSD_CLOCK_T_ was correct (unsigned long) for all arches in 4.4BSD,
>      and i386 just didn't break this.  But now __clock_t is broken for
>      all arches except i386: it is __uint32_t on arm and powerpc, which
>      is just a different spelling for the 32-bit case and is at least
>      ABI-compatible for the 64-bit case, but for all other arches
>      including all 64-bit ones, it broken to __int32_t.  Perhaps the
>      difference is explained by "long" being bad for ABI compatibility.
>      Old code like 4.4BSD-Lite1 used long excessively (since technically,
>      int might be only 16 bits).  Even pid_t was long in 4.4BSD-Lite1.
>      NetBSD changed many of these longs to ints or int32_t's for ABI
>      compatibility and/or because 64 bit longs are just too wide, and
>      FreeBSD eventually picked up these changes (mostly via 4.4BSD-Lite2
>      for general typedefs and directly from NetBSD for 64-bit arches).
>      So pid_t is now int32_t and clock_t is mostly broken.  clock_t
>      really needs all 64 bits if they are readily available, but has
>      been reduced to 31, especially when 64 are readily available.
>      OTOH, if you just want ABI and API compatibility for clock_t,
>      then it should have been changed to uint32_t for all arches and
>      not defined in any MD types file.  There is now a minor API
>      compatibility for printing clock_t's -- %lu format must be used
>      on i386, %u on others, and %d on most.  Except for the gratuitous
>      loss of unsignedness, this is just a special case of printing
>      a typedefed types.  clock_t may also be a floating point type,
>      so the only good way to print it is to convert it to [long] double
>      and then worry about the correct floating point format (how much
>      precision should it have?...).

The __clock_t case is a perfect example of how duplication leads to
differentiation and differentiation to portability issues. Currently
clock(3) only starts to return negative values after about six months
of cpu time, so I think it's relative safe to change the type to
uint32_t. I haven't checked the other cases where this type is used
though.

>> Copied and modified: head/sys/x86/include/_types.h (from r232259, head/sys/amd64/include/_types.h)
>> ==============================================================================
>> --- head/sys/amd64/include/_types.h	Tue Feb 28 15:52:01 2012	(r232259, copy source)
>> +++ head/sys/x86/include/_types.h	Tue Feb 28 18:15:28 2012	(r232261)
>> @@ -54,19 +54,41 @@ typedef	short			__int16_t;
>> typedef	unsigned short		__uint16_t;
>> typedef	int			__int32_t;
>> typedef	unsigned int		__uint32_t;
>> +#ifdef	_LP64
>> typedef	long			__int64_t;
>> typedef	unsigned long		__uint64_t;
> 
> This is about the only ifdef that is really needed.
> 
>> +#else
>> +#ifndef lint
>> +__extension__
> 
> An old bug -- work around broken lints.  Although messy, this is not
> messy enough to be correct -- __extension__ is a hard-coded gccism.
> Elsewhere, in much less important code than this, there are messy
> ifdefs to avoid hard-coded gccisms.

Maybe define __extension for lint and various compilers in cdefs.h?

>> +#endif
>> +/* LONGLONG */
> 
> long long has only been standard for 13 years now, so broken lints
> still need this messy markup.

Personally, I would just remove lint. Modern compilers produce many
of the same warnings if not more in case of clang. It has been marked
obsolete in SUSv2 (1997) and removed in SUSv3 (2001). And it only
supports C90 so with C99 and now C11 it's becoming more and more
difficult/messy to hide new language features from it.

>> +typedef	long long		__int64_t;
>> +#ifndef lint
>> +__extension__
>> +#endif
>> +/* LONGLONG */
>> +typedef	unsigned long long	__uint64_t;
>> +#endif
> 
> I ifdefed all this correctly 15+ years ago so that it compiled (but
> didn't run if *int64_t was used) for a non-gcc K&R compiler.  The
> long long abomination was not used, and __attribute__(()) was used
> to declare *int64_t, but only under a gcc ifdef.  This wasn't broken
> until 8 Jan 2011 by, erm, us.  We also added the __extensions__.
> The justification was that long long is now standard.  But there
> are still the old messes for lint, and new breakage for non-gcc to
> support C90.

I don't remember the exact details but I think the non-gcc case already
used long long and we made the gcc case use long long as well because
LL and ULL suffixes were used in limits and INT64_C macros anyway.

>> /*
>>  * Standard type definitions.
>>  */
>> +#ifdef	_LP64
>> typedef	__int32_t	__clock_t;		/* clock()... */
>> typedef	__int64_t	__critical_t;
>> typedef	double		__double_t;
>> typedef	float		__float_t;
>> typedef	__int64_t	__intfptr_t;
>> -typedef	__int64_t	__intmax_t;
>> typedef	__int64_t	__intptr_t;
>> +#else
>> +typedef	unsigned long	__clock_t;
>> +typedef	__int32_t	__critical_t;
>> +typedef	long double	__double_t;
>> +typedef	long double	__float_t;
>> +typedef	__int32_t	__intfptr_t;
>> +typedef	__int32_t	__intptr_t;
>> +#endif
> 
> [unsigned] long would work without ifdefs for everything except to
> preserve the broken __clock_t, and the FP types.  Except for i386's
> with correctly-sized longs (64 bits).  We may have discussed these
> too.
> 
>> @@ -75,6 +97,7 @@ typedef	__int8_t	__int_least8_t;
>> typedef	__int16_t	__int_least16_t;
>> typedef	__int32_t	__int_least32_t;
>> typedef	__int64_t	__int_least64_t;
>> +#ifdef	_LP64
>> typedef	__int64_t	__ptrdiff_t;		/* ptr1 - ptr2 */
>> typedef	__int64_t	__register_t;
>> typedef	__int64_t	__segsz_t;		/* segment size (in pages) */
>> @@ -82,8 +105,18 @@ typedef	__uint64_t	__size_t;		/* sizeof(
>> typedef	__int64_t	__ssize_t;		/* byte count or error */
>> typedef	__int64_t	__time_t;		/* time()... */
>> typedef	__uint64_t	__uintfptr_t;
>> -typedef	__uint64_t	__uintmax_t;
>> typedef	__uint64_t	__uintptr_t;
>> +#else
>> +typedef	__int32_t	__ptrdiff_t;
>> +typedef	__int32_t	__register_t;
>> +typedef	__int32_t	__segsz_t;
>> +typedef	__uint32_t	__size_t;
>> +typedef	__int32_t	__ssize_t;
>> +typedef	__int32_t	__time_t;
>> +typedef	__uint32_t	__uintfptr_t;
>> +typedef	__uint32_t	__uintptr_t;
>> +#endif
> 
> [unsigned] long would work without ifdefs for all of these, since all
> these expanded naturally to the register width.  Perhaps a better way,
> which also works for i386's with correctly-sized longs, is to define
> almost everything in terms of registers -- as __[u]register_t.

register_t is a machine dependent type and the others are LP64/ILP32
dependent, so they're not strictly related. MIPS N32 is an example of
that.

As for using long, I'm not sure. It has the same size as int, but they
aren't the same in format strings.

>> +typedef	__uint64_t	__uintmax_t;
>> typedef	__uint32_t	__uint_fast8_t;
>> typedef	__uint32_t	__uint_fast16_t;
>> typedef	__uint32_t	__uint_fast32_t;
>> @@ -92,12 +125,23 @@ typedef	__uint8_t	__uint_least8_t;
>> typedef	__uint16_t	__uint_least16_t;
>> typedef	__uint32_t	__uint_least32_t;
>> typedef	__uint64_t	__uint_least64_t;
>> +#ifdef	_LP64
>> typedef	__uint64_t	__u_register_t;
>> typedef	__uint64_t	__vm_offset_t;
>> -typedef	__int64_t	__vm_ooffset_t;
>> typedef	__uint64_t	__vm_paddr_t;
>> -typedef	__uint64_t	__vm_pindex_t;
>> typedef	__uint64_t	__vm_size_t;
>> +#else
>> +typedef	__uint32_t	__u_register_t;
>> +typedef	__uint32_t	__vm_offset_t;
>> +#ifdef PAE
>> +typedef	__uint64_t	__vm_paddr_t;
>> +#else
>> +typedef	__uint32_t	__vm_paddr_t;
>> +#endif
>> +typedef	__uint32_t	__vm_size_t;
>> +#endif
>> +typedef	__int64_t	__vm_ooffset_t;
>> +typedef	__uint64_t	__vm_pindex_t;
> 
> Similarly.  The patch, and possibly the ifdefs, are hard to read here.
> There's a nested ifdef for PAE.  PAE doesn't apply for amd64.  The
> above assumes that the cases where it doesn't apply are classified
> by !_LP64.

I'm not sure yet about what should happen with these vm types. I'm not
entirely convinced yet that all these "invisible" types are really
necessary at all.

> x86/include didn't have many files in it before this and similar
> commits in the same batch, and the first file that I looked at in
> it has various new and old convolutions and bugs:
> 
> x86/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.
> %  */
> 
> This comment was blindly copied from i386.  It doesn't match the
> code below.
> 
> % #define	_ALIGNBYTES	(sizeof(register_t) - 1)
> % #define	_ALIGN(p)	(((uintptr_t)(p) + _ALIGNBYTES) & ~_ALIGNBYTES)
> 
> This code is broken, since it uses register_t and uintptr_t which are not
> necessarily defined here.  Old code was careful to use only basic types,
> so that no undocumented prerequisites are required and so that this not
> broken when the undocumented prerequisites are not accidentally supplied.
> 
> The type of the result is actually uintptr_t.  On i386, uintptr_t is
> just a different spelling of unsigned int.  On amd64, it is different.
> 
> This file shouldn't exist.
> 
> {amd64,i386,pc98}/include/_align.h just include x86/include/_align.h.
> They shouldn't exist either.

I agree, but I want to finish mapping out the various POSIX standards
first before looking into where the FreeBSD specific headers fit in.

> i386/include/param.h:
> % #include <machine/_align.h>
> 
> This include is misplaced (outside of the idempotency ifdef for this
> file).  The definitions used to be here for technical reasons in
> their correct implementation.  There used to be an ifdef here so that
> this file could be included to define only the alignment macros so
> as to not get namespace pollution in unusual cases.  This was
> "cleaned up" for the unusual cases to pessimize the usual cases.
> The cleanups have rotted, so they now have not just 1, but 2 layers
> of nested includes of _align.h to untangle to see what this file is
> doing.  2 layers give about the same level of obfuscation as the
> correct implementation, plus more inefficiencies than only 1 layer.
> 
> % 
> % #ifndef _I386_INCLUDE_PARAM_H_
> % #define	_I386_INCLUDE_PARAM_H_
> % 
> % /*
> %  * Machine dependent constants for Intel 386.
> %  */
> % 
> % /*
> %  * 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.
> %  */
> 
> The comment is still correct for the definitions here.
> 
> % #ifndef _ALIGNBYTES
> % #define _ALIGNBYTES	(sizeof(int) - 1)
> % #endif
> % #ifndef _ALIGN
> % #define _ALIGN(p)	(((unsigned)(p) + _ALIGNBYTES) & ~_ALIGNBYTES)
> % #endif
> 
> But these definitions are unreachable, since <machine/_align.h> is
> included unconditionally, and it defines the alignment macros
> unconditionally.
> 
> % 
> %
> 
> Mistakes near here also added this style bug (extra blank line).
> 
> % #define __HAVE_ACPI
> 
> amd64/include/param.h:
> %  * $FreeBSD: src/sys/amd64/include/param.h,v 1.37 2011/07/19 13:00:30 attilio Exp $
> %  */
> % 
> %
> 
> Extra blank lines are scattered randomly and happen to be in different
> places.
> 
> % #ifndef _AMD64_INCLUDE_PARAM_H_
> % #define	_AMD64_INCLUDE_PARAM_H_
> % 
> % #include <machine/_align.h>
> 
> Unlike for i386, this is placed almost correctly.  It is inside the
> idempotency ifdef...
> 
> % 
> % /*
> %  * Machine dependent constants for AMD64.
> %  */
> 
> ... but not inside the comment that should describe everything in this
> file.
> 
> % 
> %
> 
> Another random extra blank line.  Or not so random.
> 
> Unlike for i386, the old definitions are not kept here.  They used to
> be placed up where the include is now on i386 (outside of the idempotency
> ifdef).  They used to use u_long to avoid prerequisites and have a
> correct comment for them.  For i386 there are now 2 correct comments
> about them (but with confusing spelling in one), while for amd64 there
> is only 1 incorrect comment.
> 
> % ...
> % #define	ALIGNBYTES		_ALIGNBYTES
> % #define	ALIGN(p)		_ALIGN(p)
> 
> These (and ALIGNED_POINTER()) are the primary APIs for these macros
> (the APIs with underscores are just to keep them out of the namespace
> in POSIX headers).  The above definitions of the primary APIs are
> completely MI once the underscored versions are defined, so they
> shouldn't be defined here or in any other MD header.
> 
> Just about all of the amd64 and i386 param.h are the same, although
> not as MI as ALIGNBYTES etc.  I don't really want the definitions
> moved to x86 though.  They are bad enough where they are.

OK. I don't think this header is important to user code.

> Other bugs in them include:
> - amd64_btop() and amd64_ptob() are named gratuitously differently
>    than i386_btop() and i386_ptob().  Names like md_btop() and
>    md_btop() or just btop() and btop() would be better.  Not having
>    these APIs might be better still.  There is considerable confusion
>    between these APIs and others that give the same results, and since
>    the results are the same and there is no type checking, it is
>    unlikely that the logically correct API is always used:
>    - the corresponding "MI" APIs are ctob() and btoc().  These are
>      ancient.  'c' in them means 'clicks' (groups of pages).  Groups
>      of more than one page have never been used in FreeBSD.  The
>      implementation of these macros is sort of backwards, with the
>      assumption that the group size is always 1 page hard-coded in
>      their definitions via PAGE_MASK and PAGE_SHIFT.
>    - atop() and ptoa() may also be "MI", but have MD implementations.
>      They are confusingly similar to ctob() and btoc().  'p' (page)
>      in them always means the same as 'c' (clicks) in practice.  'a'
>      (address) in them always means the same as 'b' (address in bytes,
>      or size in bytes) in practice.  The logical differences are
>      subtle.  There isn't even a MI API like atop() for for sizes
>      instead of addresses.
>    - {amd64,i386}_btop() and {amd64,i386}_ptob() logically handle either
>      sizes or addresses, and convert to and from pages.  They are the
>      easiest to use provided you assume that the address space is flat
>      and that the pages aren't grouped into clicks, but they are MD
>      and have the worse spelling.  This is sort of backwards again.  It
>      is MD layers that should use shorter spelling of their variables
>      and APIs, and hard-code assumptions.  i386 mostly uses atop(), and
>      most of these uses are doubly logically wrong, since they are
>      mostly for sizes and not for addresses, amd are very MD:
>      - ctob: 13 instances
>      - btoc: 0
>      - atop: 34 instances (counting its definition).  Most are for segment
>        limits, this are for sizes, thus are abuses.
>      - ptoa: 13 ... mostly for sizes.  Confusion between this and ctob is
>        apparently perfectly divided.
>      - i386_ptob: 3 instances.  Just 1 use in pmap.c duplicated for xen.
>      - i386_btop: 9 instances.  Just 4+2 uses in pmap.c's, 2 in pmap.h
>        (in a macros and an inline that expand to many more uses)
> 
> One of the reasons for putting all types declarations in the same file
> although this gives inefficiencies by requiring the compiler to read
> and parse very large files to get the few definitions that are actually,
> is to avoid the convolutions and ifdefs for minimising the declarations.
> Convolutions and ifdefs for other purposes defeat this to some extent.
> Although I was responsible for some of the tiny _foo.h files like
> <sys/_mutex.h> which are used to minimise namespace problems, I don't
> like the profileration of such files to handle even tinier problems.
> The worst that I have noticed are _null.h (this should be handled in
> machine/_types.h, where it used to be possible to handle it without
> ifdefs), _align.h, and *stdint.h (there are now 4 layers of convolutions
> with internal branches for <stdint.h>:
> - /usr/include/stdint.h -> sys/stdint.h (only a symlink)
>    - sys/stdint.h includes sys/_stdint.h and machine/_stdint.h
>      - sys/_stdint.h is a mistake to allow sys/types.h to include it
>        instead of declaring historical pollution for itself, but only
>        for some pollution that is now standard in <stdint.h> -- types.h
>        still declares things like u_int8_t unconditionally for itself;
>        the handling of these is a bit simpler because they don't need
>        ifdefs to prevent re-declaration in <stdint.h>.  Now it is harder
>        to see what types.h declares.
>      - machine/_stdint.h was the correct implementation.  Now it includes
>        x86/_stdint.h on amd64 and i386.
>        - x86/_stdint.h.  Most of the details are now here, 4 layers deep,
>  	with ifdefs.
> 
> Ideally, everything would be in <stdint.h> directly.  I don't know how
> to do it in a layer, but is it too much to ask for only 2 layers?
> Hmm, I do know how to do it in 1 layer -- flatten it at install time.
> Anyone who cared about efficiency and readability would do this :-).
> But I usually read the uninstalled sources.

If you allow ifdef _LP64 in sys/stdint.h everything in machine/_stdint.h
could be moved there, except the sig_atomic_t limits which seems to be
another __clock_t case.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 228 bytes
Desc: This is a digitally signed message part.
Url : http://lists.freebsd.org/pipermail/svn-src-all/attachments/20120301/0b34c80b/attachment.pgp