Patchwork [bootstrap] Tentative fix for PR 54281

login
register
mail settings
Submitter Diego Novillo
Date Aug. 16, 2012, 7:12 p.m.
Message ID <502D4638.40009@google.com>
Download mbox | patch
Permalink /patch/178078/
State New
Headers show

Comments

Diego Novillo - Aug. 16, 2012, 7:12 p.m.
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.
Ian Taylor - Aug. 17, 2012, 12:29 a.m.
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
Richard Guenther - Aug. 17, 2012, 7:44 a.m.
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.

Patch

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)