Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 16 Dec 2011 01:12:16 +1100 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Ed Schouten <ed@80386.nl>
Cc:        svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Bruce Evans <brde@optusnet.com.au>
Subject:   Re: svn commit: r228495 - head/sys/sys
Message-ID:  <20111216000117.R2632@besplex.bde.org>
In-Reply-To: <20111215122047.GN1771@hoeg.nl>
References:  <201112140909.pBE99bS3090646@svn.freebsd.org> <20111214234615.B3839@besplex.bde.org> <20111215122047.GN1771@hoeg.nl>

next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 15 Dec 2011, Ed Schouten wrote:

> Hello Bruce,
>
> After reading through your email, I think it's best to fix things using
> the following patch. Essentially it does the following:
>
> - It makes sure __alignof() is always present, by defining it as a macro
>  for GCC < 2.95.

I tested this with a K&R compiler.  It worked (since it is similar to
the fallback for offsetof() which always worked) after fixing 2 bugs
that prevented the fallback working with any compiler.

>From your patch (quoted below):

> | +#if !__GNUC_PREREQ__(2, 95)
> | +#define	__alignof(x)	__offsetof(struct { char __a; e __b; }, __b)
> | +#endif

The 2 bugs are:
1. Even __offsetof() is broken in the fallback case (that's all cases
    where it uses a struct like the above and doesn't use a gccish builtin).
    This is because __offsetof() uses size_t, but size_t is not necessarily
    in declared.  offsetof() in <stddef.h>, etc. works because size_t is
    necessarily declared there.  BTW, the ifdef for __offsetof in
    <stddef.h> is bogus since <sys/cdefs.h> is always included so
    __offsetof is always defined.

    __size_t could be used, but that would give lots of pollution for
    cdefs.h, and the file that declares __size_t, namely
    <machine/types.h> is broken in fallback cases:
    - recently broken for K&R since it now uses signed instead of __signed
    - broken for longer for K&R and C90 since it uses long long.  In working
      versions, the __int64_t declarations were hacked for 32-bit machines
      on to make them compile (but not work).  Non-hacked versions should
      simply not declare the 64-bit types if the compiler doesn't support
      them.
2. The macro parameter x is now mispelled, so it is never used, and the
    non-misspelled macro parameter e gives a syntax error when it is
    used in the macro body.

Test program:

% #include <sys/cdefs.h>
% 
% typedef	unsigned size_t;	/* XXX */
% 
% #define	__alignof(e)	__offsetof(struct { char __a; e __b; }, __b)
% 
% int c = __alignof(char);
% int s = __alignof(short);
% int i = __alignof(int);
% int l = __alignof(long);
% int f = __alignof(float);
% int d = __alignof(double);

Fixes:
1. Just a hack to make __offsetof() work.  I don't know a good way to
    declare __size_t.  Of course, size_t should not be used.
2. Change the spelling back to e.  I prefer x, but all the other
    definitions use e.

> - All the C1X macros can now be implemented using the ones we already
>  had (__dead2, etc). This means we don't need to repeat all the
>  compiler version specific checks.
> - While there, add struct __hack to the _Static_assert, as it always
>  requires a semicolon.

Good.

> | Index: sys/sys/cdefs.h
> | ===================================================================
> | --- sys/sys/cdefs.h	(revision 228504)
> | +++ sys/sys/cdefs.h	(working copy)
> | @@ -230,27 +230,24 @@
> |  #elif defined(__STDC_VERSION__) && __STDC_VERSION__ > 201000L
> |  /* Do nothing.  They are language keywords. */
> |  #else
> | -/* Not supported.  Implement them manually. */
> | -#ifdef __GNUC__
> | -#define	_Alignas(e)		__attribute__((__aligned__(e)))
> | -#define	_Alignof(e)		__alignof__(e)
> | -#define	_Noreturn		__attribute__((__noreturn__))
> | +/* Not supported.  Implement them using our versions. */

It may be worth emphasizing that in some cases, "our version" doesn't
exist, and that this is intentional (we omit the definition when none
is possible, and this results in __aligned() sometimes not being defined
and __thread never being defined).

> | +#define	_Alignas(e)		__aligned(e)
> | +#define	_Alignof(e)		__alignof(e)

I prefer to always spell the parameter as x.  Most older macros in this
file, and CTASSERT() in another file, spell their parameters (x, y, ...).

> | +#define	_Noreturn		__dead2
> |  #define	_Thread_local		__thread
> | -#else
> | -#define	_Alignas(e)
> | -#define	_Alignof(e)		__offsetof(struct { char __a; e __b; }, __b)
> | -#define	_Noreturn
> | -#define	_Thread_local
> | -#endif
> |  #ifdef __COUNTER__
> |  #define	_Static_assert(e, s)	__Static_assert(e, __COUNTER__)
> |  #define	__Static_assert(e, c)	___Static_assert(e, c)
> |  #define	___Static_assert(e, c)	typedef char __assert ## c[(e) ? 1 : -1]
> |  #else
> | -#define	_Static_assert(e, s)
> | +#define	_Static_assert(e, s)	struct __hack

CTASSERT() has regressed for compilers that don't support __COUNTER__,
since it uses this.  Previously:
- CTASSERT() never worked for K&R compilers, since it uses C90 token pasting
- CTASSERT() worked for all C90 and later compilers.

> |  #endif
> |  #endif
> |
> | +#if !__GNUC_PREREQ__(2, 95)
> | +#define	__alignof(x)	__offsetof(struct { char __a; e __b; }, __b)
> | +#endif

Needs the spelling fix.

> | +
> |  #if __GNUC_PREREQ__(2, 96)
> |  #define	__malloc_like	__attribute__((__malloc__))
> |  #define	__pure		__attribute__((__pure__))

Check the sorting of the new macros, especially __alignof().  I think
the ordering should be mostly on the gcc and standards version, not on
the order in which the macros were added to cdefs.h.  You sorted the 2.95
ifdef before a 2.96 ifdef, but this 2.96 ifdef is especially disordered
according to my rule.  Even if the order is "logical" instead of historical,
it makes more sense to define basic macros like __alignof() before using
them.

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111216000117.R2632>