diff mbox

Fix --enable-checking of libcpp, add valgrind workarounds

Message ID 20130227160843.GN12913@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Feb. 27, 2013, 4:08 p.m. UTC
Hi!

I've noticed that libcpp/ uses --enable-checking in configure in incompatible
way from gcc/, as the configure options are passed to both, we'd better make
them compatible.  In particular, libcpp would be built with checking even
e.g. when configured with --enable-checking=release, on the other side
wouldn't do any checking when configured without anything but DEV-PHASE is
experimental.

Fixed thusly, so that libcpp checking is enabled any time gcc
ENABLE_CHECKING (aka misc checking) is enabled.  Additionally I've added
handling for valgrind checking, which doesn't like memory pointed just
through interior pointers and complains about that.  So, when configured
e.g. with --enable-checking=yes,valgrind or similar, libcpp will put the
_cpp_buff structure at the start rather than end of the allocated area and
valgrind won't complain in that case.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2013-02-27  Jakub Jelinek  <jakub@redhat.com>

	* configure.ac: Don't define ENABLE_CHECKING whenever
	--enable-checking is seen, instead use similar --enable-checking=yes
	vs. --enable-checking=release default as gcc/ subdir has and
	define ENABLE_CHECKING if ENABLE_CHECKING is defined in gcc/.
	Define ENABLE_VALGRIND_CHECKING if requested.
	* lex.c (new_buff): If ENABLE_VALGRIND_CHECKING, put _cpp_buff
	struct first in the allocated buffer and result->base after it.
	(_cpp_free_buff): If ENABLE_VALGRIND_CHECKING, free buff itself
	instead of buff->base.
	* config.in: Regenerated.
	* configure: Regenerated.


	Jakub

Comments

Jeff Law Feb. 27, 2013, 10:06 p.m. UTC | #1
On 02/27/2013 09:08 AM, Jakub Jelinek wrote:
> Hi!
>
> I've noticed that libcpp/ uses --enable-checking in configure in incompatible
> way from gcc/, as the configure options are passed to both, we'd better make
> them compatible.  In particular, libcpp would be built with checking even
> e.g. when configured with --enable-checking=release, on the other side
> wouldn't do any checking when configured without anything but DEV-PHASE is
> experimental.
>
> Fixed thusly, so that libcpp checking is enabled any time gcc
> ENABLE_CHECKING (aka misc checking) is enabled.  Additionally I've added
> handling for valgrind checking, which doesn't like memory pointed just
> through interior pointers and complains about that.  So, when configured
> e.g. with --enable-checking=yes,valgrind or similar, libcpp will put the
> _cpp_buff structure at the start rather than end of the allocated area and
> valgrind won't complain in that case.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2013-02-27  Jakub Jelinek  <jakub@redhat.com>
>
> 	* configure.ac: Don't define ENABLE_CHECKING whenever
> 	--enable-checking is seen, instead use similar --enable-checking=yes
> 	vs. --enable-checking=release default as gcc/ subdir has and
> 	define ENABLE_CHECKING if ENABLE_CHECKING is defined in gcc/.
> 	Define ENABLE_VALGRIND_CHECKING if requested.
> 	* lex.c (new_buff): If ENABLE_VALGRIND_CHECKING, put _cpp_buff
> 	struct first in the allocated buffer and result->base after it.
> 	(_cpp_free_buff): If ENABLE_VALGRIND_CHECKING, free buff itself
> 	instead of buff->base.
> 	* config.in: Regenerated.
> 	* configure: Regenerated.
Presumably there's a good reason why we don't put __cpp_buff at the 
start of the structure all the time?  From a purely maintenance 
standpoint that seems better

Approved as is.  If you want to move cpp_buff at the start of the 
structure, I'll pre-approve that as well.

