|
From: <sv...@va...> - 2009-12-29 16:56:32
|
Author: bart
Date: 2009-12-29 16:56:18 +0000 (Tue, 29 Dec 2009)
New Revision: 10972
Log:
Removed dependency of include/pub_tool_basics.h on config.h.
Modified:
trunk/configure.in
trunk/coregrind/m_aspacemgr/aspacemgr-common.c
trunk/coregrind/m_aspacemgr/aspacemgr-linux.c
trunk/coregrind/m_syswrap/syswrap-generic.c
trunk/helgrind/hg_intercepts.c
trunk/include/pub_tool_basics.h
Modified: trunk/configure.in
===================================================================
--- trunk/configure.in 2009-12-29 15:08:14 UTC (rev 10971)
+++ trunk/configure.in 2009-12-29 16:56:18 UTC (rev 10972)
@@ -1378,24 +1378,6 @@
CFLAGS=$safe_CFLAGS
-# does this compiler support __builtin_expect?
-AC_MSG_CHECKING([if gcc supports __builtin_expect])
-
-AC_TRY_LINK(, [
-return __builtin_expect(1, 1) ? 1 : 0
-],
-[
-ac_have_builtin_expect=yes
-AC_MSG_RESULT([yes])
-], [
-ac_have_builtin_expect=no
-AC_MSG_RESULT([no])
-])
-if test x$ac_have_builtin_expect = xyes ; then
- AC_DEFINE(HAVE_BUILTIN_EXPECT, 1, [Define to 1 if gcc supports __builtin_expect.])
-fi
-
-
# does the ppc assembler support "mtocrf" et al?
AC_MSG_CHECKING([if ppc32/64 as supports mtocrf/mfocrf])
Modified: trunk/coregrind/m_aspacemgr/aspacemgr-common.c
===================================================================
--- trunk/coregrind/m_aspacemgr/aspacemgr-common.c 2009-12-29 15:08:14 UTC (rev 10971)
+++ trunk/coregrind/m_aspacemgr/aspacemgr-common.c 2009-12-29 16:56:18 UTC (rev 10972)
@@ -37,6 +37,7 @@
************************************************************* */
#include "priv_aspacemgr.h"
+#include "config.h"
/*-----------------------------------------------------------------*/
Modified: trunk/coregrind/m_aspacemgr/aspacemgr-linux.c
===================================================================
--- trunk/coregrind/m_aspacemgr/aspacemgr-linux.c 2009-12-29 15:08:14 UTC (rev 10971)
+++ trunk/coregrind/m_aspacemgr/aspacemgr-linux.c 2009-12-29 16:56:18 UTC (rev 10972)
@@ -40,6 +40,7 @@
************************************************************* */
#include "priv_aspacemgr.h"
+#include "config.h"
/* Note: many of the exported functions implemented below are
Modified: trunk/coregrind/m_syswrap/syswrap-generic.c
===================================================================
--- trunk/coregrind/m_syswrap/syswrap-generic.c 2009-12-29 15:08:14 UTC (rev 10971)
+++ trunk/coregrind/m_syswrap/syswrap-generic.c 2009-12-29 16:56:18 UTC (rev 10972)
@@ -62,7 +62,9 @@
#include "priv_types_n_macros.h"
#include "priv_syswrap-generic.h"
+#include "config.h"
+
/* Returns True iff address range is something the client can
plausibly mess with: all of it is either already belongs to the
client or is free or a reservation. */
Modified: trunk/helgrind/hg_intercepts.c
===================================================================
--- trunk/helgrind/hg_intercepts.c 2009-12-29 15:08:14 UTC (rev 10971)
+++ trunk/helgrind/hg_intercepts.c 2009-12-29 16:56:18 UTC (rev 10972)
@@ -56,6 +56,7 @@
#include "pub_tool_redir.h"
#include "valgrind.h"
#include "helgrind.h"
+#include "config.h"
#define TRACE_PTH_FNS 0
#define TRACE_QT4_FNS 0
Modified: trunk/include/pub_tool_basics.h
===================================================================
--- trunk/include/pub_tool_basics.h 2009-12-29 15:08:14 UTC (rev 10971)
+++ trunk/include/pub_tool_basics.h 2009-12-29 16:56:18 UTC (rev 10972)
@@ -49,10 +49,7 @@
// For varargs types
#include <stdarg.h>
-/* For HAVE_BUILTIN_EXPECT */
-#include "config.h"
-
/* ---------------------------------------------------------------------
symbol prefixing
------------------------------------------------------------------ */
@@ -318,7 +315,7 @@
#define VG_BUGS_TO "www.valgrind.org"
/* Branch prediction hints. */
-#if HAVE_BUILTIN_EXPECT
+#if 1 /*HAVE_BUILTIN_EXPECT*/
# define LIKELY(x) __builtin_expect(!!(x), 1)
# define UNLIKELY(x) __builtin_expect((x), 0)
#else
|
|
From: Julian S. <js...@ac...> - 2009-12-31 13:06:59
|
I must say I don't think this is a good idea. For one thing it now requires us to #include config.h everywhere we need it, whereas before, inclusion of pub_tool_basics.h was enough. This for example causes Memcheck to not start up on openSUSE 11.0 x86_64, since the GLIBC_2_x symbols are no longer visible in m_redir.c. More seriously, it means that the installation tree (still) lacks a config.h which matches/describes how the installed libraries and executables were built. Perhaps a better solution would be to revert this and to install config.h too. J On Tuesday 29 December 2009, sv...@va... wrote: > Author: bart > Date: 2009-12-29 16:56:18 +0000 (Tue, 29 Dec 2009) > New Revision: 10972 > > Log: > Removed dependency of include/pub_tool_basics.h on config.h. > > > Modified: > trunk/configure.in > trunk/coregrind/m_aspacemgr/aspacemgr-common.c > trunk/coregrind/m_aspacemgr/aspacemgr-linux.c > trunk/coregrind/m_syswrap/syswrap-generic.c > trunk/helgrind/hg_intercepts.c > trunk/include/pub_tool_basics.h > > > Modified: trunk/configure.in > =================================================================== > --- trunk/configure.in 2009-12-29 15:08:14 UTC (rev 10971) > +++ trunk/configure.in 2009-12-29 16:56:18 UTC (rev 10972) > @@ -1378,24 +1378,6 @@ > CFLAGS=$safe_CFLAGS > > > -# does this compiler support __builtin_expect? > -AC_MSG_CHECKING([if gcc supports __builtin_expect]) > - > -AC_TRY_LINK(, [ > -return __builtin_expect(1, 1) ? 1 : 0 > -], > -[ > -ac_have_builtin_expect=yes > -AC_MSG_RESULT([yes]) > -], [ > -ac_have_builtin_expect=no > -AC_MSG_RESULT([no]) > -]) > -if test x$ac_have_builtin_expect = xyes ; then > - AC_DEFINE(HAVE_BUILTIN_EXPECT, 1, [Define to 1 if gcc supports > __builtin_expect.]) -fi > - > - > # does the ppc assembler support "mtocrf" et al? > AC_MSG_CHECKING([if ppc32/64 as supports mtocrf/mfocrf]) > > > Modified: trunk/coregrind/m_aspacemgr/aspacemgr-common.c > =================================================================== > --- trunk/coregrind/m_aspacemgr/aspacemgr-common.c 2009-12-29 15:08:14 UTC > (rev 10971) +++ trunk/coregrind/m_aspacemgr/aspacemgr-common.c 2009-12-29 > 16:56:18 UTC (rev 10972) @@ -37,6 +37,7 @@ > ************************************************************* */ > > #include "priv_aspacemgr.h" > +#include "config.h" > > > /*-----------------------------------------------------------------*/ > > Modified: trunk/coregrind/m_aspacemgr/aspacemgr-linux.c > =================================================================== > --- trunk/coregrind/m_aspacemgr/aspacemgr-linux.c 2009-12-29 15:08:14 UTC > (rev 10971) +++ trunk/coregrind/m_aspacemgr/aspacemgr-linux.c 2009-12-29 > 16:56:18 UTC (rev 10972) @@ -40,6 +40,7 @@ > ************************************************************* */ > > #include "priv_aspacemgr.h" > +#include "config.h" > > > /* Note: many of the exported functions implemented below are > > Modified: trunk/coregrind/m_syswrap/syswrap-generic.c > =================================================================== > --- trunk/coregrind/m_syswrap/syswrap-generic.c 2009-12-29 15:08:14 UTC > (rev 10971) +++ trunk/coregrind/m_syswrap/syswrap-generic.c 2009-12-29 > 16:56:18 UTC (rev 10972) @@ -62,7 +62,9 @@ > #include "priv_types_n_macros.h" > #include "priv_syswrap-generic.h" > > +#include "config.h" > > + > /* Returns True iff address range is something the client can > plausibly mess with: all of it is either already belongs to the > client or is free or a reservation. */ > > Modified: trunk/helgrind/hg_intercepts.c > =================================================================== > --- trunk/helgrind/hg_intercepts.c 2009-12-29 15:08:14 UTC (rev 10971) > +++ trunk/helgrind/hg_intercepts.c 2009-12-29 16:56:18 UTC (rev 10972) > @@ -56,6 +56,7 @@ > #include "pub_tool_redir.h" > #include "valgrind.h" > #include "helgrind.h" > +#include "config.h" > > #define TRACE_PTH_FNS 0 > #define TRACE_QT4_FNS 0 > > Modified: trunk/include/pub_tool_basics.h > =================================================================== > --- trunk/include/pub_tool_basics.h 2009-12-29 15:08:14 UTC (rev 10971) > +++ trunk/include/pub_tool_basics.h 2009-12-29 16:56:18 UTC (rev 10972) > @@ -49,10 +49,7 @@ > // For varargs types > #include <stdarg.h> > > -/* For HAVE_BUILTIN_EXPECT */ > -#include "config.h" > > - > /* --------------------------------------------------------------------- > symbol prefixing > ------------------------------------------------------------------ */ > @@ -318,7 +315,7 @@ > #define VG_BUGS_TO "www.valgrind.org" > > /* Branch prediction hints. */ > -#if HAVE_BUILTIN_EXPECT > +#if 1 /*HAVE_BUILTIN_EXPECT*/ > # define LIKELY(x) __builtin_expect(!!(x), 1) > # define UNLIKELY(x) __builtin_expect((x), 0) > #else > > > --------------------------------------------------------------------------- >--- This SF.Net email is sponsored by the Verizon Developer Community > Take advantage of Verizon's best-in-class app development support > A streamlined, 14 day to market process makes app distribution fast and > easy Join now and get one step closer to millions of Verizon customers > http://p.sf.net/sfu/verizon-dev2dev > _______________________________________________ > Valgrind-developers mailing list > Val...@li... > https://lists.sourceforge.net/lists/listinfo/valgrind-developers |
|
From: Bart V. A. <bar...@gm...> - 2009-12-31 13:58:15
|
On Thu, Dec 31, 2009 at 1:38 PM, Julian Seward <js...@ac...> wrote: > > I must say I don't think this is a good idea. For one thing > it now requires us to #include config.h everywhere we need it, > whereas before, inclusion of pub_tool_basics.h was enough. > This for example causes Memcheck to not start up on openSUSE > 11.0 x86_64, since the GLIBC_2_x symbols are no longer visible > in m_redir.c. > > More seriously, it means that the installation tree (still) > lacks a config.h which matches/describes how the installed > libraries and executables were built. Perhaps a better solution > would be to revert this and to install config.h too. Sorry Julian, but I disagree completely with the above. Not including the header file "config.h" when symbols defined in that header file are necessary is a bad programming practice in my opinion. Also, including "config.h" from a public header file like pub_tool_basics.h is completely wrong because this makes it impossible for external projects that include the Valgrind public header files to have a header file with the same name, namely "config.h". Furthermore, exposing all symbols defined in Valgrind's "config.h" to external projects would create a high probability of symbol collisions / redefinitions, e.g. the HAVE_* macro's. If we really have to expose some of the symbols defined during Valgrind's configure process, we should do like Stefan Kost proposed and let the configure process transform a file valgrind_config.h.in to valgrind_config.h. That would allow us to export a minimum set of symbols from Valgrind to external projects instead of all symbols defined during the configure step. Bart. |
|
From: Dave G. <go...@mc...> - 2010-01-04 17:31:29
|
On Dec 31, 2009, at 7:32 AM, Bart Van Assche wrote: > On Thu, Dec 31, 2009 at 1:38 PM, Julian Seward <js...@ac...> > wrote: >> >> More seriously, it means that the installation tree (still) >> lacks a config.h which matches/describes how the installed >> libraries and executables were built. Perhaps a better solution >> would be to revert this and to install config.h too. > > Sorry Julian, but I disagree completely with the above. ... > Furthermore, exposing all symbols defined in Valgrind's "config.h" to > external projects would create a high probability of symbol collisions > / redefinitions, e.g. the HAVE_* macro's. The HAVE_* macros are usually the least of your problems, because they _usually_ are redefined to identical values, which is OK by the preprocessor. The most common show stoppers are PACKAGE, VERSION, etc that are practically guaranteed to have different values between two projects. The unmodified config.h is definitely not installable. > If we really have to expose some of the symbols defined during > Valgrind's configure process, we should do like Stefan Kost proposed > and let the configure process transform a file valgrind_config.h.in to > valgrind_config.h. That would allow us to export a minimum set of > symbols from Valgrind to external projects instead of all symbols > defined during the configure step. You may already be aware of this, but the AX_PREFIX_CONFIG_H macro was written so that you could safely install a namespaced version of the config.h if you need to: http://www.nongnu.org/autoconf-archive/ax_prefix_config_h.html It's a pretty heavy-handed solution though, and it might be worth the additional effort to generate a header containing only the minimum set of symbols instead. Which one makes more sense will depend on how large that minimal set is and how often it changes. -Dave |
|
From: Greg P. <gp...@ap...> - 2010-01-05 02:13:44
|
On Jan 4, 2010, at 9:31 AM, Dave Goodell wrote: > On Dec 31, 2009, at 7:32 AM, Bart Van Assche wrote: >> On Thu, Dec 31, 2009 at 1:38 PM, Julian Seward <js...@ac...> >> wrote: >>> More seriously, it means that the installation tree (still) >>> lacks a config.h which matches/describes how the installed >>> libraries and executables were built. Perhaps a better solution >>> would be to revert this and to install config.h too. >> >> Sorry Julian, but I disagree completely with the above. > ... >> Furthermore, exposing all symbols defined in Valgrind's "config.h" to >> external projects would create a high probability of symbol collisions >> / redefinitions, e.g. the HAVE_* macro's. > > The HAVE_* macros are usually the least of your problems, because they > _usually_ are redefined to identical values, which is OK by the > preprocessor. The most common show stoppers are PACKAGE, VERSION, etc > that are practically guaranteed to have different values between two > projects. The unmodified config.h is definitely not installable. Another problem with saving config.h: on platforms that support multiple CPU architectures on a single machine, there is no single config.h that will be correct for every architecture, even if you limit it to a single project. For example, if config.h records the pointer size then it can't work for both 32-bit and 64-bit builds. Mac OS X has lots of different architectures available on a single machine. autoconf-style projects are reconfigured and rebuilt separately for each architecture, and then the products of all builds are combined into a single universal binary. That way each architecture's build gets its own config.h. -- Greg Parker gp...@ap... Runtime Wrangler |
|
From: Julian S. <js...@ac...> - 2010-01-05 09:26:48
|
On Tuesday 05 January 2010, Greg Parker wrote: > Another problem with saving config.h: on platforms that support multiple > CPU architectures on a single machine, there is no single config.h that > will be correct for every architecture, [...] Yes .. evidently my initial proposal was ill thought out. I retract it. J |