Message ID | 201707192014.v6JKE6JV001711@sellcey-dt.caveonetworks.com |
---|---|
State | New |
Headers | show |
On Wed, Jul 19, 2017 at 3:14 PM, Steve Ellcey <sellcey@cavium.com> wrote: > While testing the latest glibc sources with the latest (ToT) GCC, I ran > into some build failures (during testing, not during the glibc build). > This patch fixes the build failures in the localedata subdirectory. I > believe these started failing with this GCC patch: > > https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00537.html > > It does not look like older GCC's complain about not understanding this > option so there does not seem to be any need to conditionalize its usage. I can't conveniently experiment with GCC 8.x right now, but this seems like the right general idea. zw
On Wed, 2017-07-19 at 15:20 -0500, Zack Weinberg wrote: > On Wed, Jul 19, 2017 at 3:14 PM, Steve Ellcey <sellcey@cavium.com> > wrote: > > > > While testing the latest glibc sources with the latest (ToT) GCC, I ran > > into some build failures (during testing, not during the glibc build). > > This patch fixes the build failures in the localedata subdirectory. I > > believe these started failing with this GCC patch: > > > > https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00537.html > > > > It does not look like older GCC's complain about not understanding this > > option so there does not seem to be any need to conditionalize its usage. > I can't conveniently experiment with GCC 8.x right now, but this seems > like the right general idea. > > zw Does anyone else have any comments, pro or con, on this patch? I would like to check it in today for 2.26 if there are no objections, but it is certainly not a showstopper if there are objections. Steve Ellcey sellcey@cavium.com
On 07/19/2017 04:14 PM, Steve Ellcey wrote: > While testing the latest glibc sources with the latest (ToT) GCC, I ran > into some build failures (during testing, not during the glibc build). > This patch fixes the build failures in the localedata subdirectory. I > believe these started failing with this GCC patch: > > https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00537.html > > It does not look like older GCC's complain about not understanding this > option so there does not seem to be any need to conditionalize its usage. > > Ok to checkin? OK. This looks good to me. The alternative of using the DIAG macros is not feasibly nor easy, so I'm OK to apply the error removal in a blanket fashion like this for each of the files. If you filed an upstream gcc PR please include the PR in the comment. > Steve Ellcey > sellcey@cavium.com > > > 2017-07-19 Steve Ellcey <sellcey@cavium.com> > > * localedata/Makefile (CFLAGS-tst_iswalnum.c, CFLAGS-tst_iswalpha.c > CFLAGS-tst_iswcntrl.c, CFLAGS-tst_iswdigit.c, CFLAGS-tst_iswgraph.c, > CFLAGS-tst_iswlower.c, CFLAGS-tst_iswprint.c, CFLAGS-tst_iswpunct.c, > CFLAGS-tst_iswspace.c, CFLAGS-tst_iswupper.c, CFLAGS-tst_iswxdigit.c, > CFLAGS-tst_towlower.c, CFLAGS-tst_towupper.c): New macros. > > > diff --git a/localedata/Makefile b/localedata/Makefile > index 47ca39d..20c5921 100644 > --- a/localedata/Makefile > +++ b/localedata/Makefile > @@ -122,6 +122,21 @@ $(inst_i18ndir)/charmaps/%.gz: charmaps/% $(+force) > # Install the locale source files in the appropriate directory. > $(inst_i18ndir)/locales/%: locales/% $(+force); $(do-install) > > +# These tests use multistatement macros from tests-mbwc/tst_funcs.h > +# and will not compile with GCC 8.1 without the warning turned off. > +CFLAGS-tst_iswalnum.c = -Wno-multistatement-macros > +CFLAGS-tst_iswalpha.c = -Wno-multistatement-macros > +CFLAGS-tst_iswcntrl.c = -Wno-multistatement-macros > +CFLAGS-tst_iswdigit.c = -Wno-multistatement-macros > +CFLAGS-tst_iswgraph.c = -Wno-multistatement-macros > +CFLAGS-tst_iswlower.c = -Wno-multistatement-macros > +CFLAGS-tst_iswprint.c = -Wno-multistatement-macros > +CFLAGS-tst_iswpunct.c = -Wno-multistatement-macros > +CFLAGS-tst_iswspace.c = -Wno-multistatement-macros > +CFLAGS-tst_iswupper.c = -Wno-multistatement-macros > +CFLAGS-tst_iswxdigit.c = -Wno-multistatement-macros > +CFLAGS-tst_towlower.c = -Wno-multistatement-macros > +CFLAGS-tst_towupper.c = -Wno-multistatement-macros > > ifeq ($(run-built-tests),yes) > generated-dirs += $(LOCALES) >
On Wed, 19 Jul 2017, Steve Ellcey wrote: > While testing the latest glibc sources with the latest (ToT) GCC, I ran > into some build failures (during testing, not during the glibc build). > This patch fixes the build failures in the localedata subdirectory. I > believe these started failing with this GCC patch: > > https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00537.html The warnings you quoted look very much like false positives to me, i.e. something that should be fixed in GCC not worked around in glibc. Specifically, they look quite like the issue described in <https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01827.html>. Does that patch avoid the need for these -Wno- options? If so, that indicates these -Wno- options should be removed. If not, that indicates engaging with that thread would be appropriate to explain how the glibc case differs (unless there are cases in glibc that can be shown *not* to be false positives, i.e. where the code suggests a macro expansion is entirely within an "if" etc. but only part of it is). > +# These tests use multistatement macros from tests-mbwc/tst_funcs.h > +# and will not compile with GCC 8.1 without the warning turned off. A comment referencing GCC 8.1 can't possibly be correct here yet; that's making a prediction about a version of GCC from April 2018, and so is misleading by suggesting something applies to a future release that may or may not apply to that release. All GCC 8 development versions at present are 8.0.
diff --git a/localedata/Makefile b/localedata/Makefile index 47ca39d..20c5921 100644 --- a/localedata/Makefile +++ b/localedata/Makefile @@ -122,6 +122,21 @@ $(inst_i18ndir)/charmaps/%.gz: charmaps/% $(+force) # Install the locale source files in the appropriate directory. $(inst_i18ndir)/locales/%: locales/% $(+force); $(do-install) +# These tests use multistatement macros from tests-mbwc/tst_funcs.h +# and will not compile with GCC 8.1 without the warning turned off. +CFLAGS-tst_iswalnum.c = -Wno-multistatement-macros +CFLAGS-tst_iswalpha.c = -Wno-multistatement-macros +CFLAGS-tst_iswcntrl.c = -Wno-multistatement-macros +CFLAGS-tst_iswdigit.c = -Wno-multistatement-macros +CFLAGS-tst_iswgraph.c = -Wno-multistatement-macros +CFLAGS-tst_iswlower.c = -Wno-multistatement-macros +CFLAGS-tst_iswprint.c = -Wno-multistatement-macros +CFLAGS-tst_iswpunct.c = -Wno-multistatement-macros +CFLAGS-tst_iswspace.c = -Wno-multistatement-macros +CFLAGS-tst_iswupper.c = -Wno-multistatement-macros +CFLAGS-tst_iswxdigit.c = -Wno-multistatement-macros +CFLAGS-tst_towlower.c = -Wno-multistatement-macros +CFLAGS-tst_towupper.c = -Wno-multistatement-macros ifeq ($(run-built-tests),yes) generated-dirs += $(LOCALES)