jeff
Jakub Jelinek Feb. 27, 2013, 10:16 p.m. UTC | #2
On Wed, Feb 27, 2013 at 03:06:03PM -0700, Jeff Law wrote:
> Presumably there's a good reason why we don't put __cpp_buff at the
> start of the structure all the time?  From a purely maintenance
> standpoint that seems better

I think there are two reasons, one is mentioned in the function comment
(already from 3.2 or older era):
/* Create a new allocation buffer.  Place the control block at the end
   of the buffer, so that buffer overflows will cause immediate chaos.  */

and the other reason I'd say is that by putting the buffer first we increase
the chances the actual buffer is more aligned (especially for very large
allocations for which e.g. glibc malloc would use mmap).  If it were
affecting more than two functions, I'd understand the maintainance burden
side, but as it is two tiny spots and valgrind checking is something people
normally do just a couple of times per release cycle (and as my current
--leak-check=full attempts show, perhaps even less than that, the amount of
still unfixed issues is huge), I think it is fine as is.

	Jakub
diff mbox

Patch

--- libcpp/configure.ac.jj	2012-11-16 23:02:58.000000000 +0100
+++ libcpp/configure.ac	2013-02-27 12:54:47.033401679 +0100
@@ -123,15 +123,53 @@  else
 fi
 AC_SUBST(MAINT)
 
-AC_ARG_ENABLE(checking,
-[  --enable-checking      enable expensive run-time checks],,
-enable_checking=no)
+# Enable expensive internal checks
+is_release=
+if test -f $srcdir/../gcc/DEV-PHASE \
+   && test x"`cat $srcdir/../gcc/DEV-PHASE`" != xexperimental; then
+  is_release=yes
+fi
 
-if test $enable_checking != no ; then
+AC_ARG_ENABLE(checking,
+[AS_HELP_STRING([[--enable-checking[=LIST]]],
+		[enable expensive run-time checks.  With LIST,
+		 enable only specific categories of checks.
+		 Categories are: yes,no,all,none,release.
+		 Flags are: misc,valgrind or other strings])],
+[ac_checking_flags="${enableval}"],[
+# Determine the default checks.
+if test x$is_release = x ; then
+  ac_checking_flags=yes
+else
+  ac_checking_flags=release
+fi])
+IFS="${IFS= 	}"; ac_save_IFS="$IFS"; IFS="$IFS,"
+for check in release $ac_checking_flags
+do
+	case $check in
+	# these set all the flags to specific states
+	yes|all) ac_checking=1 ; ac_valgrind_checking= ;;
+	no|none|release) ac_checking= ; ac_valgrind_checking= ;;
+	# these enable particular checks
+	misc) ac_checking=1 ;;
+	valgrind) ac_valgrind_checking=1 ;;
+	# accept
+	*) ;;
+	esac
+done
+IFS="$ac_save_IFS"
+                
+if test x$ac_checking != x ; then
   AC_DEFINE(ENABLE_CHECKING, 1,
 [Define if you want more run-time sanity checks.])
 fi
 
+if test x$ac_valgrind_checking != x ; then
+  AC_DEFINE(ENABLE_VALGRIND_CHECKING, 1,
+[Define if you want to workaround valgrind (a memory checker) warnings about
+ possible memory leaks because of libcpp use of interior pointers.])
+fi
+
 AC_ARG_ENABLE(canonical-system-headers,
 [  --enable-canonical-system-headers
                           enable or disable system headers canonicalization],
--- libcpp/lex.c.jj	2013-02-27 13:56:59.000000000 +0100
+++ libcpp/lex.c	2013-02-27 14:04:53.153690020 +0100
@@ -2846,8 +2846,17 @@  new_buff (size_t len)
     len = MIN_BUFF_SIZE;
   len = CPP_ALIGN (len);
 
+#ifdef ENABLE_VALGRIND_CHECKING
+  /* Valgrind warns about uses of interior pointers, so put _cpp_buff
+     struct first.  */
+  size_t slen = CPP_ALIGN2 (sizeof (_cpp_buff), 2 * DEFAULT_ALIGNMENT);
+  base = XNEWVEC (unsigned char, len + slen);
+  result = (_cpp_buff *) base;
+  base += slen;
+#else
   base = XNEWVEC (unsigned char, len + sizeof (_cpp_buff));
   result = (_cpp_buff *) (base + len);
+#endif
   result->base = base;
   result->cur = base;
   result->limit = base + len;
@@ -2934,7 +2943,11 @@  _cpp_free_buff (_cpp_buff *buff)
   for (; buff; buff = next)
     {
       next = buff->next;
+#ifdef ENABLE_VALGRIND_CHECKING
+      free (buff);
+#else
       free (buff->base);
+#endif
     }
 }
 
--- libcpp/config.in.jj	2012-11-16 23:02:58.000000000 +0100
+++ libcpp/config.in	2013-02-27 12:55:30.000000000 +0100
@@ -21,6 +21,10 @@ 
    language is requested. */
 #undef ENABLE_NLS
 
+/* Define if you want to workaround valgrind (a memory checker) warnings about
+   possible memory leaks because of libcpp use of interior pointers. */
+#undef ENABLE_VALGRIND_CHECKING
+
 /* Define to 1 if you have `alloca', as a function or macro. */
 #undef HAVE_ALLOCA
 
--- libcpp/configure.jj	2012-11-16 23:02:58.000000000 +0100
+++ libcpp/configure	2013-02-27 12:55:27.923187370 +0100
@@ -1333,7 +1333,11 @@  Optional Features:
   --enable-werror-always  enable -Werror despite compiler version
   --disable-rpath         do not hardcode runtime library paths
   --enable-maintainer-mode enable rules only needed by maintainers
-  --enable-checking      enable expensive run-time checks
+  --enable-checking[=LIST]
+                          enable expensive run-time checks. With LIST, enable
+                          only specific categories of checks. Categories are:
+                          yes,no,all,none,release. Flags are: misc,valgrind or
+                          other strings
   --enable-canonical-system-headers
                           enable or disable system headers canonicalization
 
@@ -7083,20 +7087,54 @@  else
 fi
 
 
+# Enable expensive internal checks
+is_release=
+if test -f $srcdir/../gcc/DEV-PHASE \
+   && test x"`cat $srcdir/../gcc/DEV-PHASE`" != xexperimental; then
+  is_release=yes
+fi
+
 # Check whether --enable-checking was given.
 if test "${enable_checking+set}" = set; then :
-  enableval=$enable_checking;
+  enableval=$enable_checking; ac_checking_flags="${enableval}"
+else
+
+# Determine the default checks.
+if test x$is_release = x ; then
+  ac_checking_flags=yes
 else
-  enable_checking=no
+  ac_checking_flags=release
+fi
 fi
 
+IFS="${IFS= 	}"; ac_save_IFS="$IFS"; IFS="$IFS,"
+for check in release $ac_checking_flags
+do
+	case $check in
+	# these set all the flags to specific states
+	yes|all) ac_checking=1 ; ac_valgrind_checking= ;;
+	no|none|release) ac_checking= ; ac_valgrind_checking= ;;
+	# these enable particular checks
+	misc) ac_checking=1 ;;
+	valgrind) ac_valgrind_checking=1 ;;
+	# accept
+	*) ;;
+	esac
+done
+IFS="$ac_save_IFS"
 
-if test $enable_checking != no ; then
+if test x$ac_checking != x ; then
 
 $as_echo "#define ENABLE_CHECKING 1" >>confdefs.h
 
 fi
 
+if test x$ac_valgrind_checking != x ; then
+
+$as_echo "#define ENABLE_VALGRIND_CHECKING 1" >>confdefs.h
+
+fi
+
 # Check whether --enable-canonical-system-headers was given.
 if test "${enable_canonical_system_headers+set}" = set; then :
   enableval=$enable_canonical_system_headers;