Fixed memory leaks.

Fixed two memory leaks:
 - AnalysisData leaked memory if startData() was called more than once.
 - Memory for the error collector object in selection parser was not
   properly managed.  Now the object is no longer dynamically allocated,
   which is much cleaner.

Also added a call to xmlCleanupParser() at the end of tests to remove
static memory allocated by libxml2 from, e.g., valgrind reports.

Also added suppressions for legacy and external library code.

Change-Id: I3ec213b6441d38f46bda138c959dbb5990144027

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 73f7889c47c70ae391fba20742317db201ea5aa4..4912f0008fd9fee80084e012de93901874d6491e 100644 (file)
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -542,6 +542,7 @@ mark_as_advanced(GMX_USE_GMOCK)
 if (BUILD_TESTING)
     add_subdirectory(src/external/gmock-1.6.0)
 endif (BUILD_TESTING)
+set(MEMORYCHECK_SUPPRESSIONS_FILE ${CMAKE_SOURCE_DIR}/cmake/legacy_and_external.supp)
 
 find_package(Doxygen)
 

diff --git a/admin/GerritBuild b/admin/GerritBuild
index 58ebefa95ae7df282ac35992f54c8d5cc0a159c0..073589b1ab0883b7951c99ee59d675604c467819 100755 (executable)
--- a/admin/GerritBuild
+++ b/admin/GerritBuild
@@ -4,4 +4,5 @@ if [ -n "${CMakeVersion}" ] ; then
 fi
 CC=gcc-${CompilerVersion} CXX=g++-${CompilerVersion} cmake -D GMX_DOUBLE=${GMX_DOUBLE} -D GMX_MPI=${GMX_MPI} -D GMX_OPENMP=${GMX_OPENMP} -DGMX_DEFAULT_SUFFIX=off -DCMAKE_BUILD_TYPE=Debug . &&
 make &&
-ctest -D ExperimentalTest -V
+ctest -D ExperimentalMemCheck -L GTest -V  #TODO parse valgrind output and show on website
+ctest -D ExperimentalTest -LE GTest -V     #TODO valgrind should be run for integration tests but with leak-check=no

diff --git a/cmake/legacy_and_external.supp b/cmake/legacy_and_external.supp
new file mode 100644 (file)
index 0000000..64f8464
--- /dev/null
+++ b/cmake/legacy_and_external.supp
@@ -0,0 +1,82 @@
+# This suppression file is used to run the C++ Unit Tests
+# All problems in external libraries are excluded
+# Memory leaks in the legacy code are excluded
+# Other problems besides memory leaks should NOT be excluded for legacy code
+
+{
+   libz
+   Memcheck:Cond
+   fun:inflateReset2
+   fun:inflateInit2_
+}
+
+{
+   gmx_fio_open1
+   Memcheck:Leak
+   fun:calloc
+   fun:save_calloc
+   fun:gmx_fio_open
+}
+
+{
+   gmx_fio_open2
+   Memcheck:Leak
+   fun:malloc
+   fun:strdup
+   fun:gmx_fio_open
+}
+
+{
+   gmx_fio_open3
+   Memcheck:Leak
+   fun:calloc
+   fun:save_calloc
+   fun:gmx_fio_make_dummy
+   fun:gmx_fio_insert
+   fun:gmx_fio_open
+}
+
+{
+   put_symtab1
+   Memcheck:Leak
+   fun:malloc
+   fun:strdup
+   fun:enter_buf
+   fun:put_symtab
+}
+
+{
+   put_symtab2
+   Memcheck:Leak
+   fun:calloc
+   fun:save_calloc
+   fun:new_symbuf
+   fun:enter_buf
+   fun:put_symtab
+}
+
+{
+   tMPI_init_once
+   Memcheck:Leak
+   fun:malloc
+   fun:tMPI_Malloc
+   fun:tMPI_Thread_mutex_init_once
+   fun:tMPI_Thread_mutex_lock
+}
+
+{
+   get_w_conf
+   Memcheck:Leak
+   fun:calloc
+   fun:save_calloc
+   fun:get_w_conf
+}
+
+{
+   read_tps_conf
+   Memcheck:Leak
+   fun:calloc
+   fun:save_calloc
+   fun:read_tps_conf
+}
+

diff --git a/src/gromacs/analysisdata/analysisdata.cpp b/src/gromacs/analysisdata/analysisdata.cpp
index 6beb1b35f0d7c8f134acc1766957c1a90ea37cb2..7b756a8a935cd07f2d8719213038c9e1e2c1f02c 100644 (file)
--- a/src/gromacs/analysisdata/analysisdata.cpp
+++ b/src/gromacs/analysisdata/analysisdata.cpp
@@ -192,9 +192,10 @@ AnalysisData::startData(AnalysisDataParallelOptions opt)
     std::auto_ptr<AnalysisDataHandle> handle(new AnalysisDataHandle(this));
     _impl->_handles.push_back(handle.get());
 
+    size_t oldSize = _impl->_pending.size();
     _impl->_pending.resize(2 * _impl->_handles.size() - 1);
     Impl::FrameList::iterator i;
-    for (i = _impl->_pending.begin(); i != _impl->_pending.end(); ++i)
+    for (i = _impl->_pending.begin() + oldSize; i != _impl->_pending.end(); ++i)
     {
         *i = new AnalysisDataFrame();
         (*i)->allocate(columnCount());

diff --git a/src/gromacs/selection/scanner.h b/src/gromacs/selection/scanner.h
index f6230b0a9fcba980508b1065e5bc9a844fee4ab4..f0a812fe58e3f50129db526f9fcd816f175839d7 100644 (file)
--- a/src/gromacs/selection/scanner.h
+++ b/src/gromacs/selection/scanner.h
@@ -61,6 +61,10 @@ _gmx_sel_init_lexer(yyscan_t *scannerp, struct gmx_ana_selcollection_t *sc,
 /** Frees memory allocated for the selection scanner. */
 void
 _gmx_sel_free_lexer(yyscan_t scanner);
+/** Sets the error reporter object for the selection scanner. */
+void
+_gmx_sel_set_lexer_error_reporter(yyscan_t scanner,
+                                  gmx::MessageStringCollector *errors);
 
 /** Returns true if the scanner is interactive. */
 bool

diff --git a/src/gromacs/selection/scanner_internal.cpp b/src/gromacs/selection/scanner_internal.cpp
index 7c617e4042094ef15c3ce2676a4bc9821200e7dd..2d6e92306c84fff8d05fcf6cf689d200c85797d8 100644 (file)
--- a/src/gromacs/selection/scanner_internal.cpp
+++ b/src/gromacs/selection/scanner_internal.cpp
@@ -58,6 +58,7 @@
 
 #include "gromacs/fatalerror/errorcodes.h"
 #include "gromacs/fatalerror/exceptions.h"
+#include "gromacs/fatalerror/gmxassert.h"
 #include "gromacs/fatalerror/messagestringcollector.h"
 
 #include "gromacs/selection/selmethod.h"
@@ -473,11 +474,9 @@ _gmx_sel_init_lexer(yyscan_t *scannerp, struct gmx_ana_selcollection_t *sc,
         GMX_THROW(gmx::InternalError("Lexer initialization failed"));
     }
 
-    gmx::MessageStringCollector *errors = new gmx::MessageStringCollector;
-
     snew(state, 1);
     state->sc        = sc;
-    state->errors    = errors;
+    state->errors    = NULL;
     state->bGroups   = bGroups;
     state->grps      = grps;
     state->nexpsel   = (maxnr > 0 ? sc->sel.size() + maxnr : -1);
@@ -523,6 +522,14 @@ _gmx_sel_free_lexer(yyscan_t scanner)
     _gmx_sel_yylex_destroy(scanner);
 }
 
+void
+_gmx_sel_set_lexer_error_reporter(yyscan_t scanner,
+                                  gmx::MessageStringCollector *errors)
+{
+    gmx_sel_lexer_t *state = _gmx_sel_yyget_extra(scanner);
+    state->errors = errors;
+}
+
 bool
 _gmx_sel_is_lexer_interactive(yyscan_t scanner)
 {
@@ -541,6 +548,7 @@ gmx::MessageStringCollector *
 _gmx_sel_lexer_error_reporter(yyscan_t scanner)
 {
     gmx_sel_lexer_t *state = _gmx_sel_yyget_extra(scanner);
+    GMX_RELEASE_ASSERT(state->errors != NULL, "Error reporter not set");
     return state->errors;
 }
 

diff --git a/src/gromacs/selection/selectioncollection.cpp b/src/gromacs/selection/selectioncollection.cpp
index f6c941a34357e510f70b211836037891ee834ded..08d0092929d0ce9d8f108bcff8682a5da1f51992 100644 (file)
--- a/src/gromacs/selection/selectioncollection.cpp
+++ b/src/gromacs/selection/selectioncollection.cpp
@@ -155,10 +155,12 @@ SelectionCollection::Impl::runParser(yyscan_t scanner, int maxnr,
                                      std::vector<Selection *> *output)
 {
     gmx_ana_selcollection_t *sc = &_sc;
-    MessageStringCollector *errors = _gmx_sel_lexer_error_reporter(scanner);
     GMX_ASSERT(sc == _gmx_sel_lexer_selcollection(scanner),
                "Incorrectly initialized lexer");
 
+    MessageStringCollector errors;
+    _gmx_sel_set_lexer_error_reporter(scanner, &errors);
+
     int oldCount = sc->sel.size();
     int bOk = !_gmx_sel_yybparse(scanner);
     _gmx_sel_free_lexer(scanner);
@@ -166,7 +168,7 @@ SelectionCollection::Impl::runParser(yyscan_t scanner, int maxnr,
     if (maxnr > 0 && nr != maxnr)
     {
         bOk = false;
-        errors->append("Too few selections provided");
+        errors.append("Too few selections provided");
     }
 
     if (bOk)
@@ -178,10 +180,10 @@ SelectionCollection::Impl::runParser(yyscan_t scanner, int maxnr,
         }
     }
 
-    if (!bOk || !errors->isEmpty())
+    if (!bOk || !errors.isEmpty())
     {
-        GMX_ASSERT(!bOk && !errors->isEmpty(), "Inconsistent error reporting");
-        GMX_THROW(InvalidInputError(errors->toString()));
+        GMX_ASSERT(!bOk && !errors.isEmpty(), "Inconsistent error reporting");
+        GMX_THROW(InvalidInputError(errors.toString()));
     }
 }
 
@@ -545,7 +547,7 @@ SelectionCollection::parseFromFile(const std::string &filename,
     {
         _impl->runParser(scanner, -1, output);
     }
-    catch (std::exception &)
+    catch (...)
     {
         ffclose(fp);
         throw;

diff --git a/src/testutils/CMakeLists.txt b/src/testutils/CMakeLists.txt
index f175fc2eee3ee0cdc2b437da9d24c82ed106055b..bf163d968a1da9e78f9336add0cb276aa1d8c430 100644 (file)
--- a/src/testutils/CMakeLists.txt
+++ b/src/testutils/CMakeLists.txt
@@ -4,12 +4,13 @@ if (GMX_USE_GTEST AND LIBXML2_FOUND)
     include_directories(${GTEST_INCLUDE_DIRS})
     list(APPEND COMMON_SOURCES refdata.cpp)
     set(TESTUTILS_HAVE_REFDATA TRUE)
+    add_definitions(-DTESTUTILS_HAVE_REFDATA)
 endif ()
 
 add_library(testutils STATIC ${COMMON_SOURCES})
 set(TESTUTILS_LIBS testutils)
 target_link_libraries(testutils libgromacs)
-if (GMX_USE_GTEST AND LIBXML2_FOUND)
+if (TESTUTILS_HAVE_REFDATA)
     list(APPEND TESTUTILS_LIBS ${GTEST_LIBRARIES} ${LIBXML2_LIBRARIES})
     target_link_libraries(testutils ${GTEST_LIBRARIES} ${LIBXML2_LIBRARIES})
 endif ()

diff --git a/src/testutils/TestMacros.cmake b/src/testutils/TestMacros.cmake
index 06a12cee4cdee06a4cb31fe06d2a505f50dc9bd5..173819c538f0e4f7e9cbb45a61a13553c310a159 100644 (file)
--- a/src/testutils/TestMacros.cmake
+++ b/src/testutils/TestMacros.cmake
@@ -11,6 +11,7 @@ function (add_gtest_test NAME EXENAME)
         set_target_properties(${EXENAME} PROPERTIES COMPILE_DEFINITIONS ${DEFS})
         add_test(NAME ${NAME}
                  COMMAND ${EXENAME} --gtest_output=xml:${CMAKE_BINARY_DIR}/Testing/Temporary/${EXENAME}.xml)
+        set_tests_properties(${NAME} PROPERTIES LABELS "GTest")
     endif ()
 endfunction ()
 
@@ -27,6 +28,7 @@ function (add_gmock_test NAME EXENAME)
         set_target_properties(${EXENAME} PROPERTIES COMPILE_DEFINITIONS ${DEFS})
         add_test(NAME ${NAME}
                  COMMAND ${EXENAME} --gtest_output=xml:${CMAKE_BINARY_DIR}/Testing/Temporary/${EXENAME}.xml)
+        set_tests_properties(${NAME} PROPERTIES LABELS "GTest")
     endif ()
 endfunction ()
 

diff --git a/src/testutils/refdata-common.cpp b/src/testutils/refdata-common.cpp
index 37b27d94668749d3543266fddc8195a976032636..bfa3c2d7b66b8a4e54b89dbc655744b9bf31efa3 100644 (file)
--- a/src/testutils/refdata-common.cpp
+++ b/src/testutils/refdata-common.cpp
@@ -86,6 +86,9 @@ int initReferenceData(int *argc, char **argv)
         }
     }
     *argc = newi;
+#ifdef TESTUTILS_HAVE_REFDATA
+    internal::addGlobalReferenceDataEnvironment();
+#endif
 
     return 0;
 }

diff --git a/src/testutils/refdata.cpp b/src/testutils/refdata.cpp
index 240354cebb69f398b3d231c83e862b9e5e65a4d9..d0988345fcaa0723319563e20049088edcaf0566 100644 (file)
--- a/src/testutils/refdata.cpp
+++ b/src/testutils/refdata.cpp
@@ -53,11 +53,35 @@
 
 #include "refdata-impl.h"
 
+namespace
+{
+
+class TestReferenceDataEnvironment : public ::testing::Environment
+{
+    public:
+        virtual void TearDown()
+        {
+            xmlCleanupParser();
+        }
+};
+
+} // namespace
+
 namespace gmx
 {
 namespace test
 {
 
+namespace internal
+{
+
+void addGlobalReferenceDataEnvironment()
+{
+    ::testing::AddGlobalTestEnvironment(new TestReferenceDataEnvironment);
+}
+
+} // namespace internal
+
 /********************************************************************
  * TestReferenceData::Impl
  */

diff --git a/src/testutils/refdata.h b/src/testutils/refdata.h
index 37ba8a80b72b280c2c6c3d5e3e62d501c7a88806..39187a0f338b1af71f748f36c060a2e2a6914af9 100644 (file)
--- a/src/testutils/refdata.h
+++ b/src/testutils/refdata.h
@@ -103,13 +103,28 @@ std::string getReferenceDataPath();
  * can be used to change it.
  * Recognized command-line arguments are removed from the list.
  *
- * Also creates the directory for storing the reference data.
- *
  * This function is automatically called by test_main_gtest.cpp and
  * test_main_gmock.cpp.
  */
 int initReferenceData(int *argc, char **argv);
 
+namespace internal
+{
+
+/*! \internal \brief
+ * Adds a global test teardown method for freeing libxml2 internal data.
+ *
+ * This method is called by initReferenceData(), and should not be called
+ * directly.
+ * It adds a global test environment object that calls xmlCleanupParser() at
+ * the end of all tests.  This makes memory reports from valgrind cleaner since
+ * otherwise they show the memory as "still reachable".
+ */
+void addGlobalReferenceDataEnvironment();
+
+} // namespace internal
+
+
 class TestReferenceChecker;
 
 /*! \libinternal \brief