[bootstrap] Tentative fix for PR 54281

[bootstrap] Tentative fix for PR 54281

[Diego Novillo]

Richi, this implements your idea for fixing PR 54281.  I don't
have an old enough compiler.  Could you please test it in your
system?

I debated whether to remove the GENERATOR_FILE predicate from the
inclusion (some files include gmp.h unconditionally).  I think it
could be removed, but only a minority of files build with
GENERATOR_FILE set, so it didn't seem harmful.

OK if tests pass?


Diego.


2012-08-16  Diego Novillo  <dnovillo@google.com>
            Richard Guenther  <rguenther@suse.de>

	PR bootstrap/54281
	* double-int.h: Move including of gmp.h ...
	* system.h: ... here.
	* realmpfr.h: Do not include gmp.h.
	* tree-ssa-loop-niter.c: Do not include gmp.h.

fortran/ChangeLog
	* gfortran.h: Do not include gmp.h.

diff --git a/gcc/double-int.h b/gcc/double-int.h
index 3d9aa2c..7ea0528 100644
--- a/gcc/double-int.h
+++ b/gcc/double-int.h
@@ -20,10 +20,6 @@ along with GCC; see the file COPYING3.  If not see
 #ifndef DOUBLE_INT_H
 #define DOUBLE_INT_H
 
-#ifndef GENERATOR_FILE
-#include <gmp.h>
-#endif
-
 /* A large integer is currently represented as a pair of HOST_WIDE_INTs.
    It therefore represents a number with precision of
    2 * HOST_BITS_PER_WIDE_INT bits (it is however possible that the
diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
index 7c4c0a4..611d16d 100644
--- a/gcc/fortran/gfortran.h
+++ b/gcc/fortran/gfortran.h
@@ -1681,7 +1681,6 @@ gfc_intrinsic_sym;
    EXPR_COMPCALL   Function (or subroutine) call of a procedure pointer
 		   component or type-bound procedure.  */
 
-#include <gmp.h>
 #include <mpfr.h>
 #include <mpc.h>
 #define GFC_RND_MODE GMP_RNDN
diff --git a/gcc/realmpfr.h b/gcc/realmpfr.h
index ab234e9..ada876e 100644
--- a/gcc/realmpfr.h
+++ b/gcc/realmpfr.h
@@ -22,7 +22,10 @@
 #ifndef GCC_REALGMP_H
 #define GCC_REALGMP_H
 
-#include <gmp.h>
+/* Note that we do not include gmp.h.  It is included in system.h
+   because it wrecks intl.h when compiling in C++ mode.
+   See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54281 for details.  */
+
 #include <mpfr.h>
 #include <mpc.h>
 #include "real.h"
diff --git a/gcc/system.h b/gcc/system.h
index 9e7d503..0ccd991 100644
--- a/gcc/system.h
+++ b/gcc/system.h
@@ -1037,4 +1037,8 @@ helper_const_non_const_cast (const char *p)
 #define DEBUG_VARIABLE
 #endif
 
+#ifndef GENERATOR_FILE
+#include <gmp.h>
+#endif
+
 #endif /* ! GCC_SYSTEM_H */
diff --git a/gcc/tree-ssa-loop-niter.c b/gcc/tree-ssa-loop-niter.c
index c719a74..4c67c26 100644
--- a/gcc/tree-ssa-loop-niter.c
+++ b/gcc/tree-ssa-loop-niter.c
@@ -38,7 +38,6 @@ along with GCC; see the file COPYING3.  If not see
 #include "flags.h"
 #include "diagnostic-core.h"
 #include "tree-inline.h"
-#include "gmp.h"
 
 #define SWAP(X, Y) do { affine_iv *tmp = (X); (X) = (Y); (Y) = tmp; } while (0)
 

Re: [bootstrap] Tentative fix for PR 54281

[Richard Guenther]

On Thu, 16 Aug 2012, Diego Novillo wrote:

> Richi, this implements your idea for fixing PR 54281.  I don't
> have an old enough compiler.  Could you please test it in your
> system?
> 
> I debated whether to remove the GENERATOR_FILE predicate from the
> inclusion (some files include gmp.h unconditionally).  I think it
> could be removed, but only a minority of files build with
> GENERATOR_FILE set, so it didn't seem harmful.
> 
> OK if tests pass?

It fixes it.

Thus, ok from my side (if your testing succeeds).

Thanks,
Richard.

