[WIP] Fix HAVE_CONFIG_H -Wundef warnings.

Message ID 5363562D.6030902@redhat.com
State Not applicable
Headers

Commit Message

Carlos O'Donell May 2, 2014, 8:24 a.m. UTC
  Roland,

Is fixing the HAVE_CONFIG_H -Wundef warnings as easy as this?

---

Followed by the mechanical change of all the `#idfef HAVE_CONFIG_H`
to `#if HAVE_CONFIG_H`.

Then verify the built binaries are identical and pass the testsuite?

We always create a config.h for glibc so this looks correct.

I don't want to waste my time fixing all of this if I've got it wrong.

Cheers,
Carlos.
  

Comments

Will Newton May 2, 2014, 8:28 a.m. UTC | #1
On 2 May 2014 09:24, Carlos O'Donell <carlos@redhat.com> wrote:

Hi Carlos,

> Is fixing the HAVE_CONFIG_H -Wundef warnings as easy as this?
>
> diff --git a/Makeconfig b/Makeconfig
> index f965398..64b64fc 100644
> --- a/Makeconfig
> +++ b/Makeconfig
> @@ -1081,6 +1081,9 @@ endif
>  sysd-rules-targets := $(sort $(foreach p,$(sysd-rules-patterns),\
>                                          $(firstword $(subst :, ,$p))))
>
> +# We always configure glibc such that config.h is available.
> +defines += -DHAVE_CONFIG_H=1
> +
>  # A sysdeps Makeconfig fragment may set libc-reentrant to yes.
>  ifeq (yes,$(libc-reentrant))
>  defines += -D_LIBC_REENTRANT
> ---
>
> Followed by the mechanical change of all the `#idfef HAVE_CONFIG_H`
> to `#if HAVE_CONFIG_H`.
>
> Then verify the built binaries are identical and pass the testsuite?
>
> We always create a config.h for glibc so this looks correct.
>
> I don't want to waste my time fixing all of this if I've got it wrong.

A reasonable number of these are from files shared with gnulib. The
gnulib versions of the files have in some cases switched to use
#ifndef _LIBC instead of #if HAVE_CONFIG_H  but I haven't delved into
why that is yet.
  
Carlos O'Donell May 2, 2014, 8:36 a.m. UTC | #2
On 05/02/2014 04:28 AM, Will Newton wrote:
> On 2 May 2014 09:24, Carlos O'Donell <carlos@redhat.com> wrote:
> 
> Hi Carlos,
> 
>> Is fixing the HAVE_CONFIG_H -Wundef warnings as easy as this?
>>
>> diff --git a/Makeconfig b/Makeconfig
>> index f965398..64b64fc 100644
>> --- a/Makeconfig
>> +++ b/Makeconfig
>> @@ -1081,6 +1081,9 @@ endif
>>  sysd-rules-targets := $(sort $(foreach p,$(sysd-rules-patterns),\
>>                                          $(firstword $(subst :, ,$p))))
>>
>> +# We always configure glibc such that config.h is available.
>> +defines += -DHAVE_CONFIG_H=1
>> +
>>  # A sysdeps Makeconfig fragment may set libc-reentrant to yes.
>>  ifeq (yes,$(libc-reentrant))
>>  defines += -D_LIBC_REENTRANT
>> ---
>>
>> Followed by the mechanical change of all the `#idfef HAVE_CONFIG_H`
>> to `#if HAVE_CONFIG_H`.
>>
>> Then verify the built binaries are identical and pass the testsuite?
>>
>> We always create a config.h for glibc so this looks correct.
>>
>> I don't want to waste my time fixing all of this if I've got it wrong.
> 
> A reasonable number of these are from files shared with gnulib. The
> gnulib versions of the files have in some cases switched to use
> #ifndef _LIBC instead of #if HAVE_CONFIG_H  but I haven't delved into
> why that is yet.
 
I can ignore the gnulib files, thanks to your nice list, and I can
change only glibc ones to use `#if HAVE_CONFIG_H`, but still define
`-DHAVE_CONFIG_H=1`, that way we're ready for the gnulib update.

I've been looking at the gettext update today, but it's more work
than I have time for right now (repeating myself today).

Cheers,
Carlos.
  
Paul Eggert May 2, 2014, 7:18 p.m. UTC | #3
On 05/02/2014 01:28 AM, Will Newton wrote:
> The
> gnulib versions of the files have in some cases switched to use
> #ifndef _LIBC instead of #if HAVE_CONFIG_H  but I haven't delved into
> why that is yet.

Some years ago gnulib changed, and it now assumes that <config.h> always 
exists.  We surround the include with "#ifndef _LIBC" only for files 
shared with glibc.  If glibc is also going to assume config.h exists, I 
suggest getting rid of the #if, and using "#include <config.h>" 
unconditionally; that's simplest.
  
Roland McGrath May 2, 2014, 7:39 p.m. UTC | #4
> Roland,
> 
> Is fixing the HAVE_CONFIG_H -Wundef warnings as easy as this?

It's relatively easy, but this is not it.

Firstly, never ever use -D rather than #define unless you have a very
serious justification.  (It's hostile to incremental builds, since GNU
make does not track command line changes.)

We do have a config.h but it's already implicitly included for every
file.  libc-symbols.h does #include <config.h>, and every compile gets
libc-symbols.h via the -include switch.  So !HAVE_CONFIG_H is the
correct state for libc.

Our config.h does not have a multiple-inclusion guard.  OTOH, it so
happens that all the contents of config.h are things that are silently
harmless to repeat (#undef of things already #undef'd, #define of
things to the identical value already #define'd).

That being the case, it's already harmless to have it included more
than once--and it would also be harmless to give it a multiple
inclusion guard, unnecessary though that is.  But what's really right
is to have it included only once, which means just the existing once
via libc-symbols.h and never directly in any source file.

The obvious thing is to put "#define HAVE_CONFIG_H 0" right after
"#include <config.h>" in libc-symbols.h.  That won't do what we want
today, because there are many files that use #ifdef rather than #if
for the test.  Since any mention of <config.h> at all should only
appear in source files shared with other projects for their benefit,
it will require coordination to change them to #if and be sure that is
OK for the other users.  Conversely, if the projects that share these
files and care about config.h all use #ifdef uniformly, then we could
just change the small minority that use #if today to use #ifdef and
never define HAVE_CONFIG_H anywhere in the libc build.


Thanks,
Roland
  

Patch

diff --git a/Makeconfig b/Makeconfig
index f965398..64b64fc 100644
--- a/Makeconfig
+++ b/Makeconfig
@@ -1081,6 +1081,9 @@  endif
 sysd-rules-targets := $(sort $(foreach p,$(sysd-rules-patterns),\
                                         $(firstword $(subst :, ,$p))))
 
+# We always configure glibc such that config.h is available.
+defines += -DHAVE_CONFIG_H=1
+
 # A sysdeps Makeconfig fragment may set libc-reentrant to yes.
 ifeq (yes,$(libc-reentrant))
 defines += -D_LIBC_REENTRANT