| Message ID | yddbowonc44.fsf@manam.CeBiTec.Uni-Bielefeld.DE |
|---|---|
| State | New |
| Headers | show |
Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes: > diff --git a/gcc/config/sol2-c.c b/gcc/config/sol2-c.c > --- a/gcc/config/sol2-c.c > +++ b/gcc/config/sol2-c.c > @@ -68,7 +68,7 @@ static const format_char_info cmn_err_ch > { NULL, 0, STD_C89, NOLENGTHS, NULL, NULL, NULL } > }; > > -const format_kind_info solaris_format_types[] = { > +extern const format_kind_info solaris_format_types[] = { > { "cmn_err", cmn_err_length_specs, cmn_err_char_table, "", NULL, > cmn_err_flag_specs, cmn_err_flag_pairs, > FMT_FLAG_ARG_CONVERT|FMT_FLAG_EMPTY_PREC_OK, s/extern const/EXPORTED_CONST/ (see various other uses of EXPORTED_CONST). This patch is OK with that change. Ian
Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes: > diff --git a/gcc/configure.ac b/gcc/configure.ac > --- a/gcc/configure.ac > +++ b/gcc/configure.ac > @@ -1041,7 +1041,16 @@ case "${host}" in > esac > AC_FUNC_FORK > > +# FIXME: g++ on Solaris 10+ defines _XOPEN_SOURCE=600, which exposes a > +# different iconv() prototype. > +if test "$ENABLE_BUILD_WITH_CXX" = "yes"; then > +AC_LANG_PUSH([C++]) > +fi > AM_ICONV > +if test "$ENABLE_BUILD_WITH_CXX" = "yes"; then > +AC_LANG_POP([C++]) > +fi I believe that this kind of conditional use of AC_LANG_PUSH/AC_LANG_POP does not work correctly. The autoconf macros don't play well with the shell conditionals. We're going to need a different approach. (That problem is why I didn't do this long ago--I had a similar patch for a while, but I had to take it out.) It seems to me that the version of iconv when _XPG6 is defined is correct, and it seems to me that that is the one we get if _XOPEN_SOURCE is defined to be 600 when #including iconv.h. Does that sound right? If so, perhaps we should change the toplevel configure.ac to use mh-sol2 for host *-*-solaris2* (or whatever is appropriate), and create the file config/mh-sol2 with BOOT_CFLAGS += -D_XOPEN_SOURCE=600. Ian
Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes: > diff --git a/libcpp/system.h b/libcpp/system.h > --- a/libcpp/system.h > +++ b/libcpp/system.h > @@ -1,6 +1,6 @@ > /* Get common system includes and various definitions and declarations based > on autoconf macros. > - Copyright (C) 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2009, 2010 > + Copyright (C) 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2009, 2010, 2011 > Free Software Foundation, Inc. > > This file is part of GCC. > @@ -353,8 +353,8 @@ extern void abort (void); > compilers, including G++. -- gdr, 2005-05-18 */ > #if !defined(HAVE_DESIGNATED_INITIALIZERS) > #define HAVE_DESIGNATED_INITIALIZERS \ > - ((!defined(__cplusplus) && (GCC_VERSION >= 2007)) \ > - || (__STDC_VERSION__ >= 199901L)) > + (!defined(__cplusplus) \ > + && ((GCC_VERSION >= 2007) || (__STDC_VERSION__ >= 199901L))) > #endif This is OK. Thanks. Ian
Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes: > diff --git a/gcc/ada/init.c b/gcc/ada/init.c > --- a/gcc/ada/init.c > +++ b/gcc/ada/init.c > @@ -1031,7 +1031,7 @@ __gnat_install_handler (void) > exceptions. Make sure that the handler isn't interrupted by another > signal that might cause a scheduling event! */ > > - act.sa_handler = __gnat_error_handler; > + act.sa_sigaction = __gnat_error_handler; > act.sa_flags = SA_NODEFER | SA_RESTART | SA_SIGINFO; > sigemptyset (&act.sa_mask); > > diff --git a/gcc/ada/tracebak.c b/gcc/ada/tracebak.c > --- a/gcc/ada/tracebak.c > +++ b/gcc/ada/tracebak.c > @@ -482,12 +482,12 @@ __gnat_backtrace (void **array, > while (cnt < size) > { > if (STOP_FRAME (current, top_stack) || > - !VALID_STACK_FRAME((char *)(current->return_address + PC_ADJUST))) > + !VALID_STACK_FRAME(((char *) current->return_address) + PC_ADJUST)) > break; > > if (current->return_address < exclude_min > || current->return_address > exclude_max) > - array[cnt++] = current->return_address + PC_ADJUST; > + array[cnt++] = ((char *) current->return_address) + PC_ADJUST; > > current = (struct layout *) ((size_t) current->next + FRAME_OFFSET (1)); > } These patches are OK unless some Ada maintainer objects in the next 24 hours. Thanks. Ian
Hello, * Ian Lance Taylor wrote on Wed, Jul 20, 2011 at 08:06:51PM CEST: > Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes: > > > diff --git a/gcc/configure.ac b/gcc/configure.ac > > --- a/gcc/configure.ac > > +++ b/gcc/configure.ac > > @@ -1041,7 +1041,16 @@ case "${host}" in > > esac > > AC_FUNC_FORK > > > > +# FIXME: g++ on Solaris 10+ defines _XOPEN_SOURCE=600, which exposes a > > +# different iconv() prototype. > > +if test "$ENABLE_BUILD_WITH_CXX" = "yes"; then > > +AC_LANG_PUSH([C++]) > > +fi > > AM_ICONV > > +if test "$ENABLE_BUILD_WITH_CXX" = "yes"; then > > +AC_LANG_POP([C++]) > > +fi > > I believe that this kind of conditional use of AC_LANG_PUSH/AC_LANG_POP > does not work correctly. The autoconf macros don't play well with the > shell conditionals. We're going to need a different approach. (That > problem is why I didn't do this long ago--I had a similar patch for a > while, but I had to take it out.) I think something like this should work: AS_IF([test "$ENABLE_BUILD_WITH_CXX" = "yes"], [AC_LANG_PUSH([C++]) AM_ICONV AC_LANG_POP([C++])], [AM_ICONV]) Rationale for using AS_IF rather than plain 'if' is that any macros required by AM_ICONV will get pulled out to before the AS_IF. If they also need to be run with C++ enabled, then we need to wrap the second argument of AS_IF into a separate macro. Untested, but please complain if it doesn't work. Thanks, Ralf
"Ralf Wildenhues" <Ralf.Wildenhues@gmx.de> writes: > Hello, > > * Ian Lance Taylor wrote on Wed, Jul 20, 2011 at 08:06:51PM CEST: >> Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes: >> >> > diff --git a/gcc/configure.ac b/gcc/configure.ac >> > --- a/gcc/configure.ac >> > +++ b/gcc/configure.ac >> > @@ -1041,7 +1041,16 @@ case "${host}" in >> > esac >> > AC_FUNC_FORK >> > >> > +# FIXME: g++ on Solaris 10+ defines _XOPEN_SOURCE=600, which exposes a >> > +# different iconv() prototype. >> > +if test "$ENABLE_BUILD_WITH_CXX" = "yes"; then >> > +AC_LANG_PUSH([C++]) >> > +fi >> > AM_ICONV >> > +if test "$ENABLE_BUILD_WITH_CXX" = "yes"; then >> > +AC_LANG_POP([C++]) >> > +fi >> >> I believe that this kind of conditional use of AC_LANG_PUSH/AC_LANG_POP >> does not work correctly. The autoconf macros don't play well with the >> shell conditionals. We're going to need a different approach. (That >> problem is why I didn't do this long ago--I had a similar patch for a >> while, but I had to take it out.) > > I think something like this should work: > > AS_IF([test "$ENABLE_BUILD_WITH_CXX" = "yes"], > [AC_LANG_PUSH([C++]) > AM_ICONV > AC_LANG_POP([C++])], > [AM_ICONV]) > > Rationale for using AS_IF rather than plain 'if' is that any macros > required by AM_ICONV will get pulled out to before the AS_IF. If they > also need to be run with C++ enabled, then we need to wrap the second > argument of AS_IF into a separate macro. Untested, but please complain > if it doesn't work. Cool. In general, though, we should ideally be using C++ when appropriate for most or all of the compilation tests, not just for AM_ICONV. Ian
> * Similarly to IRIX 6, there was a mismatch in ada/init.c for the type > of the signal handler installed with sigaction. g++ also doesn't like > arithmetic on void * ;-) I'm a bit puzzled as to why the C compiler isn't complaining, and what error message the C++ compiler is generating, can you clarify? Why is the code accepted by the C compiler in the first place? > gcc/ada: > PR bootstrap/49794 > * init.c [sun && __SVR4 && !__vxworks] (__gnat_install_handler): > Assign to act.sa_sigaction. > * tracebak.c [USE_GENERIC_UNWINDER] (__gnat_backtrace): Cast > current->return_address to char * before arithmetic. The tracebak.c part is OK. > diff --git a/gcc/ada/init.c b/gcc/ada/init.c > --- a/gcc/ada/init.c > +++ b/gcc/ada/init.c > @@ -1031,7 +1031,7 @@ __gnat_install_handler (void) > exceptions. Make sure that the handler isn't interrupted by another > signal that might cause a scheduling event! */ > > - act.sa_handler = __gnat_error_handler; > + act.sa_sigaction = __gnat_error_handler; > act.sa_flags = SA_NODEFER | SA_RESTART | SA_SIGINFO; > sigemptyset (&act.sa_mask); >
Arnaud, >> * Similarly to IRIX 6, there was a mismatch in ada/init.c for the type >> of the signal handler installed with sigaction. g++ also doesn't like >> arithmetic on void * ;-) > > I'm a bit puzzled as to why the C compiler isn't complaining, and what > error message the C++ compiler is generating, can you clarify? I'd removed the previous build before copying the exact error message. Here's what I get without the init.c change: /vol/gcc/src/hg/trunk/local/gcc/ada/init.c: In function 'void __gnat_install_handler()': /vol/gcc/src/hg/trunk/local/gcc/ada/init.c:1049:20: error: invalid conversion from 'void (*)(int, siginfo_t*, void*) {aka void (*)(int, siginfo*, void*)}' to 'void (*)(int)' [-fpermissive] which is correct. <sys/signal.h> has /* * The signal handler routine can have either one or three arguments. * Existing C code has used either form so not specifing the arguments * neatly finesses the problem. C++ doesn't accept this. To C++ * "(*sa_handler)()" indicates a routine with no arguments (ANSI C would * specify this as "(*sa_handler)(void)"). One or the other form must be * used for C++ and the only logical choice is "(*sa_handler)(int)" to allow * the SIG_* defines to work. "(*sa_sigaction)(int, siginfo_t *, void *)" * can be used for the three argument form. */ /* * Note: storage overlap by sa_handler and sa_sigaction */ struct sigaction { int sa_flags; union { #ifdef __cplusplus void (*_handler)(int); #else void (*_handler)(); #endif #if defined(__EXTENSIONS__) || defined(_KERNEL) || \ (!defined(_STRICT_STDC) && !defined(__XOPEN_OR_POSIX)) || \ (_POSIX_C_SOURCE > 2) || defined(_XPG4_2) void (*_sigaction)(int, siginfo_t *, void *); #endif } _funcptr; sigset_t sa_mask; #ifndef _LP64 int sa_resv[2]; #endif }; #define sa_handler _funcptr._handler #define sa_sigaction _funcptr._sigaction > Why is the code accepted by the C compiler in the first place? ... which explains why the C compiler accepts the code as is. Rainer
> ... which explains why the C compiler accepts the code as is.
Thanks for the details. Patch is OK then.
Arno
On Wed, Jul 20, 2011 at 18:35, Rainer Orth <ro@cebitec.uni-bielefeld.de> wrote: > I've hacked around this by wrapping the AM_ICONV calls in > AC_LANG_{PUSH, POP}(C++), but I think this exposes a fundamental > issue: the configure tests must be performed with the compiler used > for the build. That this works without is pure luck IMO. Right, but it also applies to more than this test. If you wrapped in ifs more than just this call, the approach may be fine, but first I would like to look at Autoconf (or at the diff for the regenerated configure) to check that what you're doing is safe. I'm afraid it may not be, which means you're patch is not good for the configure part. :( > * Also, the definition of HAVE_DESIGNATED_INITIALIZERS was wrong for g++ > on Solaris which again defines __STDC_VERSION__ 199901L. To fix this, > I never define H_D_I if __cplusplus. Is this a valid definition of __STDC_VERSION__ at all? Paolo
Paolo, > On Wed, Jul 20, 2011 at 18:35, Rainer Orth <ro@cebitec.uni-bielefeld.de> wrote: >> I've hacked around this by wrapping the AM_ICONV calls in >> AC_LANG_{PUSH, POP}(C++), but I think this exposes a fundamental >> issue: the configure tests must be performed with the compiler used >> for the build. That this works without is pure luck IMO. > > Right, but it also applies to more than this test. If you wrapped in > ifs more than just this call, the approach may be fine, but first I > would like to look at Autoconf (or at the diff for the regenerated > configure) to check that what you're doing is safe. I'm afraid it may the configure diff looked completely innocent to me, but that may be just me ;-( > not be, which means you're patch is not good for the configure part. > :( Sorry, I only say your mail after the weekend, and have already checked in the patch based on Ian's approval. At least so far I've not become aware of any problems caused by the patch. >> * Also, the definition of HAVE_DESIGNATED_INITIALIZERS was wrong for g++ >> on Solaris which again defines __STDC_VERSION__ 199901L. To fix this, >> I never define H_D_I if __cplusplus. > > Is this a valid definition of __STDC_VERSION__ at all? Why wouldn't it be? It's the standard C99 value. Rainer
On 07/25/2011 07:00 PM, Rainer Orth wrote: >>> >> * Also, the definition of HAVE_DESIGNATED_INITIALIZERS was wrong for g++ >>> >> on Solaris which again defines __STDC_VERSION__ 199901L. To fix this, >>> >> I never define H_D_I if __cplusplus. >> > >> > Is this a valid definition of __STDC_VERSION__ at all? > Why wouldn't it be? It's the standard C99 value. Replying to an old message, yes, it's standard C99, but C++ does not support many of the differences between C89 and C99. I would have thought the C89 definition to be more conservatively correct. Paolo
diff --git a/gcc/ada/init.c b/gcc/ada/init.c --- a/gcc/ada/init.c +++ b/gcc/ada/init.c @@ -1031,7 +1031,7 @@ __gnat_install_handler (void) exceptions. Make sure that the handler isn't interrupted by another signal that might cause a scheduling event! */ - act.sa_handler = __gnat_error_handler; + act.sa_sigaction = __gnat_error_handler; act.sa_flags = SA_NODEFER | SA_RESTART | SA_SIGINFO; sigemptyset (&act.sa_mask); diff --git a/gcc/ada/tracebak.c b/gcc/ada/tracebak.c --- a/gcc/ada/tracebak.c +++ b/gcc/ada/tracebak.c @@ -482,12 +482,12 @@ __gnat_backtrace (void **array, while (cnt < size) { if (STOP_FRAME (current, top_stack) || - !VALID_STACK_FRAME((char *)(current->return_address + PC_ADJUST))) + !VALID_STACK_FRAME(((char *) current->return_address) + PC_ADJUST)) break; if (current->return_address < exclude_min || current->return_address > exclude_max) - array[cnt++] = current->return_address + PC_ADJUST; + array[cnt++] = ((char *) current->return_address) + PC_ADJUST; current = (struct layout *) ((size_t) current->next + FRAME_OFFSET (1)); } diff --git a/gcc/config/sol2-c.c b/gcc/config/sol2-c.c --- a/gcc/config/sol2-c.c +++ b/gcc/config/sol2-c.c @@ -68,7 +68,7 @@ static const format_char_info cmn_err_ch { NULL, 0, STD_C89, NOLENGTHS, NULL, NULL, NULL } }; -const format_kind_info solaris_format_types[] = { +extern const format_kind_info solaris_format_types[] = { { "cmn_err", cmn_err_length_specs, cmn_err_char_table, "", NULL, cmn_err_flag_specs, cmn_err_flag_pairs, FMT_FLAG_ARG_CONVERT|FMT_FLAG_EMPTY_PREC_OK, diff --git a/gcc/configure.ac b/gcc/configure.ac --- a/gcc/configure.ac +++ b/gcc/configure.ac @@ -1041,7 +1041,16 @@ case "${host}" in esac AC_FUNC_FORK +# FIXME: g++ on Solaris 10+ defines _XOPEN_SOURCE=600, which exposes a +# different iconv() prototype. +if test "$ENABLE_BUILD_WITH_CXX" = "yes"; then +AC_LANG_PUSH([C++]) +fi AM_ICONV +if test "$ENABLE_BUILD_WITH_CXX" = "yes"; then +AC_LANG_POP([C++]) +fi + # Until we have in-tree GNU iconv: LIBICONV_DEP= AC_SUBST(LIBICONV_DEP) diff --git a/libcpp/configure.ac b/libcpp/configure.ac --- a/libcpp/configure.ac +++ b/libcpp/configure.ac @@ -102,7 +102,15 @@ if test $ac_cv_type_uchar = yes; then [Define if <sys/types.h> defines \`uchar'.]) fi +# FIXME: g++ on Solaris 10+ defines _XOPEN_SOURCE=600, which exposes a +# different iconv() prototype. +if test "$ENABLE_BUILD_WITH_CXX" = "yes"; then +AC_LANG_PUSH([C++]) +fi AM_ICONV +if test "$ENABLE_BUILD_WITH_CXX" = "yes"; then +AC_LANG_POP([C++]) +fi # More defines and substitutions. PACKAGE="$PACKAGE_TARNAME" diff --git a/libcpp/system.h b/libcpp/system.h --- a/libcpp/system.h +++ b/libcpp/system.h @@ -1,6 +1,6 @@ /* Get common system includes and various definitions and declarations based on autoconf macros. - Copyright (C) 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2009, 2010 + Copyright (C) 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2009, 2010, 2011 Free Software Foundation, Inc. This file is part of GCC. @@ -353,8 +353,8 @@ extern void abort (void); compilers, including G++. -- gdr, 2005-05-18 */ #if !defined(HAVE_DESIGNATED_INITIALIZERS) #define HAVE_DESIGNATED_INITIALIZERS \ - ((!defined(__cplusplus) && (GCC_VERSION >= 2007)) \ - || (__STDC_VERSION__ >= 199901L)) + (!defined(__cplusplus) \ + && ((GCC_VERSION >= 2007) || (__STDC_VERSION__ >= 199901L))) #endif #ifndef offsetof
As partially described in the PR, Solaris 10 bootstrap is broken with C++: * In both libcpp and gcc, ICONV_CONST is misdetected since the configure test is run with gcc, but the actual compilation performed with g++. Unlike the former, the latter defines _XOPEN_SOURCE=600 (for the benefit of libstdc++, among others), exposing a different iconv() prototype. I've hacked around this by wrapping the AM_ICONV calls in AC_LANG_{PUSH, POP}(C++), but I think this exposes a fundamental issue: the configure tests must be performed with the compiler used for the build. That this works without is pure luck IMO. * Also, the definition of HAVE_DESIGNATED_INITIALIZERS was wrong for g++ on Solaris which again defines __STDC_VERSION__ 199901L. To fix this, I never define H_D_I if __cplusplus. * Similarly to IRIX 6, there was a mismatch in ada/init.c for the type of the signal handler installed with sigaction. g++ also doesn't like arithmetic on void * ;-) * The last issue I don't understand at all, not being a C++ person: initially, const format_kind_info solaris_format_types[] from config/sol2-c.c wasn't even emitted into the object file. With __attribute__((used)) added, it was at least present in the .o file, but still LOCL. Only declaring it extern fixed this, no idea why this is necessary. With those fixes, an i386-pc-solaris2.10 bootstrap has just completed the build and is running the testsuite. I suppose the Ada and libcpp/system.h parts might be ok as is, but for the iconv() issue we certainly need a clean solution, not a hack like this. Rainer 2011-07-20 Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> gcc: PR bootstrap/49794 * configure.ac: Test AM_ICONV with CXX. * configure: Regenerate. * config/sol2-c.c (solaris_format_types): Declare extern. gcc/ada: PR bootstrap/49794 * init.c [sun && __SVR4 && !__vxworks] (__gnat_install_handler): Assign to act.sa_sigaction. * tracebak.c [USE_GENERIC_UNWINDER] (__gnat_backtrace): Cast current->return_address to char * before arithmetic. libcpp: PR bootstrap/49794 * configure.ac: Test AM_ICONV with CXX. * configure: Regenerate. * system.h (HAVE_DESIGNATED_INITIALIZERS): Never define for C++.