[2349] Ignore EXPECT_DEATH when run in valgrind

Valgrind complains (probably rightfully so) that memory is leaking in death-tests. This has been reported upstream, but is not addressed yet, and may not be in the near future, if at all (one response was don't use EXPECT_DEATH).

So I added a checker function, *if* valgrind headers are available, that returns true if the tests are run under valgrind. Death tests are now ignored if so.

Note that we should be very careful about using this; and only use it for things we are sure valgrind can't help, since it is possible to do things differently when using valgrind, which kind of defeats its purpose

configure.ac

@@ -1055,6 +1055,13 @@ AM_COND_IF([ENABLE_LOGGER_CHECKS], [AC_DEFINE([ENABLE_LOGGER_CHECKS], [1], [Chec
 AC_PATH_PROG(VALGRIND, valgrind, no)
 AM_CONDITIONAL(HAVE_VALGRIND, test "x$VALGRIND" != "xno")
 
+# Also check for valgrind headers
+# We could consider adding them to the source code tree, as this
+# is the encouraged method of using them; they are BSD-licensed.
+# However, until we find that this is a problem, we just use
+# the system-provided ones, if available
+AC_CHECK_HEADERS(valgrind/valgrind.h, [AC_DEFINE([HAVE_VALGRIND_HEADERS], [1], [Check valgrind headers])])
+
 found_valgrind="not found"
 if test "x$VALGRIND" != "xno"; then
    found_valgrind="found"
@@ -1206,6 +1213,7 @@ AC_CONFIG_FILES([Makefile
                  src/lib/resolve/tests/Makefile
                  src/lib/testutils/Makefile
                  src/lib/testutils/testdata/Makefile
+                 src/lib/testutils/valgrind/Makefile
                  src/lib/nsas/Makefile
                  src/lib/nsas/tests/Makefile
                  src/lib/cache/Makefile

src/lib/log/logger.h

@@ -33,9 +33,9 @@ namespace log {
 /// \page LoggingApi Logging API
 /// \section LoggingApiOverview Overview
 /// BIND 10 logging uses the concepts of the widely-used Java logging
-/// package log4j (http://logging.apache.log/log4j), albeit implemented 
+/// package log4j (http://logging.apache.log/log4j), albeit implemented
 /// in C++ using an open-source port.  Features of the system are:
-/// 
+///
 /// - Within the code objects - known as loggers - can be created and
 /// used to log messages.  These loggers have names; those with the
 /// same name share characteristics (such as output destination).
@@ -50,7 +50,7 @@ namespace log {
 /// message is logged, it is output only if it is logged at a level equal
 /// to the logger severity level or greater, e.g. if the logger's severity
 /// is WARN, only messages logged at WARN, ERROR or FATAL will be output.
-/// 
+///
 /// \section LoggingApiLoggerNames BIND 10 Logger Names
 /// Within BIND 10, the root logger root logger is given the name of the
 /// program (via the stand-alone function setRootLoggerName()). Other loggers
@@ -58,7 +58,7 @@ namespace log {
 /// This name appears in logging output, allowing users to identify both
 /// the BIND 10 program and the component within the program that generated
 /// the message.
-/// 
+///
 /// When creating a logger, the abbreviated name "<sublogger>" can be used;
 /// the program name will be prepended to it when the logger is created.
 /// In this way, individual libraries can have their own loggers without
@@ -66,7 +66,7 @@ namespace log {
 /// - The origin of the message will be clearly identified.
 /// - The same component can have different options (e.g. logging severity)
 /// in different programs at the same time.
-/// 
+///
 /// \section LoggingApiLoggingMessages Logging Messages
 /// Instead of embedding the text of messages within the code, each message
 /// is referred to using a symbolic name.  The logging code uses this name as

src/lib/log/tests/log_formatter_unittest.cc

@@ -16,6 +16,7 @@
 #include <gtest/gtest.h>
 
 #include <util/unittests/resource.h>
+#include <util/unittests/check_valgrind.h>
 
 #include <log/log_formatter.h>
 #include <log/logger_level.h>
@@ -119,12 +120,13 @@ TEST_F(FormatterTest, mismatchedPlaceholders) {
     // Likewise, if there's a redundant placeholder (or missing argument), the
     // check detects it and aborts the program.  Due to the restriction of the
     // current implementation, it doesn't throw.
-    EXPECT_DEATH({
-        isc::util::unittests::dontCreateCoreDumps();
-        Formatter(isc::log::INFO, s("Too many arguments in %1 %2"), this).
-            arg("only one");
-    }, ".*");
-
+    if (!isc::util::unittests::runningOnValgrind()) {
+        EXPECT_DEATH({
+            isc::util::unittests::dontCreateCoreDumps();
+            Formatter(isc::log::INFO, s("Too many arguments in %1 %2"), this).
+                arg("only one");
+        }, ".*");
+    }
 #endif /* EXPECT_DEATH */
     // Mixed case of above two: the exception will be thrown due to the missing
     // placeholder. The other check is disabled due to that.

src/lib/log/tests/logger_unittest.cc

@@ -11,13 +11,10 @@
 // LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
-
-#include <iostream>
-#include <string>
-
 #include <gtest/gtest.h>
 
 #include <util/unittests/resource.h>
+#include <util/unittests/check_valgrind.h>
 
 #include <log/logger.h>
 #include <log/logger_manager.h>
@@ -27,6 +24,9 @@
 
 #include <util/interprocess_sync_file.h>
 
+#include <iostream>
+#include <string>
+
 using namespace isc;
 using namespace isc::log;
 using namespace std;
@@ -356,7 +356,6 @@ TEST_F(LoggerTest, IsDebugEnabledLevel) {
 
 // Check that if a logger name is too long, it triggers the appropriate
 // assertion.
-
 TEST_F(LoggerTest, LoggerNameLength) {
     // The following statements should just declare a logger and nothing
     // should happen.
@@ -374,12 +373,14 @@ TEST_F(LoggerTest, LoggerNameLength) {
     // Too long a logger name should trigger an assertion failure.
     // Note that we just check that it dies - we don't check what message is
     // output.
-    EXPECT_DEATH({
-        isc::util::unittests::dontCreateCoreDumps();
+    if (!isc::util::unittests::runningOnValgrind()) {
+        EXPECT_DEATH({
+            isc::util::unittests::dontCreateCoreDumps();
 
-        string ok3(Logger::MAX_LOGGER_NAME_SIZE + 1, 'x');
-        Logger l3(ok3.c_str());
-    }, ".*");
+            string ok3(Logger::MAX_LOGGER_NAME_SIZE + 1, 'x');
+            Logger l3(ok3.c_str());
+        }, ".*");
+    }
 #endif
 }
 

src/lib/log/tests/message_initializer_2_unittest.cc

@@ -16,6 +16,7 @@
 #include <gtest/gtest.h>
 
 #include <util/unittests/resource.h>
+#include <util/unittests/check_valgrind.h>
 
 using namespace isc::log;
 
@@ -43,10 +44,12 @@ TEST(MessageInitializerTest2, MessageLoadTest) {
     // test for its presence and bypass the test if not available.
 #ifdef EXPECT_DEATH
     // Adding one more should take us over the limit.
-    EXPECT_DEATH({
-        isc::util::unittests::dontCreateCoreDumps();
+    if (!isc::util::unittests::runningOnValgrind()) {
+        EXPECT_DEATH({
+            isc::util::unittests::dontCreateCoreDumps();
 
-        MessageInitializer initializer2(values);
-      }, ".*");
+            MessageInitializer initializer2(values);
+          }, ".*");
+    }
 #endif
 }

src/lib/server_common/tests/portconfig_unittest.cc

@@ -18,6 +18,7 @@
 #include <testutils/socket_request.h>
 #include <testutils/mockups.h>
 
+#include <util/unittests/check_valgrind.h>
 #include <util/unittests/resource.h>
 
 #include <cc/data.h>
@@ -311,44 +312,48 @@ typedef InstallListenAddresses InstallListenAddressesDeathTest;
 // We make the socket requestor throw a "fatal" exception, one where we can't be
 // sure the state between processes is consistent. So we abort in that case.
 TEST_F(InstallListenAddressesDeathTest, inconsistent) {
-    AddressList deathAddresses;
-    deathAddresses.push_back(AddressPair("192.0.2.3", 5288));
-    // Make sure it actually kills the application (there should be an abort
-    // in this case)
-    EXPECT_DEATH({
-        isc::util::unittests::dontCreateCoreDumps();
+    if (!isc::util::unittests::runningOnValgrind()) {
+        AddressList deathAddresses;
+        deathAddresses.push_back(AddressPair("192.0.2.3", 5288));
+        // Make sure it actually kills the application (there should be an abort
+        // in this case)
+        EXPECT_DEATH({
+            isc::util::unittests::dontCreateCoreDumps();
 
-        try {
-          installListenAddresses(deathAddresses, store_, dnss_);
-        } catch (...) {
-          // Prevent exceptions killing the application, we need
-          // to make sure it dies the real hard way
-        };
-      }, "");
+            try {
+              installListenAddresses(deathAddresses, store_, dnss_);
+            } catch (...) {
+              // Prevent exceptions killing the application, we need
+              // to make sure it dies the real hard way
+            };
+          }, "");
+    }
 }
 
 // If we are unable to tell the boss we closed a socket, we abort, as we are
 // not consistent with the boss most probably.
 TEST_F(InstallListenAddressesDeathTest, cantClose) {
-    installListenAddresses(valid_, store_, dnss_);
-    AddressList empty;
-    // Instruct it to fail on close
-    sock_requestor_.break_release_ = true;
-    EXPECT_DEATH({
-        isc::util::unittests::dontCreateCoreDumps();
+    if (!isc::util::unittests::runningOnValgrind()) {
+        installListenAddresses(valid_, store_, dnss_);
+        AddressList empty;
+        // Instruct it to fail on close
+        sock_requestor_.break_release_ = true;
+        EXPECT_DEATH({
+            isc::util::unittests::dontCreateCoreDumps();
 
-        try {
-          // Setting to empty will close all current sockets.
-          // And thanks to the break_release_, the close will
-          // throw, which will make it crash.
-          installListenAddresses(empty, store_, dnss_);
-        } catch (...) {
-          // To make sure it is killed by abort, not by some
-          // (unhandled) exception
-        };
-      }, "");
-    // And reset it back, so it can safely clean up itself.
-    sock_requestor_.break_release_ = false;
+            try {
+              // Setting to empty will close all current sockets.
+              // And thanks to the break_release_, the close will
+              // throw, which will make it crash.
+              installListenAddresses(empty, store_, dnss_);
+            } catch (...) {
+              // To make sure it is killed by abort, not by some
+              // (unhandled) exception
+            };
+          }, "");
+        // And reset it back, so it can safely clean up itself.
+        sock_requestor_.break_release_ = false;
+    }
 }
 #endif // EXPECT_DEATH
 

src/lib/testutils/Makefile.am

@@ -1,4 +1,4 @@
-SUBDIRS = . testdata
+SUBDIRS = . testdata valgrind
 
 AM_CPPFLAGS = -I$(top_srcdir)/src/lib -I$(top_builddir)/src/lib
 AM_CPPFLAGS += $(BOOST_INCLUDES)

src/lib/util/tests/buffer_unittest.cc

@@ -18,6 +18,7 @@
 
 #ifdef EXPECT_DEATH
 #include <util/unittests/resource.h>
+#include <util/unittests/check_valgrind.h>
 #endif /* EXPECT_DEATH */
 
 #include <util/buffer.h>
@@ -188,16 +189,18 @@ TEST_F(BufferTest, outputBufferReadat) {
     }
 #ifdef EXPECT_DEATH
     // We use assert now, so we check it dies
-    EXPECT_DEATH({
-        isc::util::unittests::dontCreateCoreDumps();
-
-        try {
-            obuffer[sizeof(testdata)];
-        } catch (...) {
-            // Prevent exceptions killing the application, we need
-            // to make sure it dies the real hard way
-        }
-        }, "");
+    if (!isc::util::unittests::runningOnValgrind()) {
+        EXPECT_DEATH({
+            isc::util::unittests::dontCreateCoreDumps();
+
+            try {
+                obuffer[sizeof(testdata)];
+            } catch (...) {
+                // Prevent exceptions killing the application, we need
+                // to make sure it dies the real hard way
+            }
+            }, "");
+    }
 #endif
 }
 

src/lib/util/threads/tests/lock_unittest.cc

@@ -12,10 +12,11 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
+#include <gtest/gtest.h>
+
 #include <util/threads/lock.h>
 #include <util/threads/thread.h>
-
-#include <gtest/gtest.h>
+#include <util/unittests/check_valgrind.h>
 
 #include <boost/bind.hpp>
 #include <unistd.h>
@@ -42,13 +43,15 @@ TEST(MutexTest, lockMultiple) {
 // Destroying a locked mutex is a bad idea as well
 #ifdef EXPECT_DEATH
 TEST(MutexTest, destroyLocked) {
-    EXPECT_DEATH({
-        Mutex* mutex = new Mutex;
-        new Mutex::Locker(*mutex);
-        delete mutex;
-        // This'll leak the locker, but inside the slave process, it should
-        // not be an issue.
-    }, "");
+    if (!isc::util::unittests::runningOnValgrind()) {
+        EXPECT_DEATH({
+            Mutex* mutex = new Mutex;
+            new Mutex::Locker(*mutex);
+            delete mutex;
+            // This'll leak the locker, but inside the slave process, it should
+            // not be an issue.
+        }, "");
+    }
 }
 #endif
 

src/lib/util/unittests/Makefile.am

@@ -7,6 +7,7 @@ libutil_unittests_la_SOURCES += newhook.h newhook.cc
 libutil_unittests_la_SOURCES += testdata.h testdata.cc
 if HAVE_GTEST
 libutil_unittests_la_SOURCES += resource.h resource.cc
+libutil_unittests_la_SOURCES += check_valgrind.h check_valgrind.cc
 libutil_unittests_la_SOURCES += run_all.h run_all.cc
 libutil_unittests_la_SOURCES += textdata.h
 libutil_unittests_la_SOURCES += wiredata.h wiredata.cc

src/lib/util/unittests/check_valgrind.cc

@@ -0,0 +1,41 @@
+// Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#include <config.h>
+
+namespace isc {
+namespace util {
+namespace unittests {
+
+#if HAVE_VALGRIND_HEADERS
+#include <valgrind/valgrind.h>
+/// \brief Check if the program is run in valgrind
+///
+/// \return true if valgrind headers are available, and valgrind is running,
+///         false if the headers are not available, or if valgrind is not
+///         running
+bool
+runningOnValgrind() {
+    return (RUNNING_ON_VALGRIND != 0);
+}
+#else
+bool
+runningOnValgrind() {
+    return (false);
+}
+#endif // HAVE_VALGRIND_HEADERS
+
+} // end of namespace unittests
+} // end of namespace util
+} // end of namespace isc

src/lib/util/unittests/check_valgrind.h

@@ -0,0 +1,50 @@
+// Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+//
+// If we have the valgrind headers available, we can detect whether
+// valgrind is running. This should normally never be done, as you
+// want the to test the actual code in operation with valgrind.
+//
+// However, there is a limited set of operations where we want to
+// skip some tests if run under valgrind, most notably the
+// EXPECT_DEATH tests, as these would report memory leaks by
+// definition.
+//
+// If the valgrind headers are NOT available, the method checkValgrind()
+// always returns false; i.e. it always pretends the program is run
+// natively
+//
+
+#ifndef __UTIL_UNITTESTS_CHECK_VALGRIND_H
+#define __UTIL_UNITTESTS_CHECK_VALGRIND_H 1
+
+#include <config.h>
+
+namespace isc {
+namespace util {
+namespace unittests {
+
+/// \brief Check if the program is run in valgrind
+///
+/// \return true if valgrind headers are available, and valgrind is running,
+///         false if the headers are not available, or if valgrind is not
+///         running
+bool runningOnValgrind();
+
+} // end namespace unittests
+} // end namespace util
+} // end namespace isc
+
+#endif // __UTIL_UNITTESTS_CHECK_VALGRIND_H