| Message ID | 20200915150216.1079662-1-stefansf@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | S/390: Do not turn maybe-uninitialized warnings into errors | expand |
On 15.09.20 17:02, Stefan Schulze Frielinghaus wrote: > Over the last couple of months quite a few warnings about uninitialized > variables were raised while building GCC. A reason why these warnings > show up on S/390 only is due to the aggressive inlining settings here. > Some of these warnings (2c832ffedf0, b776bdca932, 2786c0221b6, > 1657178f59b) could be fixed or in case of a false positive silenced by > initializing the corresponding variable. Since the latter reoccurs and > while bootstrapping such warnings are turned into errors bootstrapping > fails on S/390 consistently. Therefore, for the moment do not turn > those warnings into errors. > > config/ChangeLog: > > * warnings.m4: Do not turn maybe-uninitialized warnings into errors > on S/390. > > fixincludes/ChangeLog: > > * configure: Regenerate. > > gcc/ChangeLog: > > * configure: Regenerate. > > libcc1/ChangeLog: > > * configure: Regenerate. > > libcpp/ChangeLog: > > * configure: Regenerate. > > libdecnumber/ChangeLog: > > * configure: Regenerate. That change looks good to me. Could a global reviewer please comment! Andreas > --- > config/warnings.m4 | 20 ++++++++++++++------ > fixincludes/configure | 8 +++++++- > gcc/configure | 12 +++++++++--- > libcc1/configure | 8 +++++++- > libcpp/configure | 8 +++++++- > libdecnumber/configure | 8 +++++++- > 6 files changed, 51 insertions(+), 13 deletions(-) > > diff --git a/config/warnings.m4 b/config/warnings.m4 > index ce007f9b73e..d977bfb20af 100644 > --- a/config/warnings.m4 > +++ b/config/warnings.m4 > @@ -101,8 +101,10 @@ AC_ARG_ENABLE(werror-always, > AS_HELP_STRING([--enable-werror-always], > [enable -Werror despite compiler version]), > [], [enable_werror_always=no]) > -AS_IF([test $enable_werror_always = yes], > - [acx_Var="$acx_Var${acx_Var:+ }-Werror"]) > +AS_IF([test $enable_werror_always = yes], [dnl > + acx_Var="$acx_Var${acx_Var:+ }-Werror" > + AS_CASE([$host], [s390*-*-*], > + [acx_Var="$acx_Var -Wno-error=maybe-uninitialized"])]) > m4_if($1, [manual],, > [AS_VAR_PUSHDEF([acx_GCCvers], [acx_cv_prog_cc_gcc_$1_or_newer])dnl > AC_CACHE_CHECK([whether $CC is GCC >=$1], acx_GCCvers, > @@ -116,7 +118,9 @@ AS_IF([test $enable_werror_always = yes], > [AS_VAR_SET(acx_GCCvers, yes)], > [AS_VAR_SET(acx_GCCvers, no)])]) > AS_IF([test AS_VAR_GET(acx_GCCvers) = yes], > - [acx_Var="$acx_Var${acx_Var:+ }-Werror"]) > + [acx_Var="$acx_Var${acx_Var:+ }-Werror" > + AS_CASE([$host], [s390*-*-*], > + [acx_Var="$acx_Var -Wno-error=maybe-uninitialized"])]) > AS_VAR_POPDEF([acx_GCCvers])]) > m4_popdef([acx_Var])dnl > AC_LANG_POP(C) > @@ -205,8 +209,10 @@ AC_ARG_ENABLE(werror-always, > AS_HELP_STRING([--enable-werror-always], > [enable -Werror despite compiler version]), > [], [enable_werror_always=no]) > -AS_IF([test $enable_werror_always = yes], > - [acx_Var="$acx_Var${acx_Var:+ }-Werror"]) > +AS_IF([test $enable_werror_always = yes], [dnl > + acx_Var="$acx_Var${acx_Var:+ }-Werror" > + AS_CASE([$host], [s390*-*-*], > + [strict_warn="$strict_warn -Wno-error=maybe-uninitialized"])]) > m4_if($1, [manual],, > [AS_VAR_PUSHDEF([acx_GXXvers], [acx_cv_prog_cxx_gxx_$1_or_newer])dnl > AC_CACHE_CHECK([whether $CXX is G++ >=$1], acx_GXXvers, > @@ -220,7 +226,9 @@ AS_IF([test $enable_werror_always = yes], > [AS_VAR_SET(acx_GXXvers, yes)], > [AS_VAR_SET(acx_GXXvers, no)])]) > AS_IF([test AS_VAR_GET(acx_GXXvers) = yes], > - [acx_Var="$acx_Var${acx_Var:+ }-Werror"]) > + [acx_Var="$acx_Var${acx_Var:+ }-Werror" > + AS_CASE([$host], [s390*-*-*], > + [acx_Var="$acx_Var -Wno-error=maybe-uninitialized"])]) > AS_VAR_POPDEF([acx_GXXvers])]) > m4_popdef([acx_Var])dnl > AC_LANG_POP(C++) > diff --git a/fixincludes/configure b/fixincludes/configure > index 6e2d67b655b..e0d679cc18e 100755 > --- a/fixincludes/configure > +++ b/fixincludes/configure > @@ -4753,7 +4753,13 @@ else > fi > > if test $enable_werror_always = yes; then : > - WERROR="$WERROR${WERROR:+ }-Werror" > + WERROR="$WERROR${WERROR:+ }-Werror" > + case $host in #( > + s390*-*-*) : > + WERROR="$WERROR -Wno-error=maybe-uninitialized" ;; #( > + *) : > + ;; > +esac > fi > > ac_ext=c > diff --git a/gcc/configure b/gcc/configure > index 0a09777dd42..ea03581537a 100755 > --- a/gcc/configure > +++ b/gcc/configure > @@ -7064,7 +7064,13 @@ else > fi > > if test $enable_werror_always = yes; then : > - strict_warn="$strict_warn${strict_warn:+ }-Werror" > + strict_warn="$strict_warn${strict_warn:+ }-Werror" > + case $host in #( > + s390*-*-*) : > + strict_warn="$strict_warn -Wno-error=maybe-uninitialized" ;; #( > + *) : > + ;; > +esac > fi > > ac_ext=cpp > @@ -19013,7 +19019,7 @@ else > lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 > lt_status=$lt_dlunknown > cat > conftest.$ac_ext <<_LT_EOF > -#line 19016 "configure" > +#line 19022 "configure" > #include "confdefs.h" > > #if HAVE_DLFCN_H > @@ -19119,7 +19125,7 @@ else > lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 > lt_status=$lt_dlunknown > cat > conftest.$ac_ext <<_LT_EOF > -#line 19122 "configure" > +#line 19128 "configure" > #include "confdefs.h" > > #if HAVE_DLFCN_H > diff --git a/libcc1/configure b/libcc1/configure > index 3610219ba65..e95274d0d46 100755 > --- a/libcc1/configure > +++ b/libcc1/configure > @@ -14835,7 +14835,13 @@ else > fi > > if test $enable_werror_always = yes; then : > - WERROR="$WERROR${WERROR:+ }-Werror" > + WERROR="$WERROR${WERROR:+ }-Werror" > + case $host in #( > + s390*-*-*) : > + WERROR="$WERROR -Wno-error=maybe-uninitialized" ;; #( > + *) : > + ;; > +esac > fi > > ac_ext=c > diff --git a/libcpp/configure b/libcpp/configure > index 7e28606f605..db378c363ee 100755 > --- a/libcpp/configure > +++ b/libcpp/configure > @@ -5142,7 +5142,13 @@ else > fi > > if test $enable_werror_always = yes; then : > - WERROR="$WERROR${WERROR:+ }-Werror" > + WERROR="$WERROR${WERROR:+ }-Werror" > + case $host in #( > + s390*-*-*) : > + WERROR="$WERROR -Wno-error=maybe-uninitialized" ;; #( > + *) : > + ;; > +esac > fi > > ac_ext=c > diff --git a/libdecnumber/configure b/libdecnumber/configure > index 6b62f8ba665..7035aef58dd 100755 > --- a/libdecnumber/configure > +++ b/libdecnumber/configure > @@ -3661,7 +3661,13 @@ else > fi > > if test $enable_werror_always = yes; then : > - WERROR="$WERROR${WERROR:+ }-Werror" > + WERROR="$WERROR${WERROR:+ }-Werror" > + case $host in #( > + s390*-*-*) : > + WERROR="$WERROR -Wno-error=maybe-uninitialized" ;; #( > + *) : > + ;; > +esac > fi > > ac_ext=c >
On Tue, Sep 22, 2020 at 02:59:30PM +0200, Andreas Krebbel wrote: > On 15.09.20 17:02, Stefan Schulze Frielinghaus wrote: > > Over the last couple of months quite a few warnings about uninitialized > > variables were raised while building GCC. A reason why these warnings > > show up on S/390 only is due to the aggressive inlining settings here. > > Some of these warnings (2c832ffedf0, b776bdca932, 2786c0221b6, > > 1657178f59b) could be fixed or in case of a false positive silenced by > > initializing the corresponding variable. Since the latter reoccurs and > > while bootstrapping such warnings are turned into errors bootstrapping > > fails on S/390 consistently. Therefore, for the moment do not turn > > those warnings into errors. > > > > config/ChangeLog: > > > > * warnings.m4: Do not turn maybe-uninitialized warnings into errors > > on S/390. > > > > fixincludes/ChangeLog: > > > > * configure: Regenerate. > > > > gcc/ChangeLog: > > > > * configure: Regenerate. > > > > libcc1/ChangeLog: > > > > * configure: Regenerate. > > > > libcpp/ChangeLog: > > > > * configure: Regenerate. > > > > libdecnumber/ChangeLog: > > > > * configure: Regenerate. > > That change looks good to me. Could a global reviewer please comment! Ping > > Andreas > > > --- > > config/warnings.m4 | 20 ++++++++++++++------ > > fixincludes/configure | 8 +++++++- > > gcc/configure | 12 +++++++++--- > > libcc1/configure | 8 +++++++- > > libcpp/configure | 8 +++++++- > > libdecnumber/configure | 8 +++++++- > > 6 files changed, 51 insertions(+), 13 deletions(-) > > > > diff --git a/config/warnings.m4 b/config/warnings.m4 > > index ce007f9b73e..d977bfb20af 100644 > > --- a/config/warnings.m4 > > +++ b/config/warnings.m4 > > @@ -101,8 +101,10 @@ AC_ARG_ENABLE(werror-always, > > AS_HELP_STRING([--enable-werror-always], > > [enable -Werror despite compiler version]), > > [], [enable_werror_always=no]) > > -AS_IF([test $enable_werror_always = yes], > > - [acx_Var="$acx_Var${acx_Var:+ }-Werror"]) > > +AS_IF([test $enable_werror_always = yes], [dnl > > + acx_Var="$acx_Var${acx_Var:+ }-Werror" > > + AS_CASE([$host], [s390*-*-*], > > + [acx_Var="$acx_Var -Wno-error=maybe-uninitialized"])]) > > m4_if($1, [manual],, > > [AS_VAR_PUSHDEF([acx_GCCvers], [acx_cv_prog_cc_gcc_$1_or_newer])dnl > > AC_CACHE_CHECK([whether $CC is GCC >=$1], acx_GCCvers, > > @@ -116,7 +118,9 @@ AS_IF([test $enable_werror_always = yes], > > [AS_VAR_SET(acx_GCCvers, yes)], > > [AS_VAR_SET(acx_GCCvers, no)])]) > > AS_IF([test AS_VAR_GET(acx_GCCvers) = yes], > > - [acx_Var="$acx_Var${acx_Var:+ }-Werror"]) > > + [acx_Var="$acx_Var${acx_Var:+ }-Werror" > > + AS_CASE([$host], [s390*-*-*], > > + [acx_Var="$acx_Var -Wno-error=maybe-uninitialized"])]) > > AS_VAR_POPDEF([acx_GCCvers])]) > > m4_popdef([acx_Var])dnl > > AC_LANG_POP(C) > > @@ -205,8 +209,10 @@ AC_ARG_ENABLE(werror-always, > > AS_HELP_STRING([--enable-werror-always], > > [enable -Werror despite compiler version]), > > [], [enable_werror_always=no]) > > -AS_IF([test $enable_werror_always = yes], > > - [acx_Var="$acx_Var${acx_Var:+ }-Werror"]) > > +AS_IF([test $enable_werror_always = yes], [dnl > > + acx_Var="$acx_Var${acx_Var:+ }-Werror" > > + AS_CASE([$host], [s390*-*-*], > > + [strict_warn="$strict_warn -Wno-error=maybe-uninitialized"])]) > > m4_if($1, [manual],, > > [AS_VAR_PUSHDEF([acx_GXXvers], [acx_cv_prog_cxx_gxx_$1_or_newer])dnl > > AC_CACHE_CHECK([whether $CXX is G++ >=$1], acx_GXXvers, > > @@ -220,7 +226,9 @@ AS_IF([test $enable_werror_always = yes], > > [AS_VAR_SET(acx_GXXvers, yes)], > > [AS_VAR_SET(acx_GXXvers, no)])]) > > AS_IF([test AS_VAR_GET(acx_GXXvers) = yes], > > - [acx_Var="$acx_Var${acx_Var:+ }-Werror"]) > > + [acx_Var="$acx_Var${acx_Var:+ }-Werror" > > + AS_CASE([$host], [s390*-*-*], > > + [acx_Var="$acx_Var -Wno-error=maybe-uninitialized"])]) > > AS_VAR_POPDEF([acx_GXXvers])]) > > m4_popdef([acx_Var])dnl > > AC_LANG_POP(C++) > > diff --git a/fixincludes/configure b/fixincludes/configure > > index 6e2d67b655b..e0d679cc18e 100755 > > --- a/fixincludes/configure > > +++ b/fixincludes/configure > > @@ -4753,7 +4753,13 @@ else > > fi > > > > if test $enable_werror_always = yes; then : > > - WERROR="$WERROR${WERROR:+ }-Werror" > > + WERROR="$WERROR${WERROR:+ }-Werror" > > + case $host in #( > > + s390*-*-*) : > > + WERROR="$WERROR -Wno-error=maybe-uninitialized" ;; #( > > + *) : > > + ;; > > +esac > > fi > > > > ac_ext=c > > diff --git a/gcc/configure b/gcc/configure > > index 0a09777dd42..ea03581537a 100755 > > --- a/gcc/configure > > +++ b/gcc/configure > > @@ -7064,7 +7064,13 @@ else > > fi > > > > if test $enable_werror_always = yes; then : > > - strict_warn="$strict_warn${strict_warn:+ }-Werror" > > + strict_warn="$strict_warn${strict_warn:+ }-Werror" > > + case $host in #( > > + s390*-*-*) : > > + strict_warn="$strict_warn -Wno-error=maybe-uninitialized" ;; #( > > + *) : > > + ;; > > +esac > > fi > > > > ac_ext=cpp > > @@ -19013,7 +19019,7 @@ else > > lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 > > lt_status=$lt_dlunknown > > cat > conftest.$ac_ext <<_LT_EOF > > -#line 19016 "configure" > > +#line 19022 "configure" > > #include "confdefs.h" > > > > #if HAVE_DLFCN_H > > @@ -19119,7 +19125,7 @@ else > > lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 > > lt_status=$lt_dlunknown > > cat > conftest.$ac_ext <<_LT_EOF > > -#line 19122 "configure" > > +#line 19128 "configure" > > #include "confdefs.h" > > > > #if HAVE_DLFCN_H > > diff --git a/libcc1/configure b/libcc1/configure > > index 3610219ba65..e95274d0d46 100755 > > --- a/libcc1/configure > > +++ b/libcc1/configure > > @@ -14835,7 +14835,13 @@ else > > fi > > > > if test $enable_werror_always = yes; then : > > - WERROR="$WERROR${WERROR:+ }-Werror" > > + WERROR="$WERROR${WERROR:+ }-Werror" > > + case $host in #( > > + s390*-*-*) : > > + WERROR="$WERROR -Wno-error=maybe-uninitialized" ;; #( > > + *) : > > + ;; > > +esac > > fi > > > > ac_ext=c > > diff --git a/libcpp/configure b/libcpp/configure > > index 7e28606f605..db378c363ee 100755 > > --- a/libcpp/configure > > +++ b/libcpp/configure > > @@ -5142,7 +5142,13 @@ else > > fi > > > > if test $enable_werror_always = yes; then : > > - WERROR="$WERROR${WERROR:+ }-Werror" > > + WERROR="$WERROR${WERROR:+ }-Werror" > > + case $host in #( > > + s390*-*-*) : > > + WERROR="$WERROR -Wno-error=maybe-uninitialized" ;; #( > > + *) : > > + ;; > > +esac > > fi > > > > ac_ext=c > > diff --git a/libdecnumber/configure b/libdecnumber/configure > > index 6b62f8ba665..7035aef58dd 100755 > > --- a/libdecnumber/configure > > +++ b/libdecnumber/configure > > @@ -3661,7 +3661,13 @@ else > > fi > > > > if test $enable_werror_always = yes; then : > > - WERROR="$WERROR${WERROR:+ }-Werror" > > + WERROR="$WERROR${WERROR:+ }-Werror" > > + case $host in #( > > + s390*-*-*) : > > + WERROR="$WERROR -Wno-error=maybe-uninitialized" ;; #( > > + *) : > > + ;; > > +esac > > fi > > > > ac_ext=c > > >
On Mon, Oct 05, 2020 at 02:02:57PM +0200, Stefan Schulze Frielinghaus via Gcc-patches wrote: > On Tue, Sep 22, 2020 at 02:59:30PM +0200, Andreas Krebbel wrote: > > On 15.09.20 17:02, Stefan Schulze Frielinghaus wrote: > > > Over the last couple of months quite a few warnings about uninitialized > > > variables were raised while building GCC. A reason why these warnings > > > show up on S/390 only is due to the aggressive inlining settings here. > > > Some of these warnings (2c832ffedf0, b776bdca932, 2786c0221b6, > > > 1657178f59b) could be fixed or in case of a false positive silenced by > > > initializing the corresponding variable. Since the latter reoccurs and > > > while bootstrapping such warnings are turned into errors bootstrapping > > > fails on S/390 consistently. Therefore, for the moment do not turn > > > those warnings into errors. > > > > > > config/ChangeLog: > > > > > > * warnings.m4: Do not turn maybe-uninitialized warnings into errors > > > on S/390. > > > > > > fixincludes/ChangeLog: > > > > > > * configure: Regenerate. > > > > > > gcc/ChangeLog: > > > > > > * configure: Regenerate. > > > > > > libcc1/ChangeLog: > > > > > > * configure: Regenerate. > > > > > > libcpp/ChangeLog: > > > > > > * configure: Regenerate. > > > > > > libdecnumber/ChangeLog: > > > > > > * configure: Regenerate. > > > > That change looks good to me. Could a global reviewer please comment! > > Ping Ping > > > > > Andreas > > > > > --- > > > config/warnings.m4 | 20 ++++++++++++++------ > > > fixincludes/configure | 8 +++++++- > > > gcc/configure | 12 +++++++++--- > > > libcc1/configure | 8 +++++++- > > > libcpp/configure | 8 +++++++- > > > libdecnumber/configure | 8 +++++++- > > > 6 files changed, 51 insertions(+), 13 deletions(-) > > > > > > diff --git a/config/warnings.m4 b/config/warnings.m4 > > > index ce007f9b73e..d977bfb20af 100644 > > > --- a/config/warnings.m4 > > > +++ b/config/warnings.m4 > > > @@ -101,8 +101,10 @@ AC_ARG_ENABLE(werror-always, > > > AS_HELP_STRING([--enable-werror-always], > > > [enable -Werror despite compiler version]), > > > [], [enable_werror_always=no]) > > > -AS_IF([test $enable_werror_always = yes], > > > - [acx_Var="$acx_Var${acx_Var:+ }-Werror"]) > > > +AS_IF([test $enable_werror_always = yes], [dnl > > > + acx_Var="$acx_Var${acx_Var:+ }-Werror" > > > + AS_CASE([$host], [s390*-*-*], > > > + [acx_Var="$acx_Var -Wno-error=maybe-uninitialized"])]) > > > m4_if($1, [manual],, > > > [AS_VAR_PUSHDEF([acx_GCCvers], [acx_cv_prog_cc_gcc_$1_or_newer])dnl > > > AC_CACHE_CHECK([whether $CC is GCC >=$1], acx_GCCvers, > > > @@ -116,7 +118,9 @@ AS_IF([test $enable_werror_always = yes], > > > [AS_VAR_SET(acx_GCCvers, yes)], > > > [AS_VAR_SET(acx_GCCvers, no)])]) > > > AS_IF([test AS_VAR_GET(acx_GCCvers) = yes], > > > - [acx_Var="$acx_Var${acx_Var:+ }-Werror"]) > > > + [acx_Var="$acx_Var${acx_Var:+ }-Werror" > > > + AS_CASE([$host], [s390*-*-*], > > > + [acx_Var="$acx_Var -Wno-error=maybe-uninitialized"])]) > > > AS_VAR_POPDEF([acx_GCCvers])]) > > > m4_popdef([acx_Var])dnl > > > AC_LANG_POP(C) > > > @@ -205,8 +209,10 @@ AC_ARG_ENABLE(werror-always, > > > AS_HELP_STRING([--enable-werror-always], > > > [enable -Werror despite compiler version]), > > > [], [enable_werror_always=no]) > > > -AS_IF([test $enable_werror_always = yes], > > > - [acx_Var="$acx_Var${acx_Var:+ }-Werror"]) > > > +AS_IF([test $enable_werror_always = yes], [dnl > > > + acx_Var="$acx_Var${acx_Var:+ }-Werror" > > > + AS_CASE([$host], [s390*-*-*], > > > + [strict_warn="$strict_warn -Wno-error=maybe-uninitialized"])]) > > > m4_if($1, [manual],, > > > [AS_VAR_PUSHDEF([acx_GXXvers], [acx_cv_prog_cxx_gxx_$1_or_newer])dnl > > > AC_CACHE_CHECK([whether $CXX is G++ >=$1], acx_GXXvers, > > > @@ -220,7 +226,9 @@ AS_IF([test $enable_werror_always = yes], > > > [AS_VAR_SET(acx_GXXvers, yes)], > > > [AS_VAR_SET(acx_GXXvers, no)])]) > > > AS_IF([test AS_VAR_GET(acx_GXXvers) = yes], > > > - [acx_Var="$acx_Var${acx_Var:+ }-Werror"]) > > > + [acx_Var="$acx_Var${acx_Var:+ }-Werror" > > > + AS_CASE([$host], [s390*-*-*], > > > + [acx_Var="$acx_Var -Wno-error=maybe-uninitialized"])]) > > > AS_VAR_POPDEF([acx_GXXvers])]) > > > m4_popdef([acx_Var])dnl > > > AC_LANG_POP(C++) > > > diff --git a/fixincludes/configure b/fixincludes/configure > > > index 6e2d67b655b..e0d679cc18e 100755 > > > --- a/fixincludes/configure > > > +++ b/fixincludes/configure > > > @@ -4753,7 +4753,13 @@ else > > > fi > > > > > > if test $enable_werror_always = yes; then : > > > - WERROR="$WERROR${WERROR:+ }-Werror" > > > + WERROR="$WERROR${WERROR:+ }-Werror" > > > + case $host in #( > > > + s390*-*-*) : > > > + WERROR="$WERROR -Wno-error=maybe-uninitialized" ;; #( > > > + *) : > > > + ;; > > > +esac > > > fi > > > > > > ac_ext=c > > > diff --git a/gcc/configure b/gcc/configure > > > index 0a09777dd42..ea03581537a 100755 > > > --- a/gcc/configure > > > +++ b/gcc/configure > > > @@ -7064,7 +7064,13 @@ else > > > fi > > > > > > if test $enable_werror_always = yes; then : > > > - strict_warn="$strict_warn${strict_warn:+ }-Werror" > > > + strict_warn="$strict_warn${strict_warn:+ }-Werror" > > > + case $host in #( > > > + s390*-*-*) : > > > + strict_warn="$strict_warn -Wno-error=maybe-uninitialized" ;; #( > > > + *) : > > > + ;; > > > +esac > > > fi > > > > > > ac_ext=cpp > > > @@ -19013,7 +19019,7 @@ else > > > lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 > > > lt_status=$lt_dlunknown > > > cat > conftest.$ac_ext <<_LT_EOF > > > -#line 19016 "configure" > > > +#line 19022 "configure" > > > #include "confdefs.h" > > > > > > #if HAVE_DLFCN_H > > > @@ -19119,7 +19125,7 @@ else > > > lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 > > > lt_status=$lt_dlunknown > > > cat > conftest.$ac_ext <<_LT_EOF > > > -#line 19122 "configure" > > > +#line 19128 "configure" > > > #include "confdefs.h" > > > > > > #if HAVE_DLFCN_H > > > diff --git a/libcc1/configure b/libcc1/configure > > > index 3610219ba65..e95274d0d46 100755 > > > --- a/libcc1/configure > > > +++ b/libcc1/configure > > > @@ -14835,7 +14835,13 @@ else > > > fi > > > > > > if test $enable_werror_always = yes; then : > > > - WERROR="$WERROR${WERROR:+ }-Werror" > > > + WERROR="$WERROR${WERROR:+ }-Werror" > > > + case $host in #( > > > + s390*-*-*) : > > > + WERROR="$WERROR -Wno-error=maybe-uninitialized" ;; #( > > > + *) : > > > + ;; > > > +esac > > > fi > > > > > > ac_ext=c > > > diff --git a/libcpp/configure b/libcpp/configure > > > index 7e28606f605..db378c363ee 100755 > > > --- a/libcpp/configure > > > +++ b/libcpp/configure > > > @@ -5142,7 +5142,13 @@ else > > > fi > > > > > > if test $enable_werror_always = yes; then : > > > - WERROR="$WERROR${WERROR:+ }-Werror" > > > + WERROR="$WERROR${WERROR:+ }-Werror" > > > + case $host in #( > > > + s390*-*-*) : > > > + WERROR="$WERROR -Wno-error=maybe-uninitialized" ;; #( > > > + *) : > > > + ;; > > > +esac > > > fi > > > > > > ac_ext=c > > > diff --git a/libdecnumber/configure b/libdecnumber/configure > > > index 6b62f8ba665..7035aef58dd 100755 > > > --- a/libdecnumber/configure > > > +++ b/libdecnumber/configure > > > @@ -3661,7 +3661,13 @@ else > > > fi > > > > > > if test $enable_werror_always = yes; then : > > > - WERROR="$WERROR${WERROR:+ }-Werror" > > > + WERROR="$WERROR${WERROR:+ }-Werror" > > > + case $host in #( > > > + s390*-*-*) : > > > + WERROR="$WERROR -Wno-error=maybe-uninitialized" ;; #( > > > + *) : > > > + ;; > > > +esac > > > fi > > > > > > ac_ext=c > > > > >
On 10/28/20 3:38 AM, Stefan Schulze Frielinghaus via Gcc-patches wrote: > On Mon, Oct 05, 2020 at 02:02:57PM +0200, Stefan Schulze Frielinghaus via Gcc-patches wrote: >> On Tue, Sep 22, 2020 at 02:59:30PM +0200, Andreas Krebbel wrote: >>> On 15.09.20 17:02, Stefan Schulze Frielinghaus wrote: >>>> Over the last couple of months quite a few warnings about uninitialized >>>> variables were raised while building GCC. A reason why these warnings >>>> show up on S/390 only is due to the aggressive inlining settings here. >>>> Some of these warnings (2c832ffedf0, b776bdca932, 2786c0221b6, >>>> 1657178f59b) could be fixed or in case of a false positive silenced by >>>> initializing the corresponding variable. Since the latter reoccurs and >>>> while bootstrapping such warnings are turned into errors bootstrapping >>>> fails on S/390 consistently. Therefore, for the moment do not turn >>>> those warnings into errors. >>>> >>>> config/ChangeLog: >>>> >>>> * warnings.m4: Do not turn maybe-uninitialized warnings into errors >>>> on S/390. >>>> >>>> fixincludes/ChangeLog: >>>> >>>> * configure: Regenerate. >>>> >>>> gcc/ChangeLog: >>>> >>>> * configure: Regenerate. >>>> >>>> libcc1/ChangeLog: >>>> >>>> * configure: Regenerate. >>>> >>>> libcpp/ChangeLog: >>>> >>>> * configure: Regenerate. >>>> >>>> libdecnumber/ChangeLog: >>>> >>>> * configure: Regenerate. >>> That change looks good to me. Could a global reviewer please comment! >> Ping > Ping I think this would be a huge mistake to install. Jeff
On Wed, Oct 28, 2020 at 08:39:41AM -0600, Jeff Law wrote: > > On 10/28/20 3:38 AM, Stefan Schulze Frielinghaus via Gcc-patches wrote: > > On Mon, Oct 05, 2020 at 02:02:57PM +0200, Stefan Schulze Frielinghaus via Gcc-patches wrote: > >> On Tue, Sep 22, 2020 at 02:59:30PM +0200, Andreas Krebbel wrote: > >>> On 15.09.20 17:02, Stefan Schulze Frielinghaus wrote: > >>>> Over the last couple of months quite a few warnings about uninitialized > >>>> variables were raised while building GCC. A reason why these warnings > >>>> show up on S/390 only is due to the aggressive inlining settings here. > >>>> Some of these warnings (2c832ffedf0, b776bdca932, 2786c0221b6, > >>>> 1657178f59b) could be fixed or in case of a false positive silenced by > >>>> initializing the corresponding variable. Since the latter reoccurs and > >>>> while bootstrapping such warnings are turned into errors bootstrapping > >>>> fails on S/390 consistently. Therefore, for the moment do not turn > >>>> those warnings into errors. > >>>> > >>>> config/ChangeLog: > >>>> > >>>> * warnings.m4: Do not turn maybe-uninitialized warnings into errors > >>>> on S/390. > >>>> > >>>> fixincludes/ChangeLog: > >>>> > >>>> * configure: Regenerate. > >>>> > >>>> gcc/ChangeLog: > >>>> > >>>> * configure: Regenerate. > >>>> > >>>> libcc1/ChangeLog: > >>>> > >>>> * configure: Regenerate. > >>>> > >>>> libcpp/ChangeLog: > >>>> > >>>> * configure: Regenerate. > >>>> > >>>> libdecnumber/ChangeLog: > >>>> > >>>> * configure: Regenerate. > >>> That change looks good to me. Could a global reviewer please comment! > >> Ping > > Ping > > I think this would be a huge mistake to install. The root cause why those false positives show up on S/390 only seems to be of more aggressive inlining w.r.t. other architectures. Because of bigger caches and a rather huge function call overhead we greatly benefit from those inlining parameters. Thus: 1) Reverting those parameters would have a negative performance impact. 2) Fixing the maybe-uninitialized warnings analysis itself seems not to happen in the near future (assuming that it is fixable at all). 3) Silencing the warning by initialising the variable itself also seems to be undesired and feels like a fight against windmills ;-) 4) Not lifting maybe-uninitialized warnings to errors on S/390 only. Option (4) has the least intrusive effect to me. At least then it is not necessary to bootstrap with --disable-werror and we would still treat all other warnings as errors. All maybe-uninitialized warnings which are triggered in common code with non-aggressive inlining are still caught by other architectures. Therefore, I'm wondering why this should be a huge mistake? What would you propose instead? Cheers, Stefan
On 10/28/20 11:29 AM, Stefan Schulze Frielinghaus wrote: > On Wed, Oct 28, 2020 at 08:39:41AM -0600, Jeff Law wrote: >> On 10/28/20 3:38 AM, Stefan Schulze Frielinghaus via Gcc-patches wrote: >>> On Mon, Oct 05, 2020 at 02:02:57PM +0200, Stefan Schulze Frielinghaus via Gcc-patches wrote: >>>> On Tue, Sep 22, 2020 at 02:59:30PM +0200, Andreas Krebbel wrote: >>>>> On 15.09.20 17:02, Stefan Schulze Frielinghaus wrote: >>>>>> Over the last couple of months quite a few warnings about uninitialized >>>>>> variables were raised while building GCC. A reason why these warnings >>>>>> show up on S/390 only is due to the aggressive inlining settings here. >>>>>> Some of these warnings (2c832ffedf0, b776bdca932, 2786c0221b6, >>>>>> 1657178f59b) could be fixed or in case of a false positive silenced by >>>>>> initializing the corresponding variable. Since the latter reoccurs and >>>>>> while bootstrapping such warnings are turned into errors bootstrapping >>>>>> fails on S/390 consistently. Therefore, for the moment do not turn >>>>>> those warnings into errors. >>>>>> >>>>>> config/ChangeLog: >>>>>> >>>>>> * warnings.m4: Do not turn maybe-uninitialized warnings into errors >>>>>> on S/390. >>>>>> >>>>>> fixincludes/ChangeLog: >>>>>> >>>>>> * configure: Regenerate. >>>>>> >>>>>> gcc/ChangeLog: >>>>>> >>>>>> * configure: Regenerate. >>>>>> >>>>>> libcc1/ChangeLog: >>>>>> >>>>>> * configure: Regenerate. >>>>>> >>>>>> libcpp/ChangeLog: >>>>>> >>>>>> * configure: Regenerate. >>>>>> >>>>>> libdecnumber/ChangeLog: >>>>>> >>>>>> * configure: Regenerate. >>>>> That change looks good to me. Could a global reviewer please comment! >>>> Ping >>> Ping >> I think this would be a huge mistake to install. > The root cause why those false positives show up on S/390 only seems to > be of more aggressive inlining w.r.t. other architectures. Because of > bigger caches and a rather huge function call overhead we greatly > benefit from those inlining parameters. Thus: > > 1) Reverting those parameters would have a negative performance impact. > > 2) Fixing the maybe-uninitialized warnings analysis itself seems not to > happen in the near future (assuming that it is fixable at all). > > 3) Silencing the warning by initialising the variable itself also seems > to be undesired and feels like a fight against windmills ;-) > > 4) Not lifting maybe-uninitialized warnings to errors on S/390 only. > > Option (4) has the least intrusive effect to me. At least then it is > not necessary to bootstrap with --disable-werror and we would still > treat all other warnings as errors. All maybe-uninitialized warnings > which are triggered in common code with non-aggressive inlining are > still caught by other architectures. Therefore, I'm wondering why this > should be a huge mistake? What would you propose instead? I'm aware of all that. What I think it all argues is that y'all need to address the issues because of how you've changed the tuning on the s390 port. Simply disabling things like you've suggested is, IMHO, horribly wrong. Improve the analysis, dummy initializers, pragmas all seem viable. But again, it feels like it's something the s390 maintainers will have to take the lead on because of how you've retuned the port. And note that this isn't just an issue with uninitialized warnings, the changes in inlining heuristics can impact all the middle end warnings. jeff
Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > On 10/28/20 11:29 AM, Stefan Schulze Frielinghaus wrote: >> On Wed, Oct 28, 2020 at 08:39:41AM -0600, Jeff Law wrote: >>> On 10/28/20 3:38 AM, Stefan Schulze Frielinghaus via Gcc-patches wrote: >>>> On Mon, Oct 05, 2020 at 02:02:57PM +0200, Stefan Schulze Frielinghaus via Gcc-patches wrote: >>>>> On Tue, Sep 22, 2020 at 02:59:30PM +0200, Andreas Krebbel wrote: >>>>>> On 15.09.20 17:02, Stefan Schulze Frielinghaus wrote: >>>>>>> Over the last couple of months quite a few warnings about uninitialized >>>>>>> variables were raised while building GCC. A reason why these warnings >>>>>>> show up on S/390 only is due to the aggressive inlining settings here. >>>>>>> Some of these warnings (2c832ffedf0, b776bdca932, 2786c0221b6, >>>>>>> 1657178f59b) could be fixed or in case of a false positive silenced by >>>>>>> initializing the corresponding variable. Since the latter reoccurs and >>>>>>> while bootstrapping such warnings are turned into errors bootstrapping >>>>>>> fails on S/390 consistently. Therefore, for the moment do not turn >>>>>>> those warnings into errors. >>>>>>> >>>>>>> config/ChangeLog: >>>>>>> >>>>>>> * warnings.m4: Do not turn maybe-uninitialized warnings into errors >>>>>>> on S/390. >>>>>>> >>>>>>> fixincludes/ChangeLog: >>>>>>> >>>>>>> * configure: Regenerate. >>>>>>> >>>>>>> gcc/ChangeLog: >>>>>>> >>>>>>> * configure: Regenerate. >>>>>>> >>>>>>> libcc1/ChangeLog: >>>>>>> >>>>>>> * configure: Regenerate. >>>>>>> >>>>>>> libcpp/ChangeLog: >>>>>>> >>>>>>> * configure: Regenerate. >>>>>>> >>>>>>> libdecnumber/ChangeLog: >>>>>>> >>>>>>> * configure: Regenerate. >>>>>> That change looks good to me. Could a global reviewer please comment! >>>>> Ping >>>> Ping >>> I think this would be a huge mistake to install. >> The root cause why those false positives show up on S/390 only seems to >> be of more aggressive inlining w.r.t. other architectures. Because of >> bigger caches and a rather huge function call overhead we greatly >> benefit from those inlining parameters. Thus: >> >> 1) Reverting those parameters would have a negative performance impact. >> >> 2) Fixing the maybe-uninitialized warnings analysis itself seems not to >> happen in the near future (assuming that it is fixable at all). >> >> 3) Silencing the warning by initialising the variable itself also seems >> to be undesired and feels like a fight against windmills ;-) >> >> 4) Not lifting maybe-uninitialized warnings to errors on S/390 only. >> >> Option (4) has the least intrusive effect to me. At least then it is >> not necessary to bootstrap with --disable-werror and we would still >> treat all other warnings as errors. All maybe-uninitialized warnings >> which are triggered in common code with non-aggressive inlining are >> still caught by other architectures. Therefore, I'm wondering why this >> should be a huge mistake? What would you propose instead? > > I'm aware of all that. What I think it all argues is that y'all need to > address the issues because of how you've changed the tuning on the s390 > port. Simply disabling things like you've suggested is, IMHO, horribly > wrong. > > > Improve the analysis, dummy initializers, pragmas all seem viable. But > again, it feels like it's something the s390 maintainers will have to > take the lead on because of how you've retuned the port. > > > And note that this isn't just an issue with uninitialized warnings, the > changes in inlining heuristics can impact all the middle end warnings. To play devil's advocate: it seems like a reasonable workaround to me. (I didn't want to approve a potentially controversial patch for “another port” so was staying silent. :-)) Isn't this just the known downside of using maybe-used-uninitialised warnings? AIUI, it's accepted that the option has false positives that vary based on the amount of optimisation that previous passes have or haven't done. So I don't think it's an issue of “fixing” the analysis: the current implementation doesn't seem like it is going to be (and perhaps it isn't meant to be) predictable from a user's perspective. I got the impression this was a deliberate trade-off we'd made in order to let fewer false negatives slip by. We already disable maybe-used-uninitialized warnings when bootstrapping with anything other than the default -O2 -g configuration. (I remember when looking at -Og a while back that there were a large number of unsuppressed false positives when bootstrapping with that.) ISTM that s390 is effectively using non-standard bootstrap options, in a similar way to --with-build-config=, and so turning these errors back into warnings is reasonable here too. Thanks, Richard
On Wed, Oct 28, 2020 at 11:34:53AM -0600, Jeff Law wrote: > > On 10/28/20 11:29 AM, Stefan Schulze Frielinghaus wrote: > > On Wed, Oct 28, 2020 at 08:39:41AM -0600, Jeff Law wrote: > >> On 10/28/20 3:38 AM, Stefan Schulze Frielinghaus via Gcc-patches wrote: > >>> On Mon, Oct 05, 2020 at 02:02:57PM +0200, Stefan Schulze Frielinghaus via Gcc-patches wrote: > >>>> On Tue, Sep 22, 2020 at 02:59:30PM +0200, Andreas Krebbel wrote: > >>>>> On 15.09.20 17:02, Stefan Schulze Frielinghaus wrote: > >>>>>> Over the last couple of months quite a few warnings about uninitialized > >>>>>> variables were raised while building GCC. A reason why these warnings > >>>>>> show up on S/390 only is due to the aggressive inlining settings here. > >>>>>> Some of these warnings (2c832ffedf0, b776bdca932, 2786c0221b6, > >>>>>> 1657178f59b) could be fixed or in case of a false positive silenced by > >>>>>> initializing the corresponding variable. Since the latter reoccurs and > >>>>>> while bootstrapping such warnings are turned into errors bootstrapping > >>>>>> fails on S/390 consistently. Therefore, for the moment do not turn > >>>>>> those warnings into errors. > >>>>>> > >>>>>> config/ChangeLog: > >>>>>> > >>>>>> * warnings.m4: Do not turn maybe-uninitialized warnings into errors > >>>>>> on S/390. > >>>>>> > >>>>>> fixincludes/ChangeLog: > >>>>>> > >>>>>> * configure: Regenerate. > >>>>>> > >>>>>> gcc/ChangeLog: > >>>>>> > >>>>>> * configure: Regenerate. > >>>>>> > >>>>>> libcc1/ChangeLog: > >>>>>> > >>>>>> * configure: Regenerate. > >>>>>> > >>>>>> libcpp/ChangeLog: > >>>>>> > >>>>>> * configure: Regenerate. > >>>>>> > >>>>>> libdecnumber/ChangeLog: > >>>>>> > >>>>>> * configure: Regenerate. > >>>>> That change looks good to me. Could a global reviewer please comment! > >>>> Ping > >>> Ping > >> I think this would be a huge mistake to install. > > The root cause why those false positives show up on S/390 only seems to > > be of more aggressive inlining w.r.t. other architectures. Because of > > bigger caches and a rather huge function call overhead we greatly > > benefit from those inlining parameters. Thus: > > > > 1) Reverting those parameters would have a negative performance impact. > > > > 2) Fixing the maybe-uninitialized warnings analysis itself seems not to > > happen in the near future (assuming that it is fixable at all). > > > > 3) Silencing the warning by initialising the variable itself also seems > > to be undesired and feels like a fight against windmills ;-) > > > > 4) Not lifting maybe-uninitialized warnings to errors on S/390 only. > > > > Option (4) has the least intrusive effect to me. At least then it is > > not necessary to bootstrap with --disable-werror and we would still > > treat all other warnings as errors. All maybe-uninitialized warnings > > which are triggered in common code with non-aggressive inlining are > > still caught by other architectures. Therefore, I'm wondering why this > > should be a huge mistake? What would you propose instead? > > I'm aware of all that. What I think it all argues is that y'all need to > address the issues because of how you've changed the tuning on the s390 > port. Simply disabling things like you've suggested is, IMHO, horribly > wrong. > > > Improve the analysis, dummy initializers, pragmas all seem viable. But > again, it feels like it's something the s390 maintainers will have to > take the lead on because of how you've retuned the port. Fixing the analysis is of course the best option. However, this sounds like a non-trivial task to me and I'm missing a lot of context here, i.e., I'm not sure what the initial goals were and if it is possible to meet those with the requirements which are necessary to solve those false positives (currently having PR96564 in mind where it was mentioned that alias info is not enough but also flow-based info is required; does this imply that we would have to reschedule the analysis at later time which was not desired in the first place etc.). In the past I tried to come up with some dummy initializers which were tough to get accepted (which I can understand up to some degree). For example, this one is still open (I would be happy if you could have a look at it and accept/reject): https://gcc.gnu.org/pipermail/gcc-patches/2020-June/547063.html Then there is at least one unreported case (similar to PR96564) where we are not talking about a variable of scalar type but of an aggregate where only one struct member must be initialized in order to silence the warning. Not sure whether a patch would be accepted where I initialize the whole structure or just a single member. Thus I'm still willing to come up with dummy initializer patches, though, I'm not sure whether they are really accepted by the community or not. > And note that this isn't just an issue with uninitialized warnings, the > changes in inlining heuristics can impact all the middle end warnings. Just curious, is this a hypothetical problem or did we have other problems with those inlining parameters in the past? If there are further concrete problems with those parameters I would be really interested to look into those. Furthermore, I'm still wondering, if those parameters are that controversial whether we should document that. It is tempting to take the documented ranges literally (although admitted we took almost a limit value for param_inline_min_speedup ;-)). Maybe only a certain subrange is meant for production? Anyhow, I did a quick test for param_max_inline_insns_auto which reveals that for values greater than 18 (default is 15) warnings are emitted. I will make a couple of benchmarks in the following days in order to find a parameter set where no false positive is thrown. What I fear most is that we get outperformed by other compilers due to less inlining just because we lifted false positive warnings to errors which feels really bad to me. Cheers, Stefan
On Fri, Oct 30, 2020 at 11:09 AM Stefan Schulze Frielinghaus via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > On Wed, Oct 28, 2020 at 11:34:53AM -0600, Jeff Law wrote: > > > > On 10/28/20 11:29 AM, Stefan Schulze Frielinghaus wrote: > > > On Wed, Oct 28, 2020 at 08:39:41AM -0600, Jeff Law wrote: > > >> On 10/28/20 3:38 AM, Stefan Schulze Frielinghaus via Gcc-patches wrote: > > >>> On Mon, Oct 05, 2020 at 02:02:57PM +0200, Stefan Schulze Frielinghaus via Gcc-patches wrote: > > >>>> On Tue, Sep 22, 2020 at 02:59:30PM +0200, Andreas Krebbel wrote: > > >>>>> On 15.09.20 17:02, Stefan Schulze Frielinghaus wrote: > > >>>>>> Over the last couple of months quite a few warnings about uninitialized > > >>>>>> variables were raised while building GCC. A reason why these warnings > > >>>>>> show up on S/390 only is due to the aggressive inlining settings here. > > >>>>>> Some of these warnings (2c832ffedf0, b776bdca932, 2786c0221b6, > > >>>>>> 1657178f59b) could be fixed or in case of a false positive silenced by > > >>>>>> initializing the corresponding variable. Since the latter reoccurs and > > >>>>>> while bootstrapping such warnings are turned into errors bootstrapping > > >>>>>> fails on S/390 consistently. Therefore, for the moment do not turn > > >>>>>> those warnings into errors. > > >>>>>> > > >>>>>> config/ChangeLog: > > >>>>>> > > >>>>>> * warnings.m4: Do not turn maybe-uninitialized warnings into errors > > >>>>>> on S/390. > > >>>>>> > > >>>>>> fixincludes/ChangeLog: > > >>>>>> > > >>>>>> * configure: Regenerate. > > >>>>>> > > >>>>>> gcc/ChangeLog: > > >>>>>> > > >>>>>> * configure: Regenerate. > > >>>>>> > > >>>>>> libcc1/ChangeLog: > > >>>>>> > > >>>>>> * configure: Regenerate. > > >>>>>> > > >>>>>> libcpp/ChangeLog: > > >>>>>> > > >>>>>> * configure: Regenerate. > > >>>>>> > > >>>>>> libdecnumber/ChangeLog: > > >>>>>> > > >>>>>> * configure: Regenerate. > > >>>>> That change looks good to me. Could a global reviewer please comment! > > >>>> Ping > > >>> Ping > > >> I think this would be a huge mistake to install. > > > The root cause why those false positives show up on S/390 only seems to > > > be of more aggressive inlining w.r.t. other architectures. Because of > > > bigger caches and a rather huge function call overhead we greatly > > > benefit from those inlining parameters. Thus: > > > > > > 1) Reverting those parameters would have a negative performance impact. > > > > > > 2) Fixing the maybe-uninitialized warnings analysis itself seems not to > > > happen in the near future (assuming that it is fixable at all). > > > > > > 3) Silencing the warning by initialising the variable itself also seems > > > to be undesired and feels like a fight against windmills ;-) > > > > > > 4) Not lifting maybe-uninitialized warnings to errors on S/390 only. > > > > > > Option (4) has the least intrusive effect to me. At least then it is > > > not necessary to bootstrap with --disable-werror and we would still > > > treat all other warnings as errors. All maybe-uninitialized warnings > > > which are triggered in common code with non-aggressive inlining are > > > still caught by other architectures. Therefore, I'm wondering why this > > > should be a huge mistake? What would you propose instead? > > > > I'm aware of all that. What I think it all argues is that y'all need to > > address the issues because of how you've changed the tuning on the s390 > > port. Simply disabling things like you've suggested is, IMHO, horribly > > wrong. > > > > > > Improve the analysis, dummy initializers, pragmas all seem viable. But > > again, it feels like it's something the s390 maintainers will have to > > take the lead on because of how you've retuned the port. > > Fixing the analysis is of course the best option. However, this sounds > like a non-trivial task to me and I'm missing a lot of context here, > i.e., I'm not sure what the initial goals were and if it is possible to > meet those with the requirements which are necessary to solve those > false positives (currently having PR96564 in mind where it was mentioned > that alias info is not enough but also flow-based info is required; does > this imply that we would have to reschedule the analysis at later time > which was not desired in the first place etc.). > > In the past I tried to come up with some dummy initializers which were > tough to get accepted (which I can understand up to some degree). For > example, this one is still open (I would be happy if you could have a > look at it and accept/reject): > https://gcc.gnu.org/pipermail/gcc-patches/2020-June/547063.html > > Then there is at least one unreported case (similar to PR96564) where we > are not talking about a variable of scalar type but of an aggregate > where only one struct member must be initialized in order to silence the > warning. Not sure whether a patch would be accepted where I initialize > the whole structure or just a single member. > > Thus I'm still willing to come up with dummy initializer patches, > though, I'm not sure whether they are really accepted by the community > or not. > > > And note that this isn't just an issue with uninitialized warnings, the > > changes in inlining heuristics can impact all the middle end warnings. > > Just curious, is this a hypothetical problem or did we have other > problems with those inlining parameters in the past? If there are > further concrete problems with those parameters I would be really > interested to look into those. > > Furthermore, I'm still wondering, if those parameters are that > controversial whether we should document that. It is tempting to take > the documented ranges literally (although admitted we took almost a > limit value for param_inline_min_speedup ;-)). Maybe only a certain > subrange is meant for production? Anyhow, I did a quick test for > param_max_inline_insns_auto which reveals that for values greater than > 18 (default is 15) warnings are emitted. > > I will make a couple of benchmarks in the following days in order to > find a parameter set where no false positive is thrown. What I fear > most is that we get outperformed by other compilers due to less inlining > just because we lifted false positive warnings to errors which feels > really bad to me. It's not that more / different inlining inherently exposes _more_ false positives in the middle-end warnings. They simply expose others and the GCC codebase is cleansed (by those who change inliner heuristics / tunings) from those by either fixing the analysis or modifying the code (like putting in initializers). The s390 backend is in the exceptional position to change _many_ of the tunables from their default that affect which false positives occur. Besides inliner limits there's also the loop unrolling limits (though those trigger at -O3 only). Whether thats a good or bad tradeoff is up to the port maintainers but I usually urge port maintainers to _not_ change defaults of parameters affecting (relatively) early IL because it makes a testing done on major platforms not be transferable, in terms of coverage, to yours. I also don't believe the telltale that the high inliner limits are necessary to get comparable performance. Note that these were re-tuned only quite some time ago and they are not tuned relative to the current default but tuned to absolute values and thus do not vary when the defaults are re-tuned (which they are for each release). Richard. > Cheers, > Stefan
On 10/30/20 7:01 AM, Richard Biener wrote: > > It's not that more / different inlining inherently exposes _more_ > false positives in the middle-end warnings. They simply expose > others and the GCC codebase is cleansed (by those who change > inliner heuristics / tunings) from those by either fixing the analysis > or modifying the code (like putting in initializers). Right. The change in heuristics inherently perturb the middle end warnings. It has been and continues to be a source of significant headaches in Fedora. > Whether thats a good or bad tradeoff is up to the port maintainers > but I usually urge port maintainers to _not_ change defaults > of parameters affecting (relatively) early IL because it makes > a testing done on major platforms not be transferable, in terms > of coverage, to yours. I'd advise against it as well. > > I also don't believe the telltale that the high inliner limits are > necessary to get comparable performance. Note that these > were re-tuned only quite some time ago and they are not > tuned relative to the current default but tuned to absolute values > and thus do not vary when the defaults are re-tuned (which they > are for each release). Precisely. Honza re-tunes the parameters for nearly every release so that they reflect the various tradeoffs of our optimizer/analysis implementation for each release. When backends override, they don't get any of that benefit and ultimately it makes more work for others at the distro level. Jeff
On 06.11.20 04:52, Jeff Law via Gcc-patches wrote: > > On 10/30/20 7:01 AM, Richard Biener wrote: >> >> It's not that more / different inlining inherently exposes _more_ >> false positives in the middle-end warnings. They simply expose >> others and the GCC codebase is cleansed (by those who change >> inliner heuristics / tunings) from those by either fixing the analysis >> or modifying the code (like putting in initializers). > > Right. The change in heuristics inherently perturb the middle end > warnings. It has been and continues to be a source of significant > headaches in Fedora. Stefan did some measurements and in fact we see only a few benchmarks improving with our aggressive settings. However, in these cases the performance benefits are significant. We will continue looking into these cases. Perhaps more selective ways can be found to achieve the same. I've just committed a patch to switch back to the default values. With that patch bootstrapping on Z works fine again even without --disable-werror. Andreas gcc/ChangeLog: * config/s390/s390.c (s390_option_override_internal): Remove override of inline params. --- gcc/config/s390/s390.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c index b8961a315aa..847cedde674 100644 --- a/gcc/config/s390/s390.c +++ b/gcc/config/s390/s390.c @@ -15469,13 +15469,6 @@ s390_option_override_internal (struct gcc_options *opts, SET_OPTION_IF_UNSET (opts, opts_set, param_sched_pressure_algorithm, 2); SET_OPTION_IF_UNSET (opts, opts_set, param_min_vect_loop_bound, 2); - /* Use aggressive inlining parameters. */ - if (opts->x_s390_tune >= PROCESSOR_2964_Z13) - { - SET_OPTION_IF_UNSET (opts, opts_set, param_inline_min_speedup, 2); - SET_OPTION_IF_UNSET (opts, opts_set, param_max_inline_insns_auto, 80); - } - /* Set the default alignment. */ s390_default_align (opts);
On 10/30/20 4:08 AM, Stefan Schulze Frielinghaus wrote: > On Wed, Oct 28, 2020 at 11:34:53AM -0600, Jeff Law wrote: >> On 10/28/20 11:29 AM, Stefan Schulze Frielinghaus wrote: >>> On Wed, Oct 28, 2020 at 08:39:41AM -0600, Jeff Law wrote: >>>> On 10/28/20 3:38 AM, Stefan Schulze Frielinghaus via Gcc-patches wrote: >>>>> On Mon, Oct 05, 2020 at 02:02:57PM +0200, Stefan Schulze Frielinghaus via Gcc-patches wrote: >>>>>> On Tue, Sep 22, 2020 at 02:59:30PM +0200, Andreas Krebbel wrote: >>>>>>> On 15.09.20 17:02, Stefan Schulze Frielinghaus wrote: >>>>>>>> Over the last couple of months quite a few warnings about uninitialized >>>>>>>> variables were raised while building GCC. A reason why these warnings >>>>>>>> show up on S/390 only is due to the aggressive inlining settings here. >>>>>>>> Some of these warnings (2c832ffedf0, b776bdca932, 2786c0221b6, >>>>>>>> 1657178f59b) could be fixed or in case of a false positive silenced by >>>>>>>> initializing the corresponding variable. Since the latter reoccurs and >>>>>>>> while bootstrapping such warnings are turned into errors bootstrapping >>>>>>>> fails on S/390 consistently. Therefore, for the moment do not turn >>>>>>>> those warnings into errors. >>>>>>>> >>>>>>>> config/ChangeLog: >>>>>>>> >>>>>>>> * warnings.m4: Do not turn maybe-uninitialized warnings into errors >>>>>>>> on S/390. >>>>>>>> >>>>>>>> fixincludes/ChangeLog: >>>>>>>> >>>>>>>> * configure: Regenerate. >>>>>>>> >>>>>>>> gcc/ChangeLog: >>>>>>>> >>>>>>>> * configure: Regenerate. >>>>>>>> >>>>>>>> libcc1/ChangeLog: >>>>>>>> >>>>>>>> * configure: Regenerate. >>>>>>>> >>>>>>>> libcpp/ChangeLog: >>>>>>>> >>>>>>>> * configure: Regenerate. >>>>>>>> >>>>>>>> libdecnumber/ChangeLog: >>>>>>>> >>>>>>>> * configure: Regenerate. >>>>>>> That change looks good to me. Could a global reviewer please comment! >>>>>> Ping >>>>> Ping >>>> I think this would be a huge mistake to install. >>> The root cause why those false positives show up on S/390 only seems to >>> be of more aggressive inlining w.r.t. other architectures. Because of >>> bigger caches and a rather huge function call overhead we greatly >>> benefit from those inlining parameters. Thus: >>> >>> 1) Reverting those parameters would have a negative performance impact. >>> >>> 2) Fixing the maybe-uninitialized warnings analysis itself seems not to >>> happen in the near future (assuming that it is fixable at all). >>> >>> 3) Silencing the warning by initialising the variable itself also seems >>> to be undesired and feels like a fight against windmills ;-) >>> >>> 4) Not lifting maybe-uninitialized warnings to errors on S/390 only. >>> >>> Option (4) has the least intrusive effect to me. At least then it is >>> not necessary to bootstrap with --disable-werror and we would still >>> treat all other warnings as errors. All maybe-uninitialized warnings >>> which are triggered in common code with non-aggressive inlining are >>> still caught by other architectures. Therefore, I'm wondering why this >>> should be a huge mistake? What would you propose instead? >> I'm aware of all that. What I think it all argues is that y'all need to >> address the issues because of how you've changed the tuning on the s390 >> port. Simply disabling things like you've suggested is, IMHO, horribly >> wrong. >> >> >> Improve the analysis, dummy initializers, pragmas all seem viable. But >> again, it feels like it's something the s390 maintainers will have to >> take the lead on because of how you've retuned the port. > Fixing the analysis is of course the best option. However, this sounds > like a non-trivial task to me and I'm missing a lot of context here, > i.e., I'm not sure what the initial goals were and if it is possible to > meet those with the requirements which are necessary to solve those > false positives (currently having PR96564 in mind where it was mentioned > that alias info is not enough but also flow-based info is required; does > this imply that we would have to reschedule the analysis at later time > which was not desired in the first place etc.). There are going to be cases we can't solve with just improvements in the analysis. My point is that we have several tools in our toolbox and we should be looking at those to solve the problem rather than just disabling the warning. > > In the past I tried to come up with some dummy initializers which were > tough to get accepted (which I can understand up to some degree). For > example, this one is still open (I would be happy if you could have a > look at it and accept/reject): > https://gcc.gnu.org/pipermail/gcc-patches/2020-June/547063.html > > Then there is at least one unreported case (similar to PR96564) where we > are not talking about a variable of scalar type but of an aggregate > where only one struct member must be initialized in order to silence the > warning. Not sure whether a patch would be accepted where I initialize > the whole structure or just a single member. > > Thus I'm still willing to come up with dummy initializer patches, > though, I'm not sure whether they are really accepted by the community > or not. They can be after analysis of the events that lead to the diagnostic. Please make sure those are attached to the -Wuninitialized meta-bug. I don't know if I'm going to have much time to look at them this stage3/stage4, but that meta bug is where I start. > >> And note that this isn't just an issue with uninitialized warnings, the >> changes in inlining heuristics can impact all the middle end warnings. > Just curious, is this a hypothetical problem or did we have other > problems with those inlining parameters in the past? If there are > further concrete problems with those parameters I would be really > interested to look into those. We've bumped up against it regularly in Fedora. Essentially any middle end warning is potentially perturbed by changes in the inlining heuristics. Sometimes the extra inlining gives enough context to make a false positive go away other times it introduces false positives. Sometimes the extra inlining allows us to catch an case that was previously missed (which is a good thing obviously). I would probably go far enough to say that these are the most common source of FTBFS issues in Fedora when we drop in new compilers -- s390 builds failing because they emitted a false positive diagnostic that no other target has (due to the differences in inlining) and the package is using -Werror. > Furthermore, I'm still wondering, if those parameters are that > controversial whether we should document that. It is tempting to take > the documented ranges literally (although admitted we took almost a > limit value for param_inline_min_speedup ;-)). Maybe only a certain > subrange is meant for production? Anyhow, I did a quick test for > param_max_inline_insns_auto which reveals that for values greater than > 18 (default is 15) warnings are emitted. > > I will make a couple of benchmarks in the following days in order to > find a parameter set where no false positive is thrown. What I fear > most is that we get outperformed by other compilers due to less inlining > just because we lifted false positive warnings to errors which feels > really bad to me. There's so many things that go into these decisions. What I keep coming back to is the design principle that, to the extent possible, gimple should be reasonably comparable across different targets. That gives predictability for the analysis and optimizers which solves one of the *huge* problems with RTL. While we never 100% achieved that, it's a good guiding principle. Jeff
diff --git a/config/warnings.m4 b/config/warnings.m4 index ce007f9b73e..d977bfb20af 100644 --- a/config/warnings.m4 +++ b/config/warnings.m4 @@ -101,8 +101,10 @@ AC_ARG_ENABLE(werror-always, AS_HELP_STRING([--enable-werror-always], [enable -Werror despite compiler version]), [], [enable_werror_always=no]) -AS_IF([test $enable_werror_always = yes], - [acx_Var="$acx_Var${acx_Var:+ }-Werror"]) +AS_IF([test $enable_werror_always = yes], [dnl + acx_Var="$acx_Var${acx_Var:+ }-Werror" + AS_CASE([$host], [s390*-*-*], + [acx_Var="$acx_Var -Wno-error=maybe-uninitialized"])]) m4_if($1, [manual],, [AS_VAR_PUSHDEF([acx_GCCvers], [acx_cv_prog_cc_gcc_$1_or_newer])dnl AC_CACHE_CHECK([whether $CC is GCC >=$1], acx_GCCvers, @@ -116,7 +118,9 @@ AS_IF([test $enable_werror_always = yes], [AS_VAR_SET(acx_GCCvers, yes)], [AS_VAR_SET(acx_GCCvers, no)])]) AS_IF([test AS_VAR_GET(acx_GCCvers) = yes], - [acx_Var="$acx_Var${acx_Var:+ }-Werror"]) + [acx_Var="$acx_Var${acx_Var:+ }-Werror" + AS_CASE([$host], [s390*-*-*], + [acx_Var="$acx_Var -Wno-error=maybe-uninitialized"])]) AS_VAR_POPDEF([acx_GCCvers])]) m4_popdef([acx_Var])dnl AC_LANG_POP(C) @@ -205,8 +209,10 @@ AC_ARG_ENABLE(werror-always, AS_HELP_STRING([--enable-werror-always], [enable -Werror despite compiler version]), [], [enable_werror_always=no]) -AS_IF([test $enable_werror_always = yes], - [acx_Var="$acx_Var${acx_Var:+ }-Werror"]) +AS_IF([test $enable_werror_always = yes], [dnl + acx_Var="$acx_Var${acx_Var:+ }-Werror" + AS_CASE([$host], [s390*-*-*], + [strict_warn="$strict_warn -Wno-error=maybe-uninitialized"])]) m4_if($1, [manual],, [AS_VAR_PUSHDEF([acx_GXXvers], [acx_cv_prog_cxx_gxx_$1_or_newer])dnl AC_CACHE_CHECK([whether $CXX is G++ >=$1], acx_GXXvers, @@ -220,7 +226,9 @@ AS_IF([test $enable_werror_always = yes], [AS_VAR_SET(acx_GXXvers, yes)], [AS_VAR_SET(acx_GXXvers, no)])]) AS_IF([test AS_VAR_GET(acx_GXXvers) = yes], - [acx_Var="$acx_Var${acx_Var:+ }-Werror"]) + [acx_Var="$acx_Var${acx_Var:+ }-Werror" + AS_CASE([$host], [s390*-*-*], + [acx_Var="$acx_Var -Wno-error=maybe-uninitialized"])]) AS_VAR_POPDEF([acx_GXXvers])]) m4_popdef([acx_Var])dnl AC_LANG_POP(C++) diff --git a/fixincludes/configure b/fixincludes/configure index 6e2d67b655b..e0d679cc18e 100755 --- a/fixincludes/configure +++ b/fixincludes/configure @@ -4753,7 +4753,13 @@ else fi if test $enable_werror_always = yes; then : - WERROR="$WERROR${WERROR:+ }-Werror" + WERROR="$WERROR${WERROR:+ }-Werror" + case $host in #( + s390*-*-*) : + WERROR="$WERROR -Wno-error=maybe-uninitialized" ;; #( + *) : + ;; +esac fi ac_ext=c diff --git a/gcc/configure b/gcc/configure index 0a09777dd42..ea03581537a 100755 --- a/gcc/configure +++ b/gcc/configure @@ -7064,7 +7064,13 @@ else fi if test $enable_werror_always = yes; then : - strict_warn="$strict_warn${strict_warn:+ }-Werror" + strict_warn="$strict_warn${strict_warn:+ }-Werror" + case $host in #( + s390*-*-*) : + strict_warn="$strict_warn -Wno-error=maybe-uninitialized" ;; #( + *) : + ;; +esac fi ac_ext=cpp @@ -19013,7 +19019,7 @@ else lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 lt_status=$lt_dlunknown cat > conftest.$ac_ext <<_LT_EOF -#line 19016 "configure" +#line 19022 "configure" #include "confdefs.h" #if HAVE_DLFCN_H @@ -19119,7 +19125,7 @@ else lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 lt_status=$lt_dlunknown cat > conftest.$ac_ext <<_LT_EOF -#line 19122 "configure" +#line 19128 "configure" #include "confdefs.h" #if HAVE_DLFCN_H diff --git a/libcc1/configure b/libcc1/configure index 3610219ba65..e95274d0d46 100755 --- a/libcc1/configure +++ b/libcc1/configure @@ -14835,7 +14835,13 @@ else fi if test $enable_werror_always = yes; then : - WERROR="$WERROR${WERROR:+ }-Werror" + WERROR="$WERROR${WERROR:+ }-Werror" + case $host in #( + s390*-*-*) : + WERROR="$WERROR -Wno-error=maybe-uninitialized" ;; #( + *) : + ;; +esac fi ac_ext=c diff --git a/libcpp/configure b/libcpp/configure index 7e28606f605..db378c363ee 100755 --- a/libcpp/configure +++ b/libcpp/configure @@ -5142,7 +5142,13 @@ else fi if test $enable_werror_always = yes; then : - WERROR="$WERROR${WERROR:+ }-Werror" + WERROR="$WERROR${WERROR:+ }-Werror" + case $host in #( + s390*-*-*) : + WERROR="$WERROR -Wno-error=maybe-uninitialized" ;; #( + *) : + ;; +esac fi ac_ext=c diff --git a/libdecnumber/configure b/libdecnumber/configure index 6b62f8ba665..7035aef58dd 100755 --- a/libdecnumber/configure +++ b/libdecnumber/configure @@ -3661,7 +3661,13 @@ else fi if test $enable_werror_always = yes; then : - WERROR="$WERROR${WERROR:+ }-Werror" + WERROR="$WERROR${WERROR:+ }-Werror" + case $host in #( + s390*-*-*) : + WERROR="$WERROR -Wno-error=maybe-uninitialized" ;; #( + *) : + ;; +esac fi ac_ext=c