> 
> Diego.
> 
> 
> 2012-08-16  Diego Novillo  <dnovillo@google.com>
>             Richard Guenther  <rguenther@suse.de>
> 
> 	PR bootstrap/54281
> 	* double-int.h: Move including of gmp.h ...
> 	* system.h: ... here.
> 	* realmpfr.h: Do not include gmp.h.
> 	* tree-ssa-loop-niter.c: Do not include gmp.h.
> 
> fortran/ChangeLog
> 	* gfortran.h: Do not include gmp.h.
> 
> diff --git a/gcc/double-int.h b/gcc/double-int.h
> index 3d9aa2c..7ea0528 100644
> --- a/gcc/double-int.h
> +++ b/gcc/double-int.h
> @@ -20,10 +20,6 @@ along with GCC; see the file COPYING3.  If not see
>  #ifndef DOUBLE_INT_H
>  #define DOUBLE_INT_H
>  
> -#ifndef GENERATOR_FILE
> -#include <gmp.h>
> -#endif
> -
>  /* A large integer is currently represented as a pair of HOST_WIDE_INTs.
>     It therefore represents a number with precision of
>     2 * HOST_BITS_PER_WIDE_INT bits (it is however possible that the
> diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
> index 7c4c0a4..611d16d 100644
> --- a/gcc/fortran/gfortran.h
> +++ b/gcc/fortran/gfortran.h
> @@ -1681,7 +1681,6 @@ gfc_intrinsic_sym;
>     EXPR_COMPCALL   Function (or subroutine) call of a procedure pointer
>  		   component or type-bound procedure.  */
>  
> -#include <gmp.h>
>  #include <mpfr.h>
>  #include <mpc.h>
>  #define GFC_RND_MODE GMP_RNDN
> diff --git a/gcc/realmpfr.h b/gcc/realmpfr.h
> index ab234e9..ada876e 100644
> --- a/gcc/realmpfr.h
> +++ b/gcc/realmpfr.h
> @@ -22,7 +22,10 @@
>  #ifndef GCC_REALGMP_H
>  #define GCC_REALGMP_H
>  
> -#include <gmp.h>
> +/* Note that we do not include gmp.h.  It is included in system.h
> +   because it wrecks intl.h when compiling in C++ mode.
> +   See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54281 for details.  */
> +
>  #include <mpfr.h>
>  #include <mpc.h>
>  #include "real.h"
> diff --git a/gcc/system.h b/gcc/system.h
> index 9e7d503..0ccd991 100644
> --- a/gcc/system.h
> +++ b/gcc/system.h
> @@ -1037,4 +1037,8 @@ helper_const_non_const_cast (const char *p)
>  #define DEBUG_VARIABLE
>  #endif
>  
> +#ifndef GENERATOR_FILE
> +#include <gmp.h>
> +#endif
> +
>  #endif /* ! GCC_SYSTEM_H */
> diff --git a/gcc/tree-ssa-loop-niter.c b/gcc/tree-ssa-loop-niter.c
> index c719a74..4c67c26 100644
> --- a/gcc/tree-ssa-loop-niter.c
> +++ b/gcc/tree-ssa-loop-niter.c
> @@ -38,7 +38,6 @@ along with GCC; see the file COPYING3.  If not see
>  #include "flags.h"
>  #include "diagnostic-core.h"
>  #include "tree-inline.h"
> -#include "gmp.h"
>  
>  #define SWAP(X, Y) do { affine_iv *tmp = (X); (X) = (Y); (Y) = tmp; } while (0)
>  
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend

Re: [bootstrap] Tentative fix for PR 54281

[Diego Novillo]

On 12-08-16 09:08 , Richard Guenther wrote:

> It fixes it.
>
> Thus, ok from my side (if your testing succeeds).

Worked here too.  Committed to trunk.


Diego.

Re: [bootstrap] Tentative fix for PR 54281

[Diego Novillo]

On 12-08-16 09:08 , Richard Guenther wrote:
> On Thu, 16 Aug 2012, Diego Novillo wrote:
>
>> Richi, this implements your idea for fixing PR 54281.  I don't
>> have an old enough compiler.  Could you please test it in your
>> system?
>>
>> I debated whether to remove the GENERATOR_FILE predicate from the
>> inclusion (some files include gmp.h unconditionally).  I think it
>> could be removed, but only a minority of files build with
>> GENERATOR_FILE set, so it didn't seem harmful.
>>
>> OK if tests pass?
>
> It fixes it.
>
> Thus, ok from my side (if your testing succeeds).

So, I had failed to include Ada in my testing and my patch breaks it. 
In several ada/*.c files, we forcefully include system.h as a C header:

#ifdef __cplusplus
extern "C" {
#endif
...
#include "system.h"
...
#ifdef __cplusplus
}
#endif


Eric, is this really needed now?  We are including gmp.h from system.h 
due to PR 54281.  This is causing Ada builds to fail.

Would it be OK to remove all the '#ifdef __cplusplus' conditionals from 
ada/*.c or is there something I'm missing?


Thanks.  Diego.

Re: [bootstrap] Tentative fix for PR 54281

[Magnus Fromreide]

On Thu, Aug 16, 2012 at 07:55:51AM -0400, Diego Novillo wrote:
> Richi, this implements your idea for fixing PR 54281.  I don't
> have an old enough compiler.  Could you please test it in your
> system?
> 
> I debated whether to remove the GENERATOR_FILE predicate from the
> inclusion (some files include gmp.h unconditionally).  I think it
> could be removed, but only a minority of files build with
> GENERATOR_FILE set, so it didn't seem harmful.
> 
> OK if tests pass?

This patch breaks building with gmp, cloog and isl in subdirectories of the
source tree.


echo timestamp > s-options
gawk -f ../../trunk/gcc/opt-functions.awk -f ../../trunk/gcc/opt-read.awk \
       -f ../../trunk/gcc/opth-gen.awk \
       < optionlist > tmp-options.h
/bin/sh ../../trunk/gcc/../move-if-change tmp-options.h options.h
echo timestamp > s-options-h
TARGET_CPU_DEFAULT="" \
HEADERS="auto-host.h ansidecl.h" DEFINES="" \
/bin/sh ../../trunk/gcc/mkconfig.sh bconfig.h
g++ -c   -g -DIN_GCC   -fno-exceptions -fno-rtti -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wmissing-format-attribute -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -fno-common  -DHAVE_CONFIG_H -DGENERATOR_FILE -I. -Ibuild -I../../trunk/gcc -I../../trunk/gcc/build -I../../trunk/gcc/../include -I../../trunk/gcc/../libcpp/include -I/home/magfr/src/gcc/build/./gmp -I/home/magfr/src/gcc/trunk/gmp -I/home/magfr/src/gcc/build/./mpfr -I/home/magfr/src/gcc/trunk/mpfr -I/home/magfr/src/gcc/trunk/mpc/src  -I../../trunk/gcc/../libdecnumber -I../../trunk/gcc/../libdecnumber/bid -I../libdecnumber -DCLOOG_INT_GMP -I/home/magfr/src/gcc/build/./cloog/include -I/home/magfr/src/gcc/trunk/cloog/include -I../trunk/cloog/include  -I/home/magfr/src/gcc/build/./isl/include -I/home/magfr/src/gcc/trunk/isl/include  \
        -o build/genconstants.o ../../trunk/gcc/genconstants.c
In file included from /usr/include/sys/resource.h:25:0,
                 from /usr/include/sys/wait.h:32,
                 from ../../trunk/gcc/system.h:351,
                 from ../../trunk/gcc/genconstants.c:29:
/usr/include/bits/resource.h:133:18: error: declaration does not declare anything [-fpermissive]
In file included from ../../trunk/gcc/genconstants.c:29:0:
../../trunk/gcc/system.h:443:23: error: declaration of C function `void* sbrk(int)' conflicts with
In file included from ../../trunk/gcc/system.h:253:0,
                 from ../../trunk/gcc/genconstants.c:29:
/usr/include/unistd.h:1067:14: error: previous declaration `void* sbrk(intptr_t)' here
In file included from ../../trunk/gcc/genconstants.c:29:0:
../../trunk/gcc/system.h:447:48: error: new declaration `char* strstr(const char*, const char*)'
In file included from /usr/lib/gcc/x86_64-redhat-linux/4.7.0/../../../../include/c++/4.7.0/cstring:44:0,
                 from ../../trunk/gcc/system.h:207,
                 from ../../trunk/gcc/genconstants.c:29:
/usr/include/string.h:323:22: error: ambiguates old declaration `const char* strstr(const char*, const char*)'
In file included from ../../trunk/gcc/genconstants.c:29:0:
../../trunk/gcc/system.h:499:34: error: declaration of C function `const char* strsignal(int)' conflicts with
In file included from /usr/lib/gcc/x86_64-redhat-linux/4.7.0/../../../../include/c++/4.7.0/cstring:44:0,
                 from ../../trunk/gcc/system.h:207,
                 from ../../trunk/gcc/genconstants.c:29:
/usr/include/string.h:566:14: error: previous declaration `char* strsignal(int)' here
In file included from ../../trunk/gcc/system.h:639:0,
                 from ../../trunk/gcc/genconstants.c:29:
../../trunk/gcc/../include/libiberty.h:110:36: error: new declaration `char* basename(const char*)'
In file included from /usr/lib/gcc/x86_64-redhat-linux/4.7.0/../../../../include/c++/4.7.0/cstring:44:0,
                 from ../../trunk/gcc/system.h:207,
                 from ../../trunk/gcc/genconstants.c:29:
/usr/include/string.h:603:28: error: ambiguates old declaration `const char* basename(const char*)'
make[3]: *** [build/genconstants.o] Error 1
make[3]: Leaving directory `/home/magfr/src/gcc/build/gcc'
make[2]: *** [all-stage1-gcc] Error 2
make[2]: Leaving directory `/home/magfr/src/gcc/build'
make[1]: *** [stage1-bubble] Error 2
make[1]: Leaving directory `/home/magfr/src/gcc/build'
make: *** [all] Error 2


The problem seems to be that during configure time the test scripts include
system.h, but it fails to add the gmp include directory to the compiler flags
so the checks for sbrk, strstr and so on fails with


configure:10294: checking whether sbrk is declared
configure:10317: gcc -c -g -I../../trunk/gcc -I../../trunk/gcc/../include  conftest.c >&5
In file included from conftest.c:125:0:
../../trunk/gcc/system.h:1041:17: fatal error: gmp.h: No such file or directory
compilation terminated.
configure:10317: $? = 1


Reverting this patch makes the compiler build.

/MF

Re: [bootstrap] Tentative fix for PR 54281

[Diego Novillo]

On Thu, Aug 16, 2012 at 1:50 PM, Magnus Fromreide <magfr@lysator.liu.se> wrote:
> On Thu, Aug 16, 2012 at 07:55:51AM -0400, Diego Novillo wrote:
>> Richi, this implements your idea for fixing PR 54281.  I don't
>> have an old enough compiler.  Could you please test it in your
>> system?
>>
>> I debated whether to remove the GENERATOR_FILE predicate from the
>> inclusion (some files include gmp.h unconditionally).  I think it
>> could be removed, but only a minority of files build with
>> GENERATOR_FILE set, so it didn't seem harmful.
>>
>> OK if tests pass?
>
> This patch breaks building with gmp, cloog and isl in subdirectories of the
> source tree.

Working on a fix.  It also exposed problems in Ada.


Diego.

Re: [bootstrap] Tentative fix for PR 54281

[Diego Novillo]

I have reverted my original fix and propose this one.  My fix caused 
build failures in Ada (which includes system.h inside 'extern "C"' 
blocks) and it also breaks in-tree isl/cloog.

Richi, I've tried building my own 4.1, but it doesn't build on my 
system.  Could you try this patch?  It includes libintl.h before 
undefining the names, this way the inclusion done from gmp.h turns into 
a nop.


Thanks.  Diego.

commit 96e3d8108901c6f94fa3b0f2de769370688836cb
Author: Diego Novillo <dnovillo@google.com>
Date:   Thu Aug 16 14:27:49 2012 -0400

     2012-08-16   Diego Novillo  <dnovillo@google.com>

         PR bootstrap/54281
         * intl.h: Always include libintl.h.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index a8ff00d..5252122 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,5 +1,10 @@
  2012-08-16   Diego Novillo  <dnovillo@google.com>

+       PR bootstrap/54281
+       * intl.h: Always include libintl.h.
+
+2012-08-16   Diego Novillo  <dnovillo@google.com>
+
         Revert

         PR bootstrap/54281
diff --git a/gcc/intl.h b/gcc/intl.h
index c4db354..745fefd 100644
--- a/gcc/intl.h
+++ b/gcc/intl.h
@@ -27,8 +27,8 @@
  # define setlocale(category, locale) (locale)
  #endif

-#ifdef ENABLE_NLS
  #include <libintl.h>
+#ifdef ENABLE_NLS
  extern void gcc_init_libintl (void);
  extern size_t gcc_gettext_width (const char *);
  #else

Re: [bootstrap] Tentative fix for PR 54281

[Joseph S. Myers]

On Thu, 16 Aug 2012, Diego Novillo wrote:

> diff --git a/gcc/intl.h b/gcc/intl.h
> index c4db354..745fefd 100644
> --- a/gcc/intl.h
> +++ b/gcc/intl.h
> @@ -27,8 +27,8 @@
>  # define setlocale(category, locale) (locale)
>  #endif
> 
> -#ifdef ENABLE_NLS
>  #include <libintl.h>
> +#ifdef ENABLE_NLS

I'm not sure it's safe to assume libintl.h exists on all hosts (e.g. 
MinGW) unless ENABLE_NLS.  (If ENABLE_NLS, the intl/ directory will have 
built that header if the host didn't have it.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [bootstrap] Tentative fix for PR 54281

[Diego Novillo]

On 12-08-16 14:46 , Joseph S. Myers wrote:
> On Thu, 16 Aug 2012, Diego Novillo wrote:
>
>> diff --git a/gcc/intl.h b/gcc/intl.h
>> index c4db354..745fefd 100644
>> --- a/gcc/intl.h
>> +++ b/gcc/intl.h
>> @@ -27,8 +27,8 @@
>>   # define setlocale(category, locale) (locale)
>>   #endif
>>
>> -#ifdef ENABLE_NLS
>>   #include <libintl.h>
>> +#ifdef ENABLE_NLS
>
> I'm not sure it's safe to assume libintl.h exists on all hosts (e.g.
> MinGW) unless ENABLE_NLS.  (If ENABLE_NLS, the intl/ directory will have
> built that header if the host didn't have it.)

I wonder if we couldn't simply '#define _LIBINTL_H 1' in the #else 
branch then.  Something like this (though it seems a bit hacky to me):

diff --git a/gcc/intl.h b/gcc/intl.h
index c4db354..3da4738 100644
--- a/gcc/intl.h
+++ b/gcc/intl.h
@@ -32,6 +32,9 @@
  extern void gcc_init_libintl (void);
  extern size_t gcc_gettext_width (const char *);
  #else
+/* Prevent libintl.h from being included, since we are truncating
+   some functions (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54281).  */
+# define _LIBINTL_H 1
  /* Stubs.  */
  # undef textdomain
  # define textdomain(domain) (domain)

Re: [bootstrap] Tentative fix for PR 54281

[Diego Novillo]

On 12-08-16 15:00 , Diego Novillo wrote:
> On 12-08-16 14:46 , Joseph S. Myers wrote:
>> On Thu, 16 Aug 2012, Diego Novillo wrote:
>>
>>> diff --git a/gcc/intl.h b/gcc/intl.h
>>> index c4db354..745fefd 100644
>>> --- a/gcc/intl.h
>>> +++ b/gcc/intl.h
>>> @@ -27,8 +27,8 @@
>>>   # define setlocale(category, locale) (locale)
>>>   #endif
>>>
>>> -#ifdef ENABLE_NLS
>>>   #include <libintl.h>
>>> +#ifdef ENABLE_NLS
>>
>> I'm not sure it's safe to assume libintl.h exists on all hosts (e.g.
>> MinGW) unless ENABLE_NLS.  (If ENABLE_NLS, the intl/ directory will have
>> built that header if the host didn't have it.)
>
> I wonder if we couldn't simply '#define _LIBINTL_H 1' in the #else
> branch then.  Something like this (though it seems a bit hacky to me):

This is the patch I'm currently testing.  I need someone with a very old 
toolchain (4.1 or earlier) to also give this a try (the original problem 
does not occur in g++ more recent than 4.1).


Thanks.  Diego.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index a8ff00d..8798b8f 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,5 +1,11 @@
  2012-08-16   Diego Novillo  <dnovillo@google.com>

+       PR bootstrap/54281
+       * intl.h: Prevent libintl.h from being included when
+       ENABLE_NLS is not set.
+
+2012-08-16   Diego Novillo  <dnovillo@google.com>
+
         Revert

         PR bootstrap/54281
diff --git a/gcc/intl.h b/gcc/intl.h
index c4db354..e10e357 100644
--- a/gcc/intl.h
+++ b/gcc/intl.h
@@ -32,6 +32,11 @@
  extern void gcc_init_libintl (void);
  extern size_t gcc_gettext_width (const char *);
  #else
+/* Prevent libintl.h from being included, since we are truncating
+   some functions (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54281).  */
+# ifndef _LIBINTL_H
+# define _LIBINTL_H 1
+# endif
  /* Stubs.  */
  # undef textdomain
  # define textdomain(domain) (domain)

Re: [bootstrap] Tentative fix for PR 54281

[Ian Lance Taylor]

On Thu, Aug 16, 2012 at 12:12 PM, Diego Novillo <dnovillo@google.com> wrote:
>
> This is the patch I'm currently testing.  I need someone with a very old
> toolchain (4.1 or earlier) to also give this a try (the original problem
> does not occur in g++ more recent than 4.1).
>
>
> Thanks.  Diego.
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index a8ff00d..8798b8f 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,5 +1,11 @@
>
>  2012-08-16   Diego Novillo  <dnovillo@google.com>
>
> +       PR bootstrap/54281
> +       * intl.h: Prevent libintl.h from being included when
> +       ENABLE_NLS is not set.

It's hard to convince myself that this patch is portable.  I don't
think we can assume that every system that provides <libintl.h> uses
_LIBINTL_H as the preprocessor guard.

The issue is that we must not #include <libintl.h> after #defining
those macros.  So the fix is to #include <libintl.h> in all cases
before #defining those macros.  Your proposal of unconditionally doing
#include <libintl.h> is not a good idea, because <libintl.h> is not
available on every system.  But we are compiling host files here, so
we can just use autoconf.  I recommend that you add libintl.h to the
AC_CHECK_HEADERS list in gcc/configure.ac, and then change intl.h to
do

#ifdef HAVE_LIBINTL_H
#include <libintl.h>
#endif

before the #ifdef ENABLE_NLS test.

Ian

Re: [bootstrap] Tentative fix for PR 54281

[Hans-Peter Nilsson]

> From: Diego Novillo <dnovillo@google.com>
> Date: Thu, 16 Aug 2012 21:12:56 +0200

> On 12-08-16 15:00 , Diego Novillo wrote:

> This is the patch I'm currently testing.  I need someone with a very old
> toolchain (4.1 or earlier) to also give this a try (the original problem
> does not occur in g++ more recent than 4.1).

> +       PR bootstrap/54281
> +       * intl.h: Prevent libintl.h from being included when
> +       ENABLE_NLS is not set.

Regtested on a x86_64 F 8 system with "gcc-4.1.2-33", no
regressions at r190450 (with --disable-nls) compared to results
at r190381 (without).

brgds, H-P

Re: [bootstrap] Tentative fix for PR 54281

[Richard Guenther]

On Thu, 16 Aug 2012, Ian Lance Taylor wrote:

> On Thu, Aug 16, 2012 at 12:12 PM, Diego Novillo <dnovillo@google.com> wrote:
> >
> > This is the patch I'm currently testing.  I need someone with a very old
> > toolchain (4.1 or earlier) to also give this a try (the original problem
> > does not occur in g++ more recent than 4.1).
> >
> >
> > Thanks.  Diego.
> >
> > diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> > index a8ff00d..8798b8f 100644
> > --- a/gcc/ChangeLog
> > +++ b/gcc/ChangeLog
> > @@ -1,5 +1,11 @@
> >
> >  2012-08-16   Diego Novillo  <dnovillo@google.com>
> >
> > +       PR bootstrap/54281
> > +       * intl.h: Prevent libintl.h from being included when
> > +       ENABLE_NLS is not set.
> 
> It's hard to convince myself that this patch is portable.  I don't
> think we can assume that every system that provides <libintl.h> uses
> _LIBINTL_H as the preprocessor guard.
> 
> The issue is that we must not #include <libintl.h> after #defining
> those macros.  So the fix is to #include <libintl.h> in all cases
> before #defining those macros.  Your proposal of unconditionally doing
> #include <libintl.h> is not a good idea, because <libintl.h> is not
> available on every system.  But we are compiling host files here, so
> we can just use autoconf.  I recommend that you add libintl.h to the
> AC_CHECK_HEADERS list in gcc/configure.ac, and then change intl.h to
> do
> 
> #ifdef HAVE_LIBINTL_H
> #include <libintl.h>
> #endif
> 
> before the #ifdef ENABLE_NLS test.

Yes.  Still I think including system headers from random gcc headers
is bogus (the gmp.h include from double-int.h), we have system.h
exactly to be able to setup things that the includes will work
before poisoning anything or for systems that require special care.

The configure check including system.h should define GENERATOR_FILE
eventually.  Or we need to split system.h.

Richard.

Re: [bootstrap] Tentative fix for PR 54281

[Diego Novillo]

[-- Attachment #1: Type: text/plain, Size: 338 bytes --]

On 12-08-16 20:29 , Ian Lance Taylor wrote:

> I recommend that you add libintl.h to the AC_CHECK_HEADERS list in
> gcc/configure.ac

Thanks.  The attached patch implements the approach.

Tested with --disable-nls and --enable-nls.  Folks with 4.1 compilers, 
could you test it there?

OK for trunk if testing succeeds?


Thanks.  Diego.

[-- Attachment #2: 54281.diff --]
[-- Type: text/plain, Size: 3841 bytes --]

commit 8583ba1f22ba310114bf64960f61f6bcc805e9c2
Author: Diego Novillo <dnovillo@google.com>
Date:   Thu Aug 16 14:27:49 2012 -0400

    2012-08-17  Diego Novillo  <dnovillo@google.com>
    
    	PR bootstrap/54281
    	* configure.ac: Add libintl.h to AC_CHECK_HEADERS list.
    	* config.in: Regenerate.
    	* configure: Regenerate.
    	* intl.h: Always include libintl.h if HAVE_LIBINTL_H is
    	set.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index c9a81d1..43b0af7 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,12 @@
+2012-08-17  Diego Novillo  <dnovillo@google.com>
+
+	PR bootstrap/54281
+	* configure.ac: Add libintl.h to AC_CHECK_HEADERS list.
+	* config.in: Regenerate.
+	* configure: Regenerate.
+	* intl.h: Always include libintl.h if HAVE_LIBINTL_H is
+	set.
+
 2012-08-17  Richard Guenther  <rguenther@suse.de>
 
 	* bitmap.h (struct bitmap_element_def): GTY annotate next/prev.
@@ -213,7 +222,7 @@
 	* config/tilegx/feedback.h: New file.
 	* config/tilepro/feedback.h: New file.
 
-2012-08-16  Diego Novillo  <dnovillo@google.com>
+2012-08-16   Diego Novillo  <dnovillo@google.com>
 
 	Revert
 
diff --git a/gcc/config.in b/gcc/config.in
index 6d986be..a9417df 100644
--- a/gcc/config.in
+++ b/gcc/config.in
@@ -1260,6 +1260,12 @@
 #endif
 
 
+/* Define to 1 if you have the <libintl.h> header file. */
+#ifndef USED_FOR_TARGET
+#undef HAVE_LIBINTL_H
+#endif
+
+
 /* Define to 1 if you have the <limits.h> header file. */
 #ifndef USED_FOR_TARGET
 #undef HAVE_LIMITS_H
diff --git a/gcc/configure b/gcc/configure
index 1585bae..7f3489d 100755
--- a/gcc/configure
+++ b/gcc/configure
@@ -8248,7 +8248,7 @@ fi
 for ac_header in limits.h stddef.h string.h strings.h stdlib.h time.h iconv.h \
 		 fcntl.h unistd.h sys/file.h sys/time.h sys/mman.h \
 		 sys/resource.h sys/param.h sys/times.h sys/stat.h \
-		 direct.h malloc.h langinfo.h ldfcn.h locale.h wchar.h
+		 direct.h malloc.h langinfo.h ldfcn.h locale.h wchar.h libintl.h
 do :
   as_ac_Header=`$as_echo "ac_cv_header_$ac_header" | $as_tr_sh`
 ac_fn_c_check_header_preproc "$LINENO" "$ac_header" "$as_ac_Header"
@@ -17742,7 +17742,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 17744 "configure"
+#line 17745 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
@@ -17848,7 +17848,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 17850 "configure"
+#line 17851 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
diff --git a/gcc/configure.ac b/gcc/configure.ac
index 579d9a8..6bfbf35 100644
--- a/gcc/configure.ac
+++ b/gcc/configure.ac
@@ -912,7 +912,7 @@ AC_HEADER_SYS_WAIT
 AC_CHECK_HEADERS(limits.h stddef.h string.h strings.h stdlib.h time.h iconv.h \
 		 fcntl.h unistd.h sys/file.h sys/time.h sys/mman.h \
 		 sys/resource.h sys/param.h sys/times.h sys/stat.h \
-		 direct.h malloc.h langinfo.h ldfcn.h locale.h wchar.h)
+		 direct.h malloc.h langinfo.h ldfcn.h locale.h wchar.h libintl.h)
 
 # Check for thread headers.
 AC_CHECK_HEADER(thread.h, [have_thread_h=yes], [have_thread_h=])
diff --git a/gcc/intl.h b/gcc/intl.h
index c4db354..42ca873 100644
--- a/gcc/intl.h
+++ b/gcc/intl.h
@@ -27,8 +27,16 @@
 # define setlocale(category, locale) (locale)
 #endif
 
+/* If libintl.h is available, include it before testing for NLS. If we
+   are building with --disable-nls and another header file includes
+   libintl.h, the stubs defined down below will cause syntax errors
+   when parsing libintl.h. See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54281
+   for details.  */
+#ifdef HAVE_LIBINTL_H
+# include <libintl.h>
+#endif
+
 #ifdef ENABLE_NLS
-#include <libintl.h>
 extern void gcc_init_libintl (void);
 extern size_t gcc_gettext_width (const char *);
 #else

Re: [bootstrap] Tentative fix for PR 54281

[Jakub Jelinek]

Hi!

On Fri, Aug 17, 2012 at 08:23:02AM -0400, Diego Novillo wrote:

> --- a/gcc/intl.h
> +++ b/gcc/intl.h
> @@ -27,8 +27,16 @@
>  # define setlocale(category, locale) (locale)
>  #endif
>  
> +/* If libintl.h is available, include it before testing for NLS. If we
> +   are building with --disable-nls and another header file includes
> +   libintl.h, the stubs defined down below will cause syntax errors
> +   when parsing libintl.h. See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54281
> +   for details.  */
> +#ifdef HAVE_LIBINTL_H
> +# include <libintl.h>
> +#endif
> +
>  #ifdef ENABLE_NLS
> -#include <libintl.h>
>  extern void gcc_init_libintl (void);
>  extern size_t gcc_gettext_width (const char *);
>  #else

Will that handle even the case where without --disable-nls intl/
creates its own libintl.h?  Dunno which targets need that, but
I'd guess configury wouldn't find it in that case.  So perhaps
it should be #if defined(HAVE_LIBINTL_H) || defined(ENABLE_NLS) ?

	Jakub

Re: [bootstrap] Tentative fix for PR 54281

[Ian Lance Taylor]

On Fri, Aug 17, 2012 at 5:32 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>
> Will that handle even the case where without --disable-nls intl/
> creates its own libintl.h?  Dunno which targets need that, but
> I'd guess configury wouldn't find it in that case.  So perhaps
> it should be #if defined(HAVE_LIBINTL_H) || defined(ENABLE_NLS) ?

Makes sense to me.

Ian

Re: [bootstrap] Tentative fix for PR 54281

[Diego Novillo]

[-- Attachment #1: Type: text/plain, Size: 1153 bytes --]

On 12-08-17 08:32 , Jakub Jelinek wrote:
> Hi!
>
> On Fri, Aug 17, 2012 at 08:23:02AM -0400, Diego Novillo wrote:
>
>> --- a/gcc/intl.h
>> +++ b/gcc/intl.h
>> @@ -27,8 +27,16 @@
>>   # define setlocale(category, locale) (locale)
>>   #endif
>>
>> +/* If libintl.h is available, include it before testing for NLS. If we
>> +   are building with --disable-nls and another header file includes
>> +   libintl.h, the stubs defined down below will cause syntax errors
>> +   when parsing libintl.h. See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54281
>> +   for details.  */
>> +#ifdef HAVE_LIBINTL_H
>> +# include <libintl.h>
>> +#endif
>> +
>>   #ifdef ENABLE_NLS
>> -#include <libintl.h>
>>   extern void gcc_init_libintl (void);
>>   extern size_t gcc_gettext_width (const char *);
>>   #else
>
> Will that handle even the case where without --disable-nls intl/
> creates its own libintl.h?  Dunno which targets need that, but
> I'd guess configury wouldn't find it in that case.  So perhaps
> it should be #if defined(HAVE_LIBINTL_H) || defined(ENABLE_NLS) ?

Sounds reasonable.  Amended patch attached.

Re-tested with --disable-nls.


OK for trunk?

[-- Attachment #2: 54281.diff --]
[-- Type: text/plain, Size: 3463 bytes --]

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index c9a81d1..43b0af7 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,12 @@
+2012-08-17  Diego Novillo  <dnovillo@google.com>
+
+	PR bootstrap/54281
+	* configure.ac: Add libintl.h to AC_CHECK_HEADERS list.
+	* config.in: Regenerate.
+	* configure: Regenerate.
+	* intl.h: Always include libintl.h if HAVE_LIBINTL_H is
+	set.
+
 2012-08-17  Richard Guenther  <rguenther@suse.de>
 
 	* bitmap.h (struct bitmap_element_def): GTY annotate next/prev.
@@ -213,7 +222,7 @@
 	* config/tilegx/feedback.h: New file.
 	* config/tilepro/feedback.h: New file.
 
-2012-08-16  Diego Novillo  <dnovillo@google.com>
+2012-08-16   Diego Novillo  <dnovillo@google.com>
 
 	Revert
 
diff --git a/gcc/config.in b/gcc/config.in
index 6d986be..a9417df 100644
--- a/gcc/config.in
+++ b/gcc/config.in
@@ -1260,6 +1260,12 @@
 #endif
 
 
+/* Define to 1 if you have the <libintl.h> header file. */
+#ifndef USED_FOR_TARGET
+#undef HAVE_LIBINTL_H
+#endif
+
+
 /* Define to 1 if you have the <limits.h> header file. */
 #ifndef USED_FOR_TARGET
 #undef HAVE_LIMITS_H
diff --git a/gcc/configure b/gcc/configure
index 1585bae..7f3489d 100755
--- a/gcc/configure
+++ b/gcc/configure
@@ -8248,7 +8248,7 @@ fi
 for ac_header in limits.h stddef.h string.h strings.h stdlib.h time.h iconv.h \
 		 fcntl.h unistd.h sys/file.h sys/time.h sys/mman.h \
 		 sys/resource.h sys/param.h sys/times.h sys/stat.h \
-		 direct.h malloc.h langinfo.h ldfcn.h locale.h wchar.h
+		 direct.h malloc.h langinfo.h ldfcn.h locale.h wchar.h libintl.h
 do :
   as_ac_Header=`$as_echo "ac_cv_header_$ac_header" | $as_tr_sh`
 ac_fn_c_check_header_preproc "$LINENO" "$ac_header" "$as_ac_Header"
@@ -17742,7 +17742,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 17744 "configure"
+#line 17745 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
@@ -17848,7 +17848,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 17850 "configure"
+#line 17851 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
diff --git a/gcc/configure.ac b/gcc/configure.ac
index 579d9a8..6bfbf35 100644
--- a/gcc/configure.ac
+++ b/gcc/configure.ac
@@ -912,7 +912,7 @@ AC_HEADER_SYS_WAIT
 AC_CHECK_HEADERS(limits.h stddef.h string.h strings.h stdlib.h time.h iconv.h \
 		 fcntl.h unistd.h sys/file.h sys/time.h sys/mman.h \
 		 sys/resource.h sys/param.h sys/times.h sys/stat.h \
-		 direct.h malloc.h langinfo.h ldfcn.h locale.h wchar.h)
+		 direct.h malloc.h langinfo.h ldfcn.h locale.h wchar.h libintl.h)
 
 # Check for thread headers.
 AC_CHECK_HEADER(thread.h, [have_thread_h=yes], [have_thread_h=])
diff --git a/gcc/intl.h b/gcc/intl.h
index c4db354..03be420 100644
--- a/gcc/intl.h
+++ b/gcc/intl.h
@@ -27,8 +27,16 @@
 # define setlocale(category, locale) (locale)
 #endif
 
+/* If libintl.h is available, include it before testing for NLS. If we
+   are building with --disable-nls and another header file includes
+   libintl.h, the stubs defined down below will cause syntax errors
+   when parsing libintl.h. See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54281
+   for details.  */
+#if defined(HAVE_LIBINTL_H) || defined(ENABLE_NLS)
+# include <libintl.h>
+#endif
+
 #ifdef ENABLE_NLS
-#include <libintl.h>
 extern void gcc_init_libintl (void);
 extern size_t gcc_gettext_width (const char *);
 #else

Re: [bootstrap] Tentative fix for PR 54281

[Jakub Jelinek]

On Fri, Aug 17, 2012 at 10:58:25AM -0400, Diego Novillo wrote:
> Sounds reasonable.  Amended patch attached.

> OK for trunk?

Yes, except for:

> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index c9a81d1..43b0af7 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -213,7 +222,7 @@
>  	* config/tilegx/feedback.h: New file.
>  	* config/tilepro/feedback.h: New file.
>  
> -2012-08-16  Diego Novillo  <dnovillo@google.com>
> +2012-08-16   Diego Novillo  <dnovillo@google.com>
>  
>  	Revert
>  

the above hunk.

	Jakub

Re: [bootstrap] Tentative fix for PR 54281

[Diego Novillo]

On Fri, Aug 17, 2012 at 11:05 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Aug 17, 2012 at 10:58:25AM -0400, Diego Novillo wrote:
>> Sounds reasonable.  Amended patch attached.
>
>> OK for trunk?
>
> Yes, except for:
>
>> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
>> index c9a81d1..43b0af7 100644
>> --- a/gcc/ChangeLog
>> +++ b/gcc/ChangeLog
>> @@ -213,7 +222,7 @@
>>       * config/tilegx/feedback.h: New file.
>>       * config/tilepro/feedback.h: New file.
>>
>> -2012-08-16  Diego Novillo  <dnovillo@google.com>
>> +2012-08-16   Diego Novillo  <dnovillo@google.com>
>>
>>       Revert
>>
>
> the above hunk.

Done.  Committed.


Thanks.  Diego.

Re: [bootstrap] Tentative fix for PR 54281

[Eric Botcazou]

[Sorry for the delay]

> So, I had failed to include Ada in my testing and my patch breaks it.
> In several ada/*.c files, we forcefully include system.h as a C header:
> 
> #ifdef __cplusplus
> extern "C" {
> #endif
> ...
> #include "system.h"
> ...
> #ifdef __cplusplus
> }
> #endif
> 
> 
> Eric, is this really needed now?  We are including gmp.h from system.h
> due to PR 54281.  This is causing Ada builds to fail.
> 
> Would it be OK to remove all the '#ifdef __cplusplus' conditionals from
> ada/*.c or is there something I'm missing?

The conditionals cannot be removed for the time being because the foreign 
language interface of the Ada part of the compiler is hardcoded for C.

Barring a massive switch to the Ada language for the GNU project, we would need 
to switch the foreign language interface to C++, which might introduce various 
maintenance issues in the short term (Arno CCed).

-- 
Eric Botcazou

Re: [bootstrap] Tentative fix for PR 54281

[Arnaud Charlet]

> The conditionals cannot be removed for the time being because the foreign 
> language interface of the Ada part of the compiler is hardcoded for C.
> 
> Barring a massive switch to the Ada language for the GNU project, we would need 
> to switch the foreign language interface to C++, which might introduce various 
> maintenance issues in the short term (Arno CCed).

Yes, that would be a large hearthquake for both the compiler and the run-time, which is unwanted at this stage. I guess the solution for now is to more selectively export the various symbols as C symbols and include header files as is.

Arno

Re: [bootstrap] Tentative fix for PR 54281

[Diego Novillo]

On 2012-08-19 07:18 , Arnaud Charlet wrote:
>
>> The conditionals cannot be removed for the time being because the
>> foreign language interface of the Ada part of the compiler is
>> hardcoded for C.
>>
>> Barring a massive switch to the Ada language for the GNU project,
>> we would need to switch the foreign language interface to C++,
>> which might introduce various maintenance issues in the short term
>> (Arno CCed).
>
> Yes, that would be a large hearthquake for both the compiler and the
> run-time, which is unwanted at this stage. I guess the solution for
> now is to more selectively export the various symbols as C symbols
> and include header files as is.

Thanks.

One potential issue I see here is that as we start using more C++ 
headers (and more C++ in existing headers), we will reach a point in 
which including something like system.h inside an extern "C" {} block 
will fail.

None of the GCC headers should be included as C code anymore.


Diego.