]> git.ipfire.org Git - thirdparty/kea.git/commitdiff
[2349] Ignore EXPECT_DEATH when run in valgrind
authorJelte Jansen <jelte@isc.org>
Thu, 11 Oct 2012 15:22:23 +0000 (17:22 +0200)
committerJelte Jansen <jelte@isc.org>
Thu, 11 Oct 2012 15:22:23 +0000 (17:22 +0200)
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

12 files changed:
configure.ac
src/lib/log/logger.h
src/lib/log/tests/log_formatter_unittest.cc
src/lib/log/tests/logger_unittest.cc
src/lib/log/tests/message_initializer_2_unittest.cc
src/lib/server_common/tests/portconfig_unittest.cc
src/lib/testutils/Makefile.am
src/lib/util/tests/buffer_unittest.cc
src/lib/util/threads/tests/lock_unittest.cc
src/lib/util/unittests/Makefile.am
src/lib/util/unittests/check_valgrind.cc [new file with mode: 0644]
src/lib/util/unittests/check_valgrind.h [new file with mode: 0644]

index 9fb813c28b4f847e3314af1ee2a8078f8a74104a..df579438d635ccc90086582e0a498257a9c052a3 100644 (file)
@@ -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
index 6405488b47b14befc79e9ef22f2794c5e6bf552e..4291eaec495a591e99eb428c7162b13cfffcbf72 100644 (file)
@@ -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
index 435b20097954d12ba03e6c895f2da8fd5eacbd71..f0a9a594e79d19ce530e34830191eb6d55e49103 100644 (file)
@@ -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.
index a9330a9a647afd146d0599a74e920a335e82609d..7b62d7923a6fabe1358c2cf51c7eb86e59894ce2 100644 (file)
 // 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
 }
 
index b479eeed7c6059a12f6cf38f93a186843b981b1e..8cc78fade1a82e812e2ef4f84e145aa26d33f8ba 100644 (file)
@@ -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
 }
index ac880c0747fdeafc681962c166a22fb1888156eb..0c971eeb33c96a9d907f3b910e581628a0e955e4 100644 (file)
@@ -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
 
index 2281b6d7003da70ffd188e6b2b815b91c53a1e4c..dc275b047bf5d8505f8b2e0ac8f16f431953ebb8 100644 (file)
@@ -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)
index 9af3d57c6eaa0b2017f22b084fe24ab40e2b66a8..02ca83d80382aa6be4c74078f167f99287785d46 100644 (file)
@@ -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
 }
 
index 0b4d3ce4ecffcac34a791e0e5e1e18daef2487bd..c6e2e1939dc54f1ccb394abb7e9ad40d3074702e 100644 (file)
 // 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
 
index 451ab4e34cca95f46e1811861269758a21c5c1c2..55e0372977809ffe7dcd428d37bae0c87da25a9e 100644 (file)
@@ -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
diff --git a/src/lib/util/unittests/check_valgrind.cc b/src/lib/util/unittests/check_valgrind.cc
new file mode 100644 (file)
index 0000000..2951d25
--- /dev/null
@@ -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
diff --git a/src/lib/util/unittests/check_valgrind.h b/src/lib/util/unittests/check_valgrind.h
new file mode 100644 (file)
index 0000000..c08b58b
--- /dev/null
@@ -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