Date: Fri, 29 May 2015 17:40:16 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Brooks Davis <brooks@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r283671 - head/sys/sys Message-ID: <20150529154745.F900@besplex.bde.org> In-Reply-To: <201505282206.t4SM66Xj090527@svn.freebsd.org> References: <201505282206.t4SM66Xj090527@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 28 May 2015, Brooks Davis wrote:
> Log:
> Revert r102953
>
> The bitfile padding was always unallocated on real-world FreeBSD systems and
> depended on the assumption that (abs(sizeof(long) - sizeof(char*)) <= 32).
Actually, it was bit-field padding that depended on the assumption that
(abs(sizeof(long) - sizeof(char*)) <= CHAR_BIT * sizeof(int).
It did work under this assumption, and was needed on non-real-world FreeBSD
systems with correctly-sized longs. Why break it?
A correctly-sized long is twice as large as a machine register, so it is 64
bits on i386 and 128 bits on amd64. The 32 bits extra for this on i386 was
supported by the padding, but the 64-bits extra for this on amd64 is too
many.
Compilers now support an __int128_t type on amd64. This is the correctly-
sized long for amd64. But it is too difficult to use since it is not
spelled "long". There is null support for printing this type, and it
being even longer than intmax_t must cause problems (e.g., casting it to
intmax_t for printing it doesn't work). This is unusable, and not permitted
by standardards (intmax_t is specified to be capable of representing any
value of _any_ signed integer type, although it is not required to have
rank >= that of any signed integer type). Expanding intmax_t to 128 bits
to support this type would cause larger problems since it would break
all ABIs that use intmax_t and pessimize most internal uses of intmax_t.
Similar bit-field padding in <sys/stat.h> is more important and depends
on even more relationships between type sizes, but handles up to 64 bits
of padding using 2 bit-fields.
BTW, someone broke namespaces in <sys/stat.h>. Old versions were careful
to use struct __timespec in the !__BSD_VISIBLE case. Some time after 2001,
POSIX itself was broken to allow (but not require) <sys/stat.h> to declare
struct timespec. FreeBSD was changed to take advantage of this
simplification, but by declaring and using struct timespec unconditionaly
it broke support for older versions of POSIX that don't allow this, and
turned its delicately layered includes into nonsense instead of over-
engineering.
Breaking <sys/stat.h> removed just 1 set of delicate bit-field padding.
Many cases were handled by ignoring the problem and/or living with
historical behaviour: from sys/stat.h:
X #if __BSD_VISIBLE
X struct timespec st_atimespec; /* time of last access */
X struct timespec st_mtimespec; /* time of last data modification */
X struct timespec st_ctimespec; /* time of last file status change */
X #else
X time_t st_atime; /* time of last access */
X long st_atimensec; /* nsec of last access */
X time_t st_mtime; /* time of last data modification */
X long st_mtimensec; /* nsec of last data modification */
X time_t st_ctime; /* time of last file status change */
X long st_ctimensec; /* nsec of last file status change */
X #endif
Here the nsec fields aren't really usable in the !__BSD_VISIBLE case.
They are essentially just sloppy padding using longs. This happens to
work on all supported arches. If time_t is 64 bits and long is 32 bits,
or vice versa, there may be padding from 96 total bits to 128, but it is
consistent.
X off_t st_size; /* file size, in bytes */
X __int64_t st_blocks; /* blocks allocated for file */
X __uint32_t st_blksize; /* optimal blocksize for I/O */
X fflags_t st_flags; /* user defined flags for file */
X __uint32_t st_gen; /* file generation number */
X __int32_t st_lspare;
X #if __BSD_VISIBLE
X struct timespec st_birthtimespec; /* time of file creation */
X /*
X * Explicitly pad st_birthtimespec to 16 bytes so that the size of
X * struct stat is backwards compatible. We use bitfields instead
X * of an array of chars so that this doesn't require a C99 compiler
X * to compile if the size of the padding is 0. We use 2 bitfields
X * to cover up to 64 bits on 32-bit machines. We assume that
X * CHAR_BIT is 8...
X */
X unsigned int :(8 / 2) * (16 - (int)sizeof(struct timespec));
X unsigned int :(8 / 2) * (16 - (int)sizeof(struct timespec));
X #else
X time_t st_birthtime; /* time of file creation */
X long st_birthtimensec; /* nsec of file creation */
X unsigned int :(8 / 2) * (16 - (int)sizeof(struct __timespec));
X unsigned int :(8 / 2) * (16 - (int)sizeof(struct __timespec));
X #endif
Breaking the ifdef removed the second set of padding, but birthtimes
still need explicit padding in the struct case here and in struct
nstat (BTW, struct nstat is garbage. It has something to do with
NetBSD compatibility, but with no other support for NetBSD compatibility
struct nstat is unsable and unused).
Someone, probably me, was more careful here than in nlist_aout.h.
Apparently the padding actually needs to be wider than 32 bits in cases
of interest. This is handled by using 2 bit-fields. The assumption
that CHAR_BIT is 8 is guaranteed in later versions of POSIX.
> Modified:
> head/sys/sys/nlist_aout.h
>
> Modified: head/sys/sys/nlist_aout.h
> ==============================================================================
> --- head/sys/sys/nlist_aout.h Thu May 28 22:01:50 2015 (r283670)
> +++ head/sys/sys/nlist_aout.h Thu May 28 22:06:05 2015 (r283671)
> @@ -56,8 +56,6 @@ struct nlist {
> } n_un;
> #else
> const char *n_name; /* symbol name (in memory) */
> - int : 8 * (sizeof(long) > sizeof(char *) ?
> - sizeof(long) - sizeof(char *) : sizeof(char *) - sizeof(long));
The complication for unions can probably be handled by unnamed unions
or just C99 initializers, but old aout code shouldn't depend on new
features like that. Unnamed unions are probably still too nonstandard
to use in any header visible to applications. I thought that they
were a standard feature, but when I tried to use them recently, I
found that the gcc syntax for them was limited, and you have to use
-fms-extensions to get more usable support, but that is more unportable.
> #endif
> unsigned char n_type; /* type defines */
> char n_other; /* ".type" and binding information */
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150529154745.F900>