[06/13] Installed header hygiene (BZ#20366): Macros used in #if without checking whether they are defined.
Commit Message
sysdeps/unix/sysv/linux/bits/socket.h wants to know whether __flexarr
will produce a real flexible array member -- specifically, one that
doesn't alter sizeof(the structure containing it). Rather than make
its approximation to that condition any more complicated, I added a
new macro to sys/cdefs.h, __flexarr_is_fake, which reveals exactly
what it wants to know. I also took the opportunity to flatten the
rather messy conditional nest defining __flexarr.
It seems to me that _LIBC should not appear in installed headers, but
avoiding that for argp specifically would require more surgery than
feels appropriate for this patch set. It's possible that
"#ifdef _LIBC" would be sufficient, but I wanted to be conservative.
* argp/argp.h: Check whether _LIBC is defined before expanding it.
* posix/glob.h: Check whether __USE_XOPEN2K8 is defined instead
of expanding it.
* misc/sys/cdefs.h: Tidy up conditional nest defining __flexarr.
Define __flexarr_is_fake when the compiler does not support
flexible array members.
* sysdeps/unix/sysv/linux/bits/socket.h: Use __flexarr_is_fake
in definitions of struct cmsghdr and CMSG_DATA.
---
argp/argp.h | 4 ++--
misc/sys/cdefs.h | 27 +++++++++++++++------------
posix/glob.h | 2 +-
sysdeps/unix/sysv/linux/bits/socket.h | 4 ++--
4 files changed, 20 insertions(+), 17 deletions(-)
Comments
On 08/29/2016 09:16 PM, Zack Weinberg wrote:
> sysdeps/unix/sysv/linux/bits/socket.h wants to know whether __flexarr
> will produce a real flexible array member -- specifically, one that
> doesn't alter sizeof(the structure containing it). Rather than make
> its approximation to that condition any more complicated, I added a
> new macro to sys/cdefs.h, __flexarr_is_fake, which reveals exactly
> what it wants to know. I also took the opportunity to flatten the
> rather messy conditional nest defining __flexarr.
>
> It seems to me that _LIBC should not appear in installed headers, but
> avoiding that for argp specifically would require more surgery than
> feels appropriate for this patch set. It's possible that
> "#ifdef _LIBC" would be sufficient, but I wanted to be conservative.
>
> * argp/argp.h: Check whether _LIBC is defined before expanding it.
> * posix/glob.h: Check whether __USE_XOPEN2K8 is defined instead
> of expanding it.
>
> * misc/sys/cdefs.h: Tidy up conditional nest defining __flexarr.
> Define __flexarr_is_fake when the compiler does not support
> flexible array members.
> * sysdeps/unix/sysv/linux/bits/socket.h: Use __flexarr_is_fake
> in definitions of struct cmsghdr and CMSG_DATA.
This patch fails to follow the Macro API best practice that all macros
be defined and that we should check for their values not their defined-ness
which can introduce subtle errors e.g. __flexarr_is_fake.
Please see:
https://sourceware.org/glibc/wiki/Wundef
At a high level I would expect _LIBC to always be defined as either 0 or 1.
On Wed, 21 Sep 2016, Carlos O'Donell wrote:
> At a high level I would expect _LIBC to always be defined as either 0 or 1.
_LIBC is effectively with external code, because it's used (with #if) in
code shared by gnulib. So we can't change its semantics like that;
defining to 0 with installed glibc would break building gnulib.
On Wed, Sep 21, 2016 at 2:02 PM, Carlos O'Donell <carlos@redhat.com> wrote:
>> * misc/sys/cdefs.h: Tidy up conditional nest defining __flexarr.
>> Define __flexarr_is_fake when the compiler does not support
>> flexible array members.
>
> This patch fails to follow the Macro API best practice that all macros
> be defined and that we should check for their values not their defined-ness
> which can introduce subtle errors e.g. __flexarr_is_fake.
Doh, I should have remembered that. I think I'll also flip the sense
and call it __glibc_c99_flexarr_available. It's a bit of a mouthful
but it makes for a clearer test at the point of use.
> At a high level I would expect _LIBC to always be defined as either 0 or 1.
My expectation is actually that it is either undefined or 1, for
historical reasons, but I wouldn't want to swear that it never gets
defined to be 0.
zw
On 09/21/2016 02:05 PM, Joseph Myers wrote:
> On Wed, 21 Sep 2016, Carlos O'Donell wrote:
>
>> At a high level I would expect _LIBC to always be defined as either 0 or 1.
>
> _LIBC is effectively with external code, because it's used (with #if) in
> code shared by gnulib. So we can't change its semantics like that;
> defining to 0 with installed glibc would break building gnulib.
Isn't that just a normal coordination issue with gnulib?
Changes in GCC routinely break building glibc, either intentional or
uintentional.
In this case there would be a dependency between this glibc version
and the usable versions of gnulib which could be built with that glibc?
I agree that such gratuitous breakage for a weak reason like installed
header hygiene would be a bad idea.
However, I don't think we should avoid the change, but rather coordinate
with gnulib if we ever need it.
On 09/21/2016 11:40 AM, Carlos O'Donell wrote:
> Isn't that just a normal coordination issue with gnulib?
I think so. Gnulib could readily change to follow whatever _LIBC
practice is thought to be convenient for libc itself.
On 09/21/2016 02:46 PM, Paul Eggert wrote:
> On 09/21/2016 11:40 AM, Carlos O'Donell wrote:
>> Isn't that just a normal coordination issue with gnulib?
>
> I think so. Gnulib could readily change to follow whatever _LIBC
> practice is thought to be convenient for libc itself.
Thanks for confirming.
Best practice for macro APIs, where possible, is to always define the
macro e.g. set it to 0 or 1, and then enable the use of -Wundef to catch
any mistakes.
This isn't always possible with defines coming from other projects or
the compiler itself (where behaviour is standardized), but in our own
GNU projects I think we could adopt the less error prone usages.
I'm not suggesting Zack tackle this now.
On Wed, 21 Sep 2016, Carlos O'Donell wrote:
> On 09/21/2016 02:05 PM, Joseph Myers wrote:
> > On Wed, 21 Sep 2016, Carlos O'Donell wrote:
> >
> >> At a high level I would expect _LIBC to always be defined as either 0 or 1.
> >
> > _LIBC is effectively with external code, because it's used (with #if) in
> > code shared by gnulib. So we can't change its semantics like that;
> > defining to 0 with installed glibc would break building gnulib.
>
> Isn't that just a normal coordination issue with gnulib?
No. It should be possible to build existing versions of GNU software, and
other packages using gnulib, with new versions of glibc, without needing
to wait possibly years for loads of packages to have new releases with
updated gnulib. Occasionally a new glibc may break a few external
packages and require coordination with them, but we shouldn't do things
that would cause the sort of global breakage of most gnulib-using software
that would result from changing the public _LIBC interface with external
code.
On 09/21/2016 03:59 PM, Joseph Myers wrote:
> On Wed, 21 Sep 2016, Carlos O'Donell wrote:
>
>> On 09/21/2016 02:05 PM, Joseph Myers wrote:
>>> On Wed, 21 Sep 2016, Carlos O'Donell wrote:
>>>
>>>> At a high level I would expect _LIBC to always be defined as either 0 or 1.
>>>
>>> _LIBC is effectively with external code, because it's used (with #if) in
>>> code shared by gnulib. So we can't change its semantics like that;
>>> defining to 0 with installed glibc would break building gnulib.
>>
>> Isn't that just a normal coordination issue with gnulib?
>
> No. It should be possible to build existing versions of GNU software, and
> other packages using gnulib, with new versions of glibc, without needing
> to wait possibly years for loads of packages to have new releases with
> updated gnulib. Occasionally a new glibc may break a few external
> packages and require coordination with them, but we shouldn't do things
> that would cause the sort of global breakage of most gnulib-using software
> that would result from changing the public _LIBC interface with external
> code.
You use the words "should" twice, which matches my expectation here, that
we would like it to work, but by coordinating with gnulib we might change
it in the future given a good enough reason. I call this a coordination
issue with gnulib.
In this situation I see no case to be made for changing _LIBC's behaviour.
I think we are both in agreement. Perhaps I sounded too cavalier in claiming
it was "just" a coordination issue.
@@ -511,7 +511,7 @@ extern void *__argp_input (const struct argp *__restrict __argp,
#ifdef __USE_EXTERN_INLINES
-# if !_LIBC
+# if !(defined _LIBC && _LIBC)
# define __argp_usage argp_usage
# define __argp_state_help argp_state_help
# define __option_is_short _option_is_short
@@ -546,7 +546,7 @@ __NTH (__option_is_end (const struct argp_option *__opt))
return !__opt->key && !__opt->name && !__opt->doc && !__opt->group;
}
-# if !_LIBC
+# if !(defined _LIBC && _LIBC)
# undef __argp_usage
# undef __argp_state_help
# undef __option_is_short
@@ -153,21 +153,24 @@
# define __errordecl(name, msg) extern void name (void)
#endif
-/* Support for flexible arrays. */
-#if __GNUC_PREREQ (2,97)
-/* GCC 2.97 supports C99 flexible array members. */
+/* Support for flexible arrays.
+ Headers that should use flexible arrays only if they're "real"
+ (e.g. only if they won't affect sizeof()) should test
+ #ifndef __flexarr_is_fake. */
+#if defined __STDC_VERSION__ && __STDC_VERSION__ >= 199901L
# define __flexarr []
+#elif __GNUC_PREREQ (2,97)
+/* GCC 2.97 supports C99 flexible array members as an extension,
+ even when in C89 mode or compiling C++ (any version). */
+# define __flexarr []
+#elif defined __GNUC__
+/* Pre-2.97 GCC did not support C99 flexible arrays but did have
+ an equivalent extension with slightly different notation. */
+# define __flexarr [0]
#else
-# ifdef __GNUC__
-# define __flexarr [0]
-# else
-# if defined __STDC_VERSION__ && __STDC_VERSION__ >= 199901L
-# define __flexarr []
-# else
/* Some other non-C99 compiler. Approximate with [1]. */
-# define __flexarr [1]
-# endif
-# endif
+# define __flexarr [1]
+# define __flexarr_is_fake 1
#endif
@@ -25,7 +25,7 @@ __BEGIN_DECLS
/* We need `size_t' for the following definitions. */
#ifndef __size_t
typedef __SIZE_TYPE__ __size_t;
-# if defined __USE_XOPEN || __USE_XOPEN2K8
+# if defined __USE_XOPEN || defined __USE_XOPEN2K8
typedef __SIZE_TYPE__ size_t;
# endif
#else
@@ -273,13 +273,13 @@ struct cmsghdr
with this. */
int cmsg_level; /* Originating protocol. */
int cmsg_type; /* Protocol specific type. */
-#if (!defined __STRICT_ANSI__ && __GNUC__ >= 2) || __STDC_VERSION__ >= 199901L
+#ifndef __flexarr_is_fake
__extension__ unsigned char __cmsg_data __flexarr; /* Ancillary data. */
#endif
};
/* Ancillary data object manipulation macros. */
-#if (!defined __STRICT_ANSI__ && __GNUC__ >= 2) || __STDC_VERSION__ >= 199901L
+#ifndef __flexarr_is_fake
# define CMSG_DATA(cmsg) ((cmsg)->__cmsg_data)
#else
# define CMSG_DATA(cmsg) ((unsigned char *) ((struct cmsghdr *) (cmsg) + 1))