| Message ID | 20141029102817.GP10376@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10/29/2014 11:28 AM, Jakub Jelinek wrote: > If this passes bootstrap/regtest, is it ok for trunk (should fix > two bootstrap issues)? Is the > https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02936.html > patch ok too (that one already tested; another bootstrap issue)? Both seem okay, though I'd have to look at the whole thread to understand what libcc1 is. :) Just two questions: 1) what's the issue that you need to disable asan for? 2) why is GMPLIB not handled in the same way? Paolo
On Wed, Oct 29, 2014 at 11:37:42AM +0100, Paolo Bonzini wrote: > > > On 10/29/2014 11:28 AM, Jakub Jelinek wrote: > > If this passes bootstrap/regtest, is it ok for trunk (should fix > > two bootstrap issues)? Is the > > https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02936.html > > patch ok too (that one already tested; another bootstrap issue)? > > Both seem okay, though I'd have to look at the whole thread to > understand what libcc1 is. :) It is a library for communication between the debugger and a GCC plugin (and the plugin itself). So, the library is dlopened into GDB and the plugin that links against that library is dlopened by GCC when GDB asks the library it dlopened to run the compiler with the plugin. > Just two questions: > > 1) what's the issue that you need to disable asan for? -fsanitize=address generally doesn't work or doesn't work too well, if the binary is not built with -fsanitize=address, but shared library dlopened into it is. So, we want to avoid instrumenting plugins that way (we already don't instrument lto-plugin for that reason, because ld might not be asan instrumented, and libcc1 is similar case, when gdb dlopens the library, it might not be instrumented either). > 2) why is GMPLIB not handled in the same way? The only problem is that system.h includes gmp.h, so we need a way to find that header. I think libcc1 doesn't use any functions from gmp itself, so if gmp.h can be included, GMPLIB isn't really needed. Jakub
On 10/29/2014 11:51 AM, Jakub Jelinek wrote: > On Wed, Oct 29, 2014 at 11:37:42AM +0100, Paolo Bonzini wrote: >> >> >> On 10/29/2014 11:28 AM, Jakub Jelinek wrote: >>> If this passes bootstrap/regtest, is it ok for trunk (should fix >>> two bootstrap issues)? Is the >>> https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02936.html >>> patch ok too (that one already tested; another bootstrap issue)? >> >> Both seem okay, though I'd have to look at the whole thread to >> understand what libcc1 is. :) > > It is a library for communication between the debugger and > a GCC plugin (and the plugin itself). So, the library is > dlopened into GDB and the plugin that links against that library > is dlopened by GCC when GDB asks the library it dlopened to > run the compiler with the plugin. > >> Just two questions: >> >> 1) what's the issue that you need to disable asan for? > > -fsanitize=address generally doesn't work or doesn't work too well, > if the binary is not built with -fsanitize=address, but shared library > dlopened into it is. So, we want to avoid instrumenting plugins > that way (we already don't instrument lto-plugin for that reason, > because ld might not be asan instrumented, and libcc1 is similar case, > when gdb dlopens the library, it might not be instrumented either). Thanks for explaining. I can see intuitively why that could be a problem... >> 2) why is GMPLIB not handled in the same way? > > The only problem is that system.h includes gmp.h, so we need a way > to find that header. I think libcc1 doesn't use any functions from gmp > itself, so if gmp.h can be included, GMPLIB isn't really needed. Ah, got it. Is it hard to move the inclusion to the actual users? Paolo
On Wed, Oct 29, 2014 at 11:53:28AM +0100, Paolo Bonzini wrote: > >> 2) why is GMPLIB not handled in the same way? > > > > The only problem is that system.h includes gmp.h, so we need a way > > to find that header. I think libcc1 doesn't use any functions from gmp > > itself, so if gmp.h can be included, GMPLIB isn't really needed. > > Ah, got it. Is it hard to move the inclusion to the actual users? I think it is hard. I think it has been moved to system.h very much intentionally, as including gmp.h only in selected headers was causing lots of troubles, e.g. because of #pragma GCC poison at the end of system.h, I believe some gmp.h versions were using some poisoned symbols. system.h doesn't include gmp.h if -DGENERATOR_FILE, but libcc1 is not a generator, so that is not appropriate, it can use various other GCC headers that are not suitable for generators. GMPINC has been suggested by Joseph, I'd think if we ever need also GMPLIB, we'd clearly see it as link failures of libcc1 first and could add it only when really needed. Jakub
On 29/10/14 10:53, Paolo Bonzini wrote: >>> 2) why is GMPLIB not handled in the same way? >> >> The only problem is that system.h includes gmp.h, so we need a way >> to find that header. I think libcc1 doesn't use any functions from gmp >> itself, so if gmp.h can be included, GMPLIB isn't really needed. > > Ah, got it. Is it hard to move the inclusion to the actual users? We don't, I was looking at this issue today. It is just as Jakub explains. Cheers Phil
On 29/10/14 10:53, Paolo Bonzini wrote: > > > On 10/29/2014 11:51 AM, Jakub Jelinek wrote: >> On Wed, Oct 29, 2014 at 11:37:42AM +0100, Paolo Bonzini wrote: >>> >>> >>> On 10/29/2014 11:28 AM, Jakub Jelinek wrote: >>>> If this passes bootstrap/regtest, is it ok for trunk (should fix >>>> two bootstrap issues)? Is the >>>> https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02936.html >>>> patch ok too (that one already tested; another bootstrap issue)? >>> >>> Both seem okay, though I'd have to look at the whole thread to >>> understand what libcc1 is. :) >> >> It is a library for communication between the debugger and >> a GCC plugin (and the plugin itself). So, the library is >> dlopened into GDB and the plugin that links against that library >> is dlopened by GCC when GDB asks the library it dlopened to >> run the compiler with the plugin. >> Adding on to what Jakub said, it allows GDB access to GCC's parser. There are a number of reasons why, but right now that means we can compile code snippets in GDB (without the source of the current inferior), allow access to symbols of that inferior in that source snippet, etc. We then inject and execute it. We are currently writing a wiki article about it. Not required reading or anything, but more information for the curious. https://sourceware.org/gdb/wiki/GCCCompileAndExecute (That is a work in progress). There is also a video of a presentation I did at Cauldron somewhere. Cheers Phil
On 10/29/2014 11:59 AM, Jakub Jelinek wrote: >> > Ah, got it. Is it hard to move the inclusion to the actual users? > I think it is hard. I think it has been moved to system.h very much > intentionally, as including gmp.h only in selected headers was causing lots > of troubles, e.g. because of #pragma GCC poison at the end of system.h, > I believe some gmp.h versions were using some poisoned symbols. > system.h doesn't include gmp.h if -DGENERATOR_FILE, but libcc1 is not a > generator, so that is not appropriate, it can use various other GCC headers > that are not suitable for generators. GMPINC has been suggested by Joseph, > I'd think if we ever need also GMPLIB, we'd clearly see it as link failures > of libcc1 first and could add it only when really needed. Fair enough, thanks! Paolo
On 10/29/14 04:28, Jakub Jelinek wrote: > On Tue, Oct 28, 2014 at 05:36:50PM +0000, Phil Muldoon wrote: >> On 28/10/14 13:19, Joseph S. Myers wrote: >>> I'm seeing a different bootstrap failure from those already discussed: >>> >>> In file included from >>> /scratch/jmyers/fsf/gcc-mainline/libcc1/../gcc/gcc-plugin.h:28:0, >>> from >>> /scratch/jmyers/fsf/gcc-mainline/libcc1/plugin.cc:34: >>> /scratch/jmyers/fsf/gcc-mainline/libcc1/../gcc/system.h:653:17: fatal error: gmp.h: No such file or directory >>> >>> It appears the build is ignoring the --with-gmp option passed to >>> configure. Since <gmp.h> is included in system.h, if you include system.h >>> you have to pass the right -I option corresponding to --with-gmp / >>> --with-gmp-include. (There are several other such configure options for >>> MPFR, MPC, CLooG, ISL, libiconv at least - whether they are relevant >>> depends on whether your code ends up including the relevant headers.) >> >> Hi, sorry for the troubles! I am having difficulty seeing this fail on >> my system. I built gmp from upstream, installed it, and pointed to >> the install location with --with-gmp. Which stage does your build fail >> at? >> >> I am actually not totally sure how to respect the -with-gmp argument >> in libcc1. auto* tools are not my strongest skill. ;) >> >> I notice gcc/configure.ac I think just exports the variables to >> Makefile.in from the main configure script. That what we should do in >> this case? > > Here is a patch I'm bootstrapping/regtesting now (but, with system gmp > installed). I've verified that with this patch stage1 libcc1 is built > without -Werror in flags, while stage2 libcc1 is built with -Werror. > > If this passes bootstrap/regtest, is it ok for trunk (should fix > two bootstrap issues)? Is the > https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02936.html > patch ok too (that one already tested; another bootstrap issue)? > > 2014-10-29 Jakub Jelinek <jakub@redhat.com> > Phil Muldoon <pmuldoon@redhat.com> > > * configure.ac: Remove -Werror addition to WARN_FLAGS. Add > ACX_PROG_CC_WARNINGS_ARE_ERRORS and AC_ARG_VAR for GMPINC. > * Makefile.am (AM_CPPFLAGS): Add $(GMPINC). > (WERROR_FLAG): Remove. > (AM_CXXFLAGS): Use $(WERROR) instead of $(WERROR_FLAG). > * configure: Regenerated. > * Makefile.in: Regenerated. So is this still relevant if we stop bootstrapping libcc1? The patch is OK if it's still needed. Thanks, Jeff
--- libcc1/configure.ac.jj 2014-10-28 14:39:52.000000000 +0100 +++ libcc1/configure.ac 2014-10-29 10:01:36.515497687 +0100 @@ -52,8 +52,10 @@ gcc_version=`cat $srcdir/../gcc/BASE-VER AC_SUBST(gcc_version) ACX_PROG_CC_WARNING_OPTS([-W -Wall], [WARN_FLAGS]) -WARN_FLAGS="$WARN_FLAGS -Werror" AC_SUBST(WARN_FLAGS) +ACX_PROG_CC_WARNINGS_ARE_ERRORS([manual]) + +AC_ARG_VAR(GMPINC,[How to find GMP include files]) libsuffix= if test "$GXX" = yes; then --- libcc1/Makefile.am.jj 2014-10-29 09:53:00.000000000 +0100 +++ libcc1/Makefile.am 2014-10-29 10:02:08.481885746 +0100 @@ -21,9 +21,8 @@ gcc_build_dir = ../$(host_subdir)/gcc AM_CPPFLAGS = -I $(srcdir)/../include -I $(srcdir)/../libgcc \ -I $(gcc_build_dir) -I$(srcdir)/../gcc \ -I $(srcdir)/../gcc/c -I $(srcdir)/../gcc/c-family \ - -I $(srcdir)/../libcpp/include -WERROR_FLAG = -Werror -AM_CXXFLAGS = $(WARN_FLAGS) $(WERROR_FLAG) $(visibility) + -I $(srcdir)/../libcpp/include $(GMPINC) +AM_CXXFLAGS = $(WARN_FLAGS) $(WERROR) $(visibility) override CXXFLAGS := $(filter-out -fsanitize=address,$(CXXFLAGS)) override LDFLAGS := $(filter-out -fsanitize=address,$(LDFLAGS)) # Can be simplified when libiberty becomes a normal convenience library. --- libcc1/configure.jj 2014-10-28 14:39:52.000000000 +0100 +++ libcc1/configure 2014-10-29 10:02:32.957419099 +0100 @@ -605,6 +605,8 @@ LIBOBJS ENABLE_PLUGIN_FALSE ENABLE_PLUGIN_TRUE libsuffix +GMPINC +WERROR WARN_FLAGS gcc_version visibility @@ -743,6 +745,7 @@ with_pic enable_fast_install with_gnu_ld enable_libtool_lock +enable_werror_always enable_plugin ' ac_precious_vars='build_alias @@ -757,7 +760,8 @@ CPP CXX CXXFLAGS CCC -CXXCPP' +CXXCPP +GMPINC' # Initialize some variables set by options. @@ -1387,6 +1391,7 @@ Optional Features: --enable-fast-install[=PKGS] optimize for fast installation [default=yes] --disable-libtool-lock avoid locking (might break parallel builds) + --enable-werror-always enable -Werror despite compiler version --enable-plugin enable plugin support Optional Packages: @@ -1409,6 +1414,7 @@ Some influential environment variables: CXX C++ compiler command CXXFLAGS C++ compiler flags CXXCPP C++ preprocessor + GMPINC How to find GMP include files Use these variables to override the choices made by `configure' or to help it to find libraries and programs with nonstandard names/locations. @@ -10530,7 +10536,7 @@ else lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 lt_status=$lt_dlunknown cat > conftest.$ac_ext <<_LT_EOF -#line 10533 "configure" +#line 10539 "configure" #include "confdefs.h" #if HAVE_DLFCN_H @@ -10636,7 +10642,7 @@ else lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 lt_status=$lt_dlunknown cat > conftest.$ac_ext <<_LT_EOF -#line 10639 "configure" +#line 10645 "configure" #include "confdefs.h" #if HAVE_DLFCN_H @@ -14342,7 +14348,21 @@ fi done CFLAGS="$save_CFLAGS" -WARN_FLAGS="$WARN_FLAGS -Werror" + +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 + + + libsuffix= --- libcc1/Makefile.in.jj 2014-10-29 09:53:00.000000000 +0100 +++ libcc1/Makefile.in 2014-10-29 10:02:35.739365984 +0100 @@ -140,6 +140,7 @@ ECHO_T = @ECHO_T@ EGREP = @EGREP@ EXEEXT = @EXEEXT@ FGREP = @FGREP@ +GMPINC = @GMPINC@ GREP = @GREP@ INSTALL = @INSTALL@ INSTALL_DATA = @INSTALL_DATA@ @@ -178,6 +179,7 @@ SHELL = @SHELL@ STRIP = @STRIP@ VERSION = @VERSION@ WARN_FLAGS = @WARN_FLAGS@ +WERROR = @WERROR@ abs_builddir = @abs_builddir@ abs_srcdir = @abs_srcdir@ abs_top_builddir = @abs_top_builddir@ @@ -247,10 +249,9 @@ gcc_build_dir = ../$(host_subdir)/gcc AM_CPPFLAGS = -I $(srcdir)/../include -I $(srcdir)/../libgcc \ -I $(gcc_build_dir) -I$(srcdir)/../gcc \ -I $(srcdir)/../gcc/c -I $(srcdir)/../gcc/c-family \ - -I $(srcdir)/../libcpp/include + -I $(srcdir)/../libcpp/include $(GMPINC) -WERROR_FLAG = -Werror -AM_CXXFLAGS = $(WARN_FLAGS) $(WERROR_FLAG) $(visibility) +AM_CXXFLAGS = $(WARN_FLAGS) $(WERROR) $(visibility) # Can be simplified when libiberty becomes a normal convenience library. libiberty_normal = ../libiberty/libiberty.a libiberty_noasan = ../libiberty/noasan/libiberty.a