From: Jelte Jansen Date: Thu, 11 Oct 2012 15:22:23 +0000 (+0200) Subject: [2349] Ignore EXPECT_DEATH when run in valgrind X-Git-Tag: trac2402_base~17^2~8 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=030aef19982a37a8b81376adb15864097b208ff8;p=thirdparty%2Fkea.git [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 --- diff --git a/configure.ac b/configure.ac index 9fb813c28b..df579438d6 100644 --- a/configure.ac +++ b/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 diff --git a/src/lib/log/logger.h b/src/lib/log/logger.h index 6405488b47..4291eaec49 100644 --- a/src/lib/log/logger.h +++ b/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 "" 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 diff --git a/src/lib/log/tests/log_formatter_unittest.cc b/src/lib/log/tests/log_formatter_unittest.cc index 435b200979..f0a9a594e7 100644 --- a/src/lib/log/tests/log_formatter_unittest.cc +++ b/src/lib/log/tests/log_formatter_unittest.cc @@ -16,6 +16,7 @@ #include #include +#include #include #include @@ -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. diff --git a/src/lib/log/tests/logger_unittest.cc b/src/lib/log/tests/logger_unittest.cc index a9330a9a64..7b62d7923a 100644 --- a/src/lib/log/tests/logger_unittest.cc +++ b/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 -#include - #include #include +#include #include #include @@ -27,6 +24,9 @@ #include +#include +#include + 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 } diff --git a/src/lib/log/tests/message_initializer_2_unittest.cc b/src/lib/log/tests/message_initializer_2_unittest.cc index b479eeed7c..8cc78fade1 100644 --- a/src/lib/log/tests/message_initializer_2_unittest.cc +++ b/src/lib/log/tests/message_initializer_2_unittest.cc @@ -16,6 +16,7 @@ #include #include +#include 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 } diff --git a/src/lib/server_common/tests/portconfig_unittest.cc b/src/lib/server_common/tests/portconfig_unittest.cc index ac880c0747..0c971eeb33 100644 --- a/src/lib/server_common/tests/portconfig_unittest.cc +++ b/src/lib/server_common/tests/portconfig_unittest.cc @@ -18,6 +18,7 @@ #include #include +#include #include #include @@ -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 diff --git a/src/lib/testutils/Makefile.am b/src/lib/testutils/Makefile.am index 2281b6d700..dc275b047b 100644 --- a/src/lib/testutils/Makefile.am +++ b/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) diff --git a/src/lib/util/tests/buffer_unittest.cc b/src/lib/util/tests/buffer_unittest.cc index 9af3d57c6e..02ca83d803 100644 --- a/src/lib/util/tests/buffer_unittest.cc +++ b/src/lib/util/tests/buffer_unittest.cc @@ -18,6 +18,7 @@ #ifdef EXPECT_DEATH #include +#include #endif /* EXPECT_DEATH */ #include @@ -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 } diff --git a/src/lib/util/threads/tests/lock_unittest.cc b/src/lib/util/threads/tests/lock_unittest.cc index 0b4d3ce4ec..c6e2e1939d 100644 --- a/src/lib/util/threads/tests/lock_unittest.cc +++ b/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 + #include #include - -#include +#include #include #include @@ -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 diff --git a/src/lib/util/unittests/Makefile.am b/src/lib/util/unittests/Makefile.am index 451ab4e34c..55e0372977 100644 --- a/src/lib/util/unittests/Makefile.am +++ b/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 diff --git a/src/lib/util/unittests/check_valgrind.cc b/src/lib/util/unittests/check_valgrind.cc new file mode 100644 index 0000000000..2951d2515e --- /dev/null +++ b/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 + +namespace isc { +namespace util { +namespace unittests { + +#if HAVE_VALGRIND_HEADERS +#include +/// \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 index 0000000000..c08b58bc10 --- /dev/null +++ b/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 + +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