| Message ID | 20241003194450.1052220-4-christophe.lyon@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | aarch64: Clean warnings in libgcc | expand |
> On 3 Oct 2024, at 21:44, Christophe Lyon <christophe.lyon@linaro.org> wrote: > > External email: Use caution opening links or attachments > > > When --enable-werror is enabled when running the top-level configure, > it passes --enable-werror-always to subdirs. Some of them, like > libgcc, ignore it. > > This patch adds support for it, enabled only for aarch64, to avoid > breaking bootstrap for other targets. > The aarch64 part is ok but you’ll need a wider libgcc approval. It seems to me that if libgcc is intended to compile cleanly with -Werror then it should be a libgcc-wide change, but maybe doing it port-by-port is the only practical way of getting there? Thanks, Kyrill > The patch also adds -Wno-prio-ctor-dtor to avoid a warning when compiling lse_init.c > > libgcc/ > * Makefile.in (WERROR): New. > * config/aarch64/t-aarch64: Handle WERROR. Always use > -Wno-prio-ctor-dtor. > * configure.ac: Add support for --enable-werror-always. > * configure: Regenerate. > --- > libgcc/Makefile.in | 1 + > libgcc/config/aarch64/t-aarch64 | 1 + > libgcc/configure | 31 +++++++++++++++++++++++++++++++ > libgcc/configure.ac | 5 +++++ > 4 files changed, 38 insertions(+) > > diff --git a/libgcc/Makefile.in b/libgcc/Makefile.in > index 0e46e9ef768..eca62546642 100644 > --- a/libgcc/Makefile.in > +++ b/libgcc/Makefile.in > @@ -84,6 +84,7 @@ AR_FLAGS = rc > > CC = @CC@ > CFLAGS = @CFLAGS@ > +WERROR = @WERROR@ > RANLIB = @RANLIB@ > LN_S = @LN_S@ > > diff --git a/libgcc/config/aarch64/t-aarch64 b/libgcc/config/aarch64/t-aarch64 > index b70e7b94edd..ae1588ce307 100644 > --- a/libgcc/config/aarch64/t-aarch64 > +++ b/libgcc/config/aarch64/t-aarch64 > @@ -30,3 +30,4 @@ LIB2ADDEH += \ > $(srcdir)/config/aarch64/__arm_za_disable.S > > SHLIB_MAPFILES += $(srcdir)/config/aarch64/libgcc-sme.ver > +LIBGCC2_CFLAGS += $(WERROR) -Wno-prio-ctor-dtor > diff --git a/libgcc/configure b/libgcc/configure > index cff1eff9625..ae56f7dbdc9 100755 > --- a/libgcc/configure > +++ b/libgcc/configure > @@ -592,6 +592,7 @@ enable_execute_stack > asm_hidden_op > extra_parts > cpu_type > +WERROR > get_gcc_base_ver > HAVE_STRUB_SUPPORT > thread_header > @@ -719,6 +720,7 @@ enable_tm_clone_registry > with_glibc_version > enable_tls > with_gcc_major_version_only > +enable_werror_always > ' > ac_precious_vars='build_alias > host_alias > @@ -1361,6 +1363,7 @@ Optional Features: > installations without PT_GNU_EH_FRAME support > --disable-tm-clone-registry disable TM clone registry > --enable-tls Use thread-local storage [default=yes] > + --enable-werror-always enable -Werror despite compiler version > > Optional Packages: > --with-PACKAGE[=ARG] use PACKAGE [ARG=yes] > @@ -5808,6 +5811,34 @@ fi > > > > +# Only enable with --enable-werror-always until existing warnings are > +# corrected. > +ac_ext=c > +ac_cpp='$CPP $CPPFLAGS' > +ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5' > +ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5' > +ac_compiler_gnu=$ac_cv_c_compiler_gnu > + > +WERROR= > +# Check whether --enable-werror-always was given. > +if test "${enable_werror_always+set}" = set; then : > + enableval=$enable_werror_always; > +else > + enable_werror_always=no > +fi > + > +if test $enable_werror_always = yes; then : > + WERROR="$WERROR${WERROR:+ }-Werror" > +fi > + > +ac_ext=c > +ac_cpp='$CPP $CPPFLAGS' > +ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5' > +ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5' > +ac_compiler_gnu=$ac_cv_c_compiler_gnu > + > + > + > # Substitute configuration variables > > > diff --git a/libgcc/configure.ac b/libgcc/configure.ac > index 4e8c036990f..6b3ea2aea5c 100644 > --- a/libgcc/configure.ac > +++ b/libgcc/configure.ac > @@ -13,6 +13,7 @@ sinclude(../config/unwind_ipinfo.m4) > sinclude(../config/gthr.m4) > sinclude(../config/sjlj.m4) > sinclude(../config/cet.m4) > +sinclude(../config/warnings.m4) > > AC_INIT([GNU C Runtime Library], 1.0,,[libgcc]) > AC_CONFIG_SRCDIR([static-object.mk]) > @@ -746,6 +747,10 @@ AC_SUBST(HAVE_STRUB_SUPPORT) > # Determine what GCC version number to use in filesystem paths. > GCC_BASE_VER > > +# Only enable with --enable-werror-always until existing warnings are > +# corrected. > +ACX_PROG_CC_WARNINGS_ARE_ERRORS([manual]) > + > # Substitute configuration variables > AC_SUBST(cpu_type) > AC_SUBST(extra_parts) > -- > 2.34.1 >
On Fri, 4 Oct 2024 at 10:50, Kyrylo Tkachov <ktkachov@nvidia.com> wrote: > > > > > On 3 Oct 2024, at 21:44, Christophe Lyon <christophe.lyon@linaro.org> wrote: > > > > External email: Use caution opening links or attachments > > > > > > When --enable-werror is enabled when running the top-level configure, > > it passes --enable-werror-always to subdirs. Some of them, like > > libgcc, ignore it. > > > > This patch adds support for it, enabled only for aarch64, to avoid > > breaking bootstrap for other targets. > > > > The aarch64 part is ok but you’ll need a wider libgcc approval. > It seems to me that if libgcc is intended to compile cleanly with -Werror then it should be a libgcc-wide change, but maybe doing it port-by-port is the only practical way of getting there? Indeed, it was not clear to me if libgcc is supposed to compile without warnings.... My feeling is that warnings or often worth having a look, but without -Werror they get unnoticed. Adding Ian in cc as libgcc maintainer. Thanks, Christophe > Thanks, > Kyrill > > > > The patch also adds -Wno-prio-ctor-dtor to avoid a warning when compiling lse_init.c > > > > libgcc/ > > * Makefile.in (WERROR): New. > > * config/aarch64/t-aarch64: Handle WERROR. Always use > > -Wno-prio-ctor-dtor. > > * configure.ac: Add support for --enable-werror-always. > > * configure: Regenerate. > > --- > > libgcc/Makefile.in | 1 + > > libgcc/config/aarch64/t-aarch64 | 1 + > > libgcc/configure | 31 +++++++++++++++++++++++++++++++ > > libgcc/configure.ac | 5 +++++ > > 4 files changed, 38 insertions(+) > > > > diff --git a/libgcc/Makefile.in b/libgcc/Makefile.in > > index 0e46e9ef768..eca62546642 100644 > > --- a/libgcc/Makefile.in > > +++ b/libgcc/Makefile.in > > @@ -84,6 +84,7 @@ AR_FLAGS = rc > > > > CC = @CC@ > > CFLAGS = @CFLAGS@ > > +WERROR = @WERROR@ > > RANLIB = @RANLIB@ > > LN_S = @LN_S@ > > > > diff --git a/libgcc/config/aarch64/t-aarch64 b/libgcc/config/aarch64/t-aarch64 > > index b70e7b94edd..ae1588ce307 100644 > > --- a/libgcc/config/aarch64/t-aarch64 > > +++ b/libgcc/config/aarch64/t-aarch64 > > @@ -30,3 +30,4 @@ LIB2ADDEH += \ > > $(srcdir)/config/aarch64/__arm_za_disable.S > > > > SHLIB_MAPFILES += $(srcdir)/config/aarch64/libgcc-sme.ver > > +LIBGCC2_CFLAGS += $(WERROR) -Wno-prio-ctor-dtor > > diff --git a/libgcc/configure b/libgcc/configure > > index cff1eff9625..ae56f7dbdc9 100755 > > --- a/libgcc/configure > > +++ b/libgcc/configure > > @@ -592,6 +592,7 @@ enable_execute_stack > > asm_hidden_op > > extra_parts > > cpu_type > > +WERROR > > get_gcc_base_ver > > HAVE_STRUB_SUPPORT > > thread_header > > @@ -719,6 +720,7 @@ enable_tm_clone_registry > > with_glibc_version > > enable_tls > > with_gcc_major_version_only > > +enable_werror_always > > ' > > ac_precious_vars='build_alias > > host_alias > > @@ -1361,6 +1363,7 @@ Optional Features: > > installations without PT_GNU_EH_FRAME support > > --disable-tm-clone-registry disable TM clone registry > > --enable-tls Use thread-local storage [default=yes] > > + --enable-werror-always enable -Werror despite compiler version > > > > Optional Packages: > > --with-PACKAGE[=ARG] use PACKAGE [ARG=yes] > > @@ -5808,6 +5811,34 @@ fi > > > > > > > > +# Only enable with --enable-werror-always until existing warnings are > > +# corrected. > > +ac_ext=c > > +ac_cpp='$CPP $CPPFLAGS' > > +ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5' > > +ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5' > > +ac_compiler_gnu=$ac_cv_c_compiler_gnu > > + > > +WERROR= > > +# Check whether --enable-werror-always was given. > > +if test "${enable_werror_always+set}" = set; then : > > + enableval=$enable_werror_always; > > +else > > + enable_werror_always=no > > +fi > > + > > +if test $enable_werror_always = yes; then : > > + WERROR="$WERROR${WERROR:+ }-Werror" > > +fi > > + > > +ac_ext=c > > +ac_cpp='$CPP $CPPFLAGS' > > +ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5' > > +ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5' > > +ac_compiler_gnu=$ac_cv_c_compiler_gnu > > + > > + > > + > > # Substitute configuration variables > > > > > > diff --git a/libgcc/configure.ac b/libgcc/configure.ac > > index 4e8c036990f..6b3ea2aea5c 100644 > > --- a/libgcc/configure.ac > > +++ b/libgcc/configure.ac > > @@ -13,6 +13,7 @@ sinclude(../config/unwind_ipinfo.m4) > > sinclude(../config/gthr.m4) > > sinclude(../config/sjlj.m4) > > sinclude(../config/cet.m4) > > +sinclude(../config/warnings.m4) > > > > AC_INIT([GNU C Runtime Library], 1.0,,[libgcc]) > > AC_CONFIG_SRCDIR([static-object.mk]) > > @@ -746,6 +747,10 @@ AC_SUBST(HAVE_STRUB_SUPPORT) > > # Determine what GCC version number to use in filesystem paths. > > GCC_BASE_VER > > > > +# Only enable with --enable-werror-always until existing warnings are > > +# corrected. > > +ACX_PROG_CC_WARNINGS_ARE_ERRORS([manual]) > > + > > # Substitute configuration variables > > AC_SUBST(cpu_type) > > AC_SUBST(extra_parts) > > -- > > 2.34.1 > > >
Christophe Lyon <christophe.lyon@linaro.org> writes: > When --enable-werror is enabled when running the top-level configure, > it passes --enable-werror-always to subdirs. Some of them, like > libgcc, ignore it. > > This patch adds support for it, enabled only for aarch64, to avoid > breaking bootstrap for other targets. > > The patch also adds -Wno-prio-ctor-dtor to avoid a warning when compiling lse_init.c > > libgcc/ > * Makefile.in (WERROR): New. > * config/aarch64/t-aarch64: Handle WERROR. Always use > -Wno-prio-ctor-dtor. > * configure.ac: Add support for --enable-werror-always. > * configure: Regenerate. > --- > libgcc/Makefile.in | 1 + > libgcc/config/aarch64/t-aarch64 | 1 + > libgcc/configure | 31 +++++++++++++++++++++++++++++++ > libgcc/configure.ac | 5 +++++ > 4 files changed, 38 insertions(+) > > [...] > diff --git a/libgcc/configure.ac b/libgcc/configure.ac > index 4e8c036990f..6b3ea2aea5c 100644 > --- a/libgcc/configure.ac > +++ b/libgcc/configure.ac > @@ -13,6 +13,7 @@ sinclude(../config/unwind_ipinfo.m4) > sinclude(../config/gthr.m4) > sinclude(../config/sjlj.m4) > sinclude(../config/cet.m4) > +sinclude(../config/warnings.m4) > > AC_INIT([GNU C Runtime Library], 1.0,,[libgcc]) > AC_CONFIG_SRCDIR([static-object.mk]) > @@ -746,6 +747,10 @@ AC_SUBST(HAVE_STRUB_SUPPORT) > # Determine what GCC version number to use in filesystem paths. > GCC_BASE_VER > > +# Only enable with --enable-werror-always until existing warnings are > +# corrected. > +ACX_PROG_CC_WARNINGS_ARE_ERRORS([manual]) It looks like this is borrowed from libcpp and/or libdecnumber. Those are a bit different from libgcc in that they're host libraries that can be built with any supported compiler (including non-GCC ones). In constrast, libgcc can only be built with the corresponding version of GCC. The usual restrictions on -Werror -- only use it during stages 2 and 3, or if the user explicitly passes --enable-werror -- don't apply in libgcc's case. We should always be building with the "right" version of GCC (even for Canadian crosses) and so should always be able to use -Werror. So personally, I think we should just go with: diff --git a/libgcc/config/aarch64/t-aarch64 b/libgcc/config/aarch64/t-aarch64 index b70e7b94edd..ae1588ce307 100644 --- a/libgcc/config/aarch64/t-aarch64 +++ b/libgcc/config/aarch64/t-aarch64 @@ -30,3 +30,4 @@ LIB2ADDEH += \ $(srcdir)/config/aarch64/__arm_za_disable.S SHLIB_MAPFILES += $(srcdir)/config/aarch64/libgcc-sme.ver +LIBGCC2_CFLAGS += $(WERROR) -Wno-prio-ctor-dtor ...this, but with $(WERROR) replaced by -Werror. At least, it would be a good way of finding out if there's a case I've forgotten :) Let's see what others think though. Thanks, Richard
On Tue, 8 Oct 2024 at 12:24, Richard Sandiford <richard.sandiford@arm.com> wrote: > > Christophe Lyon <christophe.lyon@linaro.org> writes: > > When --enable-werror is enabled when running the top-level configure, > > it passes --enable-werror-always to subdirs. Some of them, like > > libgcc, ignore it. > > > > This patch adds support for it, enabled only for aarch64, to avoid > > breaking bootstrap for other targets. > > > > The patch also adds -Wno-prio-ctor-dtor to avoid a warning when compiling lse_init.c > > > > libgcc/ > > * Makefile.in (WERROR): New. > > * config/aarch64/t-aarch64: Handle WERROR. Always use > > -Wno-prio-ctor-dtor. > > * configure.ac: Add support for --enable-werror-always. > > * configure: Regenerate. > > --- > > libgcc/Makefile.in | 1 + > > libgcc/config/aarch64/t-aarch64 | 1 + > > libgcc/configure | 31 +++++++++++++++++++++++++++++++ > > libgcc/configure.ac | 5 +++++ > > 4 files changed, 38 insertions(+) > > > > [...] > > diff --git a/libgcc/configure.ac b/libgcc/configure.ac > > index 4e8c036990f..6b3ea2aea5c 100644 > > --- a/libgcc/configure.ac > > +++ b/libgcc/configure.ac > > @@ -13,6 +13,7 @@ sinclude(../config/unwind_ipinfo.m4) > > sinclude(../config/gthr.m4) > > sinclude(../config/sjlj.m4) > > sinclude(../config/cet.m4) > > +sinclude(../config/warnings.m4) > > > > AC_INIT([GNU C Runtime Library], 1.0,,[libgcc]) > > AC_CONFIG_SRCDIR([static-object.mk]) > > @@ -746,6 +747,10 @@ AC_SUBST(HAVE_STRUB_SUPPORT) > > # Determine what GCC version number to use in filesystem paths. > > GCC_BASE_VER > > > > +# Only enable with --enable-werror-always until existing warnings are > > +# corrected. > > +ACX_PROG_CC_WARNINGS_ARE_ERRORS([manual]) > > It looks like this is borrowed from libcpp and/or libdecnumber. > Those are a bit different from libgcc in that they're host libraries > that can be built with any supported compiler (including non-GCC ones). > In constrast, libgcc can only be built with the corresponding version > of GCC. The usual restrictions on -Werror -- only use it during stages > 2 and 3, or if the user explicitly passes --enable-werror -- don't apply > in libgcc's case. We should always be building with the "right" version > of GCC (even for Canadian crosses) and so should always be able to use > -Werror. > > So personally, I think we should just go with: > > diff --git a/libgcc/config/aarch64/t-aarch64 b/libgcc/config/aarch64/t-aarch64 > index b70e7b94edd..ae1588ce307 100644 > --- a/libgcc/config/aarch64/t-aarch64 > +++ b/libgcc/config/aarch64/t-aarch64 > @@ -30,3 +30,4 @@ LIB2ADDEH += \ > $(srcdir)/config/aarch64/__arm_za_disable.S > > SHLIB_MAPFILES += $(srcdir)/config/aarch64/libgcc-sme.ver > +LIBGCC2_CFLAGS += $(WERROR) -Wno-prio-ctor-dtor > > ...this, but with $(WERROR) replaced by -Werror. > > At least, it would be a good way of finding out if there's a case > I've forgotten :) > Indeed that would be efficient :-) As I said, I just dared to make use of -Werror if we could inherit it from the top-level configure. > Let's see what others think though. Sure, I'm happy with forcing -Werror if others agree :-) Thanks, Christophe > > Thanks, > Richard
On Tue, Oct 8, 2024 at 6:25 AM Richard Sandiford <richard.sandiford@arm.com> wrote: > > Christophe Lyon <christophe.lyon@linaro.org> writes: > > When --enable-werror is enabled when running the top-level configure, > > it passes --enable-werror-always to subdirs. Some of them, like > > libgcc, ignore it. > > > > This patch adds support for it, enabled only for aarch64, to avoid > > breaking bootstrap for other targets. > > > > The patch also adds -Wno-prio-ctor-dtor to avoid a warning when compiling lse_init.c > > > > libgcc/ > > * Makefile.in (WERROR): New. > > * config/aarch64/t-aarch64: Handle WERROR. Always use > > -Wno-prio-ctor-dtor. > > * configure.ac: Add support for --enable-werror-always. > > * configure: Regenerate. > > --- > > libgcc/Makefile.in | 1 + > > libgcc/config/aarch64/t-aarch64 | 1 + > > libgcc/configure | 31 +++++++++++++++++++++++++++++++ > > libgcc/configure.ac | 5 +++++ > > 4 files changed, 38 insertions(+) > > > > [...] > > diff --git a/libgcc/configure.ac b/libgcc/configure.ac > > index 4e8c036990f..6b3ea2aea5c 100644 > > --- a/libgcc/configure.ac > > +++ b/libgcc/configure.ac > > @@ -13,6 +13,7 @@ sinclude(../config/unwind_ipinfo.m4) > > sinclude(../config/gthr.m4) > > sinclude(../config/sjlj.m4) > > sinclude(../config/cet.m4) > > +sinclude(../config/warnings.m4) > > > > AC_INIT([GNU C Runtime Library], 1.0,,[libgcc]) > > AC_CONFIG_SRCDIR([static-object.mk]) > > @@ -746,6 +747,10 @@ AC_SUBST(HAVE_STRUB_SUPPORT) > > # Determine what GCC version number to use in filesystem paths. > > GCC_BASE_VER > > > > +# Only enable with --enable-werror-always until existing warnings are > > +# corrected. > > +ACX_PROG_CC_WARNINGS_ARE_ERRORS([manual]) > > It looks like this is borrowed from libcpp and/or libdecnumber. > Those are a bit different from libgcc in that they're host libraries > that can be built with any supported compiler (including non-GCC ones). > In constrast, libgcc can only be built with the corresponding version > of GCC. The usual restrictions on -Werror -- only use it during stages > 2 and 3, or if the user explicitly passes --enable-werror -- don't apply > in libgcc's case. We should always be building with the "right" version > of GCC (even for Canadian crosses) and so should always be able to use > -Werror. > > So personally, I think we should just go with: > > diff --git a/libgcc/config/aarch64/t-aarch64 b/libgcc/config/aarch64/t-aarch64 > index b70e7b94edd..ae1588ce307 100644 > --- a/libgcc/config/aarch64/t-aarch64 > +++ b/libgcc/config/aarch64/t-aarch64 > @@ -30,3 +30,4 @@ LIB2ADDEH += \ > $(srcdir)/config/aarch64/__arm_za_disable.S > > SHLIB_MAPFILES += $(srcdir)/config/aarch64/libgcc-sme.ver > +LIBGCC2_CFLAGS += $(WERROR) -Wno-prio-ctor-dtor > > ...this, but with $(WERROR) replaced by -Werror. > > At least, it would be a good way of finding out if there's a case > I've forgotten :) > > Let's see what others think though. I think it would be worthwhile to test this assumption first; I have a vague memory of having seen warnings in libgcc previously that would presumably get turned into errors if -Werror were applied unconditionally... > > Thanks, > Richard
On Wed, 9 Oct 2024 at 03:05, Eric Gallager <egall@gwmail.gwu.edu> wrote: > > On Tue, Oct 8, 2024 at 6:25 AM Richard Sandiford > <richard.sandiford@arm.com> wrote: > > > > Christophe Lyon <christophe.lyon@linaro.org> writes: > > > When --enable-werror is enabled when running the top-level configure, > > > it passes --enable-werror-always to subdirs. Some of them, like > > > libgcc, ignore it. > > > > > > This patch adds support for it, enabled only for aarch64, to avoid > > > breaking bootstrap for other targets. > > > > > > The patch also adds -Wno-prio-ctor-dtor to avoid a warning when compiling lse_init.c > > > > > > libgcc/ > > > * Makefile.in (WERROR): New. > > > * config/aarch64/t-aarch64: Handle WERROR. Always use > > > -Wno-prio-ctor-dtor. > > > * configure.ac: Add support for --enable-werror-always. > > > * configure: Regenerate. > > > --- > > > libgcc/Makefile.in | 1 + > > > libgcc/config/aarch64/t-aarch64 | 1 + > > > libgcc/configure | 31 +++++++++++++++++++++++++++++++ > > > libgcc/configure.ac | 5 +++++ > > > 4 files changed, 38 insertions(+) > > > > > > [...] > > > diff --git a/libgcc/configure.ac b/libgcc/configure.ac > > > index 4e8c036990f..6b3ea2aea5c 100644 > > > --- a/libgcc/configure.ac > > > +++ b/libgcc/configure.ac > > > @@ -13,6 +13,7 @@ sinclude(../config/unwind_ipinfo.m4) > > > sinclude(../config/gthr.m4) > > > sinclude(../config/sjlj.m4) > > > sinclude(../config/cet.m4) > > > +sinclude(../config/warnings.m4) > > > > > > AC_INIT([GNU C Runtime Library], 1.0,,[libgcc]) > > > AC_CONFIG_SRCDIR([static-object.mk]) > > > @@ -746,6 +747,10 @@ AC_SUBST(HAVE_STRUB_SUPPORT) > > > # Determine what GCC version number to use in filesystem paths. > > > GCC_BASE_VER > > > > > > +# Only enable with --enable-werror-always until existing warnings are > > > +# corrected. > > > +ACX_PROG_CC_WARNINGS_ARE_ERRORS([manual]) > > > > It looks like this is borrowed from libcpp and/or libdecnumber. > > Those are a bit different from libgcc in that they're host libraries > > that can be built with any supported compiler (including non-GCC ones). > > In constrast, libgcc can only be built with the corresponding version > > of GCC. The usual restrictions on -Werror -- only use it during stages > > 2 and 3, or if the user explicitly passes --enable-werror -- don't apply > > in libgcc's case. We should always be building with the "right" version > > of GCC (even for Canadian crosses) and so should always be able to use > > -Werror. > > > > So personally, I think we should just go with: > > > > diff --git a/libgcc/config/aarch64/t-aarch64 b/libgcc/config/aarch64/t-aarch64 > > index b70e7b94edd..ae1588ce307 100644 > > --- a/libgcc/config/aarch64/t-aarch64 > > +++ b/libgcc/config/aarch64/t-aarch64 > > @@ -30,3 +30,4 @@ LIB2ADDEH += \ > > $(srcdir)/config/aarch64/__arm_za_disable.S > > > > SHLIB_MAPFILES += $(srcdir)/config/aarch64/libgcc-sme.ver > > +LIBGCC2_CFLAGS += $(WERROR) -Wno-prio-ctor-dtor > > > > ...this, but with $(WERROR) replaced by -Werror. > > > > At least, it would be a good way of finding out if there's a case > > I've forgotten :) > > > > Let's see what others think though. > > I think it would be worthwhile to test this assumption first; I have a > vague memory of having seen warnings in libgcc previously that would > presumably get turned into errors if -Werror were applied > unconditionally... > Sorry, it's not clear to me what you mean by "test this assumption" ? Do you mean I should push the patch with unconditional -Werror and monitor what happens for a while? Or investigate more / other targets? Or wait for others to commit? Thanks, Christophe > > > > Thanks, > > Richard
On Wed, Oct 9, 2024 at 4:54 AM Christophe Lyon <christophe.lyon@linaro.org> wrote: > > On Wed, 9 Oct 2024 at 03:05, Eric Gallager <egall@gwmail.gwu.edu> wrote: > > > > On Tue, Oct 8, 2024 at 6:25 AM Richard Sandiford > > <richard.sandiford@arm.com> wrote: > > > > > > Christophe Lyon <christophe.lyon@linaro.org> writes: > > > > When --enable-werror is enabled when running the top-level configure, > > > > it passes --enable-werror-always to subdirs. Some of them, like > > > > libgcc, ignore it. > > > > > > > > This patch adds support for it, enabled only for aarch64, to avoid > > > > breaking bootstrap for other targets. > > > > > > > > The patch also adds -Wno-prio-ctor-dtor to avoid a warning when compiling lse_init.c > > > > > > > > libgcc/ > > > > * Makefile.in (WERROR): New. > > > > * config/aarch64/t-aarch64: Handle WERROR. Always use > > > > -Wno-prio-ctor-dtor. > > > > * configure.ac: Add support for --enable-werror-always. > > > > * configure: Regenerate. > > > > --- > > > > libgcc/Makefile.in | 1 + > > > > libgcc/config/aarch64/t-aarch64 | 1 + > > > > libgcc/configure | 31 +++++++++++++++++++++++++++++++ > > > > libgcc/configure.ac | 5 +++++ > > > > 4 files changed, 38 insertions(+) > > > > > > > > [...] > > > > diff --git a/libgcc/configure.ac b/libgcc/configure.ac > > > > index 4e8c036990f..6b3ea2aea5c 100644 > > > > --- a/libgcc/configure.ac > > > > +++ b/libgcc/configure.ac > > > > @@ -13,6 +13,7 @@ sinclude(../config/unwind_ipinfo.m4) > > > > sinclude(../config/gthr.m4) > > > > sinclude(../config/sjlj.m4) > > > > sinclude(../config/cet.m4) > > > > +sinclude(../config/warnings.m4) > > > > > > > > AC_INIT([GNU C Runtime Library], 1.0,,[libgcc]) > > > > AC_CONFIG_SRCDIR([static-object.mk]) > > > > @@ -746,6 +747,10 @@ AC_SUBST(HAVE_STRUB_SUPPORT) > > > > # Determine what GCC version number to use in filesystem paths. > > > > GCC_BASE_VER > > > > > > > > +# Only enable with --enable-werror-always until existing warnings are > > > > +# corrected. > > > > +ACX_PROG_CC_WARNINGS_ARE_ERRORS([manual]) > > > > > > It looks like this is borrowed from libcpp and/or libdecnumber. > > > Those are a bit different from libgcc in that they're host libraries > > > that can be built with any supported compiler (including non-GCC ones). > > > In constrast, libgcc can only be built with the corresponding version > > > of GCC. The usual restrictions on -Werror -- only use it during stages > > > 2 and 3, or if the user explicitly passes --enable-werror -- don't apply > > > in libgcc's case. We should always be building with the "right" version > > > of GCC (even for Canadian crosses) and so should always be able to use > > > -Werror. > > > > > > So personally, I think we should just go with: > > > > > > diff --git a/libgcc/config/aarch64/t-aarch64 b/libgcc/config/aarch64/t-aarch64 > > > index b70e7b94edd..ae1588ce307 100644 > > > --- a/libgcc/config/aarch64/t-aarch64 > > > +++ b/libgcc/config/aarch64/t-aarch64 > > > @@ -30,3 +30,4 @@ LIB2ADDEH += \ > > > $(srcdir)/config/aarch64/__arm_za_disable.S > > > > > > SHLIB_MAPFILES += $(srcdir)/config/aarch64/libgcc-sme.ver > > > +LIBGCC2_CFLAGS += $(WERROR) -Wno-prio-ctor-dtor > > > > > > ...this, but with $(WERROR) replaced by -Werror. > > > > > > At least, it would be a good way of finding out if there's a case > > > I've forgotten :) > > > > > > Let's see what others think though. > > > > I think it would be worthwhile to test this assumption first; I have a > > vague memory of having seen warnings in libgcc previously that would > > presumably get turned into errors if -Werror were applied > > unconditionally... > > > Sorry, it's not clear to me what you mean by "test this assumption" ? > Do you mean I should push the patch with unconditional -Werror and > monitor what happens for a while? > Or investigate more / other targets? > Or wait for others to commit? > I mean, I think we should try the original approach of having it be enableable manually first, let some people test by enabling it manually, and then if they all report back success, then we can go ahead with the unconditional -Werror version of it. > Thanks, > > Christophe > > > > > > > Thanks, > > > Richard
Eric Gallager <egall@gwmail.gwu.edu> writes: > On Wed, Oct 9, 2024 at 4:54 AM Christophe Lyon > <christophe.lyon@linaro.org> wrote: >> >> On Wed, 9 Oct 2024 at 03:05, Eric Gallager <egall@gwmail.gwu.edu> wrote: >> > >> > On Tue, Oct 8, 2024 at 6:25 AM Richard Sandiford >> > <richard.sandiford@arm.com> wrote: >> > > >> > > Christophe Lyon <christophe.lyon@linaro.org> writes: >> > > > When --enable-werror is enabled when running the top-level configure, >> > > > it passes --enable-werror-always to subdirs. Some of them, like >> > > > libgcc, ignore it. >> > > > >> > > > This patch adds support for it, enabled only for aarch64, to avoid >> > > > breaking bootstrap for other targets. >> > > > >> > > > The patch also adds -Wno-prio-ctor-dtor to avoid a warning when compiling lse_init.c >> > > > >> > > > libgcc/ >> > > > * Makefile.in (WERROR): New. >> > > > * config/aarch64/t-aarch64: Handle WERROR. Always use >> > > > -Wno-prio-ctor-dtor. >> > > > * configure.ac: Add support for --enable-werror-always. >> > > > * configure: Regenerate. >> > > > --- >> > > > libgcc/Makefile.in | 1 + >> > > > libgcc/config/aarch64/t-aarch64 | 1 + >> > > > libgcc/configure | 31 +++++++++++++++++++++++++++++++ >> > > > libgcc/configure.ac | 5 +++++ >> > > > 4 files changed, 38 insertions(+) >> > > > >> > > > [...] >> > > > diff --git a/libgcc/configure.ac b/libgcc/configure.ac >> > > > index 4e8c036990f..6b3ea2aea5c 100644 >> > > > --- a/libgcc/configure.ac >> > > > +++ b/libgcc/configure.ac >> > > > @@ -13,6 +13,7 @@ sinclude(../config/unwind_ipinfo.m4) >> > > > sinclude(../config/gthr.m4) >> > > > sinclude(../config/sjlj.m4) >> > > > sinclude(../config/cet.m4) >> > > > +sinclude(../config/warnings.m4) >> > > > >> > > > AC_INIT([GNU C Runtime Library], 1.0,,[libgcc]) >> > > > AC_CONFIG_SRCDIR([static-object.mk]) >> > > > @@ -746,6 +747,10 @@ AC_SUBST(HAVE_STRUB_SUPPORT) >> > > > # Determine what GCC version number to use in filesystem paths. >> > > > GCC_BASE_VER >> > > > >> > > > +# Only enable with --enable-werror-always until existing warnings are >> > > > +# corrected. >> > > > +ACX_PROG_CC_WARNINGS_ARE_ERRORS([manual]) >> > > >> > > It looks like this is borrowed from libcpp and/or libdecnumber. >> > > Those are a bit different from libgcc in that they're host libraries >> > > that can be built with any supported compiler (including non-GCC ones). >> > > In constrast, libgcc can only be built with the corresponding version >> > > of GCC. The usual restrictions on -Werror -- only use it during stages >> > > 2 and 3, or if the user explicitly passes --enable-werror -- don't apply >> > > in libgcc's case. We should always be building with the "right" version >> > > of GCC (even for Canadian crosses) and so should always be able to use >> > > -Werror. >> > > >> > > So personally, I think we should just go with: >> > > >> > > diff --git a/libgcc/config/aarch64/t-aarch64 b/libgcc/config/aarch64/t-aarch64 >> > > index b70e7b94edd..ae1588ce307 100644 >> > > --- a/libgcc/config/aarch64/t-aarch64 >> > > +++ b/libgcc/config/aarch64/t-aarch64 >> > > @@ -30,3 +30,4 @@ LIB2ADDEH += \ >> > > $(srcdir)/config/aarch64/__arm_za_disable.S >> > > >> > > SHLIB_MAPFILES += $(srcdir)/config/aarch64/libgcc-sme.ver >> > > +LIBGCC2_CFLAGS += $(WERROR) -Wno-prio-ctor-dtor >> > > >> > > ...this, but with $(WERROR) replaced by -Werror. >> > > >> > > At least, it would be a good way of finding out if there's a case >> > > I've forgotten :) >> > > >> > > Let's see what others think though. >> > >> > I think it would be worthwhile to test this assumption first; I have a >> > vague memory of having seen warnings in libgcc previously that would >> > presumably get turned into errors if -Werror were applied >> > unconditionally... >> > >> Sorry, it's not clear to me what you mean by "test this assumption" ? >> Do you mean I should push the patch with unconditional -Werror and >> monitor what happens for a while? >> Or investigate more / other targets? >> Or wait for others to commit? >> > > I mean, I think we should try the original approach of having it be > enableable manually first, let some people test by enabling it > manually, and then if they all report back success, then we can go > ahead with the unconditional -Werror version of it. I can see the attraction. But that would mean adding code only to take it out later. Plus, the original patch would enable -Werror for stages 2 and 3 anyway, so everyone who bootstraps would still be testing the -Werror path, whether they'd chosen to or not. My point is that stage 1 isn't really a different case from stages 2 and 3 for libgcc, since in all three cases, it's the recently built gcc that is being used to build libgcc. libgcc also gets the majority of its command-line flags directly from the gcc directory (via libgcc.mvars), rather than from libgcc's own configure line. How about going for an unconditional -Werror (for aarch64 only), but with a pre-approval for anyone with commit access to revert it if it breaks their build? Thanks, Richard
Richard Sandiford <richard.sandiford@arm.com> writes: > Christophe Lyon <christophe.lyon@linaro.org> writes: >> When --enable-werror is enabled when running the top-level configure, >> it passes --enable-werror-always to subdirs. Some of them, like >> libgcc, ignore it. >> >> This patch adds support for it, enabled only for aarch64, to avoid >> breaking bootstrap for other targets. >> >> The patch also adds -Wno-prio-ctor-dtor to avoid a warning when compiling lse_init.c >> >> libgcc/ >> * Makefile.in (WERROR): New. >> * config/aarch64/t-aarch64: Handle WERROR. Always use >> -Wno-prio-ctor-dtor. >> * configure.ac: Add support for --enable-werror-always. >> * configure: Regenerate. >> --- >> libgcc/Makefile.in | 1 + >> libgcc/config/aarch64/t-aarch64 | 1 + >> libgcc/configure | 31 +++++++++++++++++++++++++++++++ >> libgcc/configure.ac | 5 +++++ >> 4 files changed, 38 insertions(+) >> >> [...] >> diff --git a/libgcc/configure.ac b/libgcc/configure.ac >> index 4e8c036990f..6b3ea2aea5c 100644 >> --- a/libgcc/configure.ac >> +++ b/libgcc/configure.ac >> @@ -13,6 +13,7 @@ sinclude(../config/unwind_ipinfo.m4) >> sinclude(../config/gthr.m4) >> sinclude(../config/sjlj.m4) >> sinclude(../config/cet.m4) >> +sinclude(../config/warnings.m4) >> >> AC_INIT([GNU C Runtime Library], 1.0,,[libgcc]) >> AC_CONFIG_SRCDIR([static-object.mk]) >> @@ -746,6 +747,10 @@ AC_SUBST(HAVE_STRUB_SUPPORT) >> # Determine what GCC version number to use in filesystem paths. >> GCC_BASE_VER >> >> +# Only enable with --enable-werror-always until existing warnings are >> +# corrected. >> +ACX_PROG_CC_WARNINGS_ARE_ERRORS([manual]) > > It looks like this is borrowed from libcpp and/or libdecnumber. > Those are a bit different from libgcc in that they're host libraries > that can be built with any supported compiler (including non-GCC ones). > In constrast, libgcc can only be built with the corresponding version > of GCC. The usual restrictions on -Werror -- only use it during stages > 2 and 3, or if the user explicitly passes --enable-werror -- don't apply > in libgcc's case. We should always be building with the "right" version > of GCC (even for Canadian crosses) and so should always be able to use > -Werror. > > So personally, I think we should just go with: > > diff --git a/libgcc/config/aarch64/t-aarch64 b/libgcc/config/aarch64/t-aarch64 > index b70e7b94edd..ae1588ce307 100644 > --- a/libgcc/config/aarch64/t-aarch64 > +++ b/libgcc/config/aarch64/t-aarch64 > @@ -30,3 +30,4 @@ LIB2ADDEH += \ > $(srcdir)/config/aarch64/__arm_za_disable.S > > SHLIB_MAPFILES += $(srcdir)/config/aarch64/libgcc-sme.ver > +LIBGCC2_CFLAGS += $(WERROR) -Wno-prio-ctor-dtor > > ...this, but with $(WERROR) replaced by -Werror. > > At least, it would be a good way of finding out if there's a case > I've forgotten :) > > Let's see what others think though. As per the later discussion, the t-aarch64 change described above is OK for trunk, but anyone with commit access should feel free to revert it if it breaks their build. (Although please post a description of what went wrong as well :)) Thanks for doing this. Richard
On Thu, 17 Oct 2024 at 15:06, Richard Sandiford <richard.sandiford@arm.com> wrote: > > Richard Sandiford <richard.sandiford@arm.com> writes: > > Christophe Lyon <christophe.lyon@linaro.org> writes: > >> When --enable-werror is enabled when running the top-level configure, > >> it passes --enable-werror-always to subdirs. Some of them, like > >> libgcc, ignore it. > >> > >> This patch adds support for it, enabled only for aarch64, to avoid > >> breaking bootstrap for other targets. > >> > >> The patch also adds -Wno-prio-ctor-dtor to avoid a warning when compiling lse_init.c > >> > >> libgcc/ > >> * Makefile.in (WERROR): New. > >> * config/aarch64/t-aarch64: Handle WERROR. Always use > >> -Wno-prio-ctor-dtor. > >> * configure.ac: Add support for --enable-werror-always. > >> * configure: Regenerate. > >> --- > >> libgcc/Makefile.in | 1 + > >> libgcc/config/aarch64/t-aarch64 | 1 + > >> libgcc/configure | 31 +++++++++++++++++++++++++++++++ > >> libgcc/configure.ac | 5 +++++ > >> 4 files changed, 38 insertions(+) > >> > >> [...] > >> diff --git a/libgcc/configure.ac b/libgcc/configure.ac > >> index 4e8c036990f..6b3ea2aea5c 100644 > >> --- a/libgcc/configure.ac > >> +++ b/libgcc/configure.ac > >> @@ -13,6 +13,7 @@ sinclude(../config/unwind_ipinfo.m4) > >> sinclude(../config/gthr.m4) > >> sinclude(../config/sjlj.m4) > >> sinclude(../config/cet.m4) > >> +sinclude(../config/warnings.m4) > >> > >> AC_INIT([GNU C Runtime Library], 1.0,,[libgcc]) > >> AC_CONFIG_SRCDIR([static-object.mk]) > >> @@ -746,6 +747,10 @@ AC_SUBST(HAVE_STRUB_SUPPORT) > >> # Determine what GCC version number to use in filesystem paths. > >> GCC_BASE_VER > >> > >> +# Only enable with --enable-werror-always until existing warnings are > >> +# corrected. > >> +ACX_PROG_CC_WARNINGS_ARE_ERRORS([manual]) > > > > It looks like this is borrowed from libcpp and/or libdecnumber. > > Those are a bit different from libgcc in that they're host libraries > > that can be built with any supported compiler (including non-GCC ones). > > In constrast, libgcc can only be built with the corresponding version > > of GCC. The usual restrictions on -Werror -- only use it during stages > > 2 and 3, or if the user explicitly passes --enable-werror -- don't apply > > in libgcc's case. We should always be building with the "right" version > > of GCC (even for Canadian crosses) and so should always be able to use > > -Werror. > > > > So personally, I think we should just go with: > > > > diff --git a/libgcc/config/aarch64/t-aarch64 b/libgcc/config/aarch64/t-aarch64 > > index b70e7b94edd..ae1588ce307 100644 > > --- a/libgcc/config/aarch64/t-aarch64 > > +++ b/libgcc/config/aarch64/t-aarch64 > > @@ -30,3 +30,4 @@ LIB2ADDEH += \ > > $(srcdir)/config/aarch64/__arm_za_disable.S > > > > SHLIB_MAPFILES += $(srcdir)/config/aarch64/libgcc-sme.ver > > +LIBGCC2_CFLAGS += $(WERROR) -Wno-prio-ctor-dtor > > > > ...this, but with $(WERROR) replaced by -Werror. > > > > At least, it would be a good way of finding out if there's a case > > I've forgotten :) > > > > Let's see what others think though. > > As per the later discussion, the t-aarch64 change described above is OK > for trunk, but anyone with commit access should feel free to revert it > if it breaks their build. (Although please post a description of what > went wrong as well :)) > > Thanks for doing this. > Thanks, find attached what I'm pushing. Christophe > Richard
diff --git a/libgcc/Makefile.in b/libgcc/Makefile.in index 0e46e9ef768..eca62546642 100644 --- a/libgcc/Makefile.in +++ b/libgcc/Makefile.in @@ -84,6 +84,7 @@ AR_FLAGS = rc CC = @CC@ CFLAGS = @CFLAGS@ +WERROR = @WERROR@ RANLIB = @RANLIB@ LN_S = @LN_S@ diff --git a/libgcc/config/aarch64/t-aarch64 b/libgcc/config/aarch64/t-aarch64 index b70e7b94edd..ae1588ce307 100644 --- a/libgcc/config/aarch64/t-aarch64 +++ b/libgcc/config/aarch64/t-aarch64 @@ -30,3 +30,4 @@ LIB2ADDEH += \ $(srcdir)/config/aarch64/__arm_za_disable.S SHLIB_MAPFILES += $(srcdir)/config/aarch64/libgcc-sme.ver +LIBGCC2_CFLAGS += $(WERROR) -Wno-prio-ctor-dtor diff --git a/libgcc/configure b/libgcc/configure index cff1eff9625..ae56f7dbdc9 100755 --- a/libgcc/configure +++ b/libgcc/configure @@ -592,6 +592,7 @@ enable_execute_stack asm_hidden_op extra_parts cpu_type +WERROR get_gcc_base_ver HAVE_STRUB_SUPPORT thread_header @@ -719,6 +720,7 @@ enable_tm_clone_registry with_glibc_version enable_tls with_gcc_major_version_only +enable_werror_always ' ac_precious_vars='build_alias host_alias @@ -1361,6 +1363,7 @@ Optional Features: installations without PT_GNU_EH_FRAME support --disable-tm-clone-registry disable TM clone registry --enable-tls Use thread-local storage [default=yes] + --enable-werror-always enable -Werror despite compiler version Optional Packages: --with-PACKAGE[=ARG] use PACKAGE [ARG=yes] @@ -5808,6 +5811,34 @@ fi +# Only enable with --enable-werror-always until existing warnings are +# corrected. +ac_ext=c +ac_cpp='$CPP $CPPFLAGS' +ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5' +ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5' +ac_compiler_gnu=$ac_cv_c_compiler_gnu + +WERROR= +# Check whether --enable-werror-always was given. +if test "${enable_werror_always+set}" = set; then : + enableval=$enable_werror_always; +else + enable_werror_always=no +fi + +if test $enable_werror_always = yes; then : + WERROR="$WERROR${WERROR:+ }-Werror" +fi + +ac_ext=c +ac_cpp='$CPP $CPPFLAGS' +ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5' +ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5' +ac_compiler_gnu=$ac_cv_c_compiler_gnu + + + # Substitute configuration variables diff --git a/libgcc/configure.ac b/libgcc/configure.ac index 4e8c036990f..6b3ea2aea5c 100644 --- a/libgcc/configure.ac +++ b/libgcc/configure.ac @@ -13,6 +13,7 @@ sinclude(../config/unwind_ipinfo.m4) sinclude(../config/gthr.m4) sinclude(../config/sjlj.m4) sinclude(../config/cet.m4) +sinclude(../config/warnings.m4) AC_INIT([GNU C Runtime Library], 1.0,,[libgcc]) AC_CONFIG_SRCDIR([static-object.mk]) @@ -746,6 +747,10 @@ AC_SUBST(HAVE_STRUB_SUPPORT) # Determine what GCC version number to use in filesystem paths. GCC_BASE_VER +# Only enable with --enable-werror-always until existing warnings are +# corrected. +ACX_PROG_CC_WARNINGS_ARE_ERRORS([manual]) + # Substitute configuration variables AC_SUBST(cpu_type) AC_SUBST(extra_parts)