Error/exception handling improvements.

- Change formatErrorMessage() to printFatalErrorMessage() that directly
  prints the error message for an exception into a FILE (typically
  stderr) instead of creating a std::string.  This makes it possible to
  handle out-of-memory errors internally within the function.
  The function is much easier to use if we can guarantee that it never
  throws.
- Add basic handling of std::bad_alloc into the above function.
- Resolve clang warnings in the function.
- Add a macro for conveniently catching all exceptions at C++ - C
  boundary and reporting them as fatal errors.
- Initialize ProgramInfo in all C++ binaries.

Related to #838.

Change-Id: I77c355480485f563c57b6c6ea9b07a8ce7f433a8

diff --git a/share/template/template.cpp b/share/template/template.cpp
index c3c6eb3b81481e95adfb85596fb35d0518e2e4bf..0a414814584e0aa1dd1e36e267bebef94e70dabe 100644 (file)
--- a/share/template/template.cpp
+++ b/share/template/template.cpp
@@ -235,6 +235,7 @@ AnalysisTemplate::writeOutput()
 int
 main(int argc, char *argv[])
 {
+    ProgramInfo::init(argc, argv);
     try
     {
         AnalysisTemplate module;
@@ -243,7 +244,7 @@ main(int argc, char *argv[])
     }
     catch (const std::exception &ex)
     {
-        fprintf(stderr, "%s", gmx::formatErrorMessage(ex).c_str());
+        gmx::printFatalErrorMessage(stderr, ex);
         return 1;
     }
 }

diff --git a/src/gromacs/commandline/cmdlinemodulemanager.h b/src/gromacs/commandline/cmdlinemodulemanager.h
index a665488f426fad20df52ff0da12ecbaabb66d2f5..f27b26b35de91ddd2a0060a9939bcfdfdfd1f6dc 100644 (file)
--- a/src/gromacs/commandline/cmdlinemodulemanager.h
+++ b/src/gromacs/commandline/cmdlinemodulemanager.h
@@ -72,7 +72,7 @@ main(int argc, char *argv[])
     }
     catch (const std::exception &ex)
     {
-        fprintf(stderr, "%s", gmx::formatErrorMessage(ex).c_str());
+        gmx::printFatalErrorMessage(stderr, ex);
         return 1;
     }
 }

diff --git a/src/gromacs/gmxlib/wman.cpp b/src/gromacs/gmxlib/wman.cpp
index e9cdec25085aab67213b4fa4ad0c39ead444eafd..83b30d344bd40d68f1b947763ef20e5a559d767b 100644 (file)
--- a/src/gromacs/gmxlib/wman.cpp
+++ b/src/gromacs/gmxlib/wman.cpp
@@ -41,6 +41,7 @@
 
 #include <string>
 
+#include "gromacs/utility/exceptions.h"
 #include "gromacs/utility/stringutil.h"
 
 #include "gmx_fatal.h"
@@ -489,11 +490,7 @@ static char *repall(const char *s,int nsr,const t_sandr_const sa[])
         }
         return gmx_strdup(result.c_str());
     }
-    catch (const std::bad_alloc &)
-    {
-        gmx_fatal(FARGS, "Out of memory");
-    }
-    return gmx_strdup(s);
+    GMX_CATCH_ALL_AND_EXIT_WITH_FATAL_ERROR;
 }
 
 static char *repallww(const char *s,int nsr,const t_sandr sa[])
@@ -507,11 +504,7 @@ static char *repallww(const char *s,int nsr,const t_sandr sa[])
         }
         return gmx_strdup(result.c_str());
     }
-    catch (const std::bad_alloc &)
-    {
-        gmx_fatal(FARGS, "Out of memory");
-    }
-    return gmx_strdup(s);
+    GMX_CATCH_ALL_AND_EXIT_WITH_FATAL_ERROR;
 }
 
 static char *html_xref(char *s,const char *program, t_linkdata *links,gmx_bool bWiki)

diff --git a/src/gromacs/trajectoryanalysis.h b/src/gromacs/trajectoryanalysis.h
index 8f443f1291c3eba6d8c6c4257e218f086212f708..02d8440ee38f093fda010ce827f9bf07f9f02c0e 100644 (file)
--- a/src/gromacs/trajectoryanalysis.h
+++ b/src/gromacs/trajectoryanalysis.h
@@ -83,6 +83,7 @@
 #include "trajectoryanalysis/analysissettings.h"
 #include "trajectoryanalysis/cmdlinerunner.h"
 #include "utility/exceptions.h"
+#include "utility/programinfo.h"
 #include "utility/stringutil.h"
 
 #endif

diff --git a/src/gromacs/trajectoryanalysis/tests/test_selection.cpp b/src/gromacs/trajectoryanalysis/tests/test_selection.cpp
index d3d25a50f0a72d680284933f39f2532c4dec19e3..795a9c6ec6fba2522df461c7a2f854f4bd6d2bdd 100644 (file)
--- a/src/gromacs/trajectoryanalysis/tests/test_selection.cpp
+++ b/src/gromacs/trajectoryanalysis/tests/test_selection.cpp
@@ -33,19 +33,16 @@
  *
  * \author Teemu Murtola <teemu.murtola@cbr.su.se>
  */
-#ifdef HAVE_CONFIG_H
-#include <config.h>
-#endif
-
-#include <gromacs/options/basicoptions.h>
-#include <gromacs/options/options.h>
-#include <gromacs/selection/selection.h>
-#include <gromacs/selection/selectionoption.h>
-#include <gromacs/trajectoryanalysis/analysismodule.h>
-#include <gromacs/trajectoryanalysis/analysissettings.h>
-#include <gromacs/trajectoryanalysis/cmdlinerunner.h>
-#include <gromacs/utility/exceptions.h>
-#include <gromacs/utility/stringutil.h>
+#include "gromacs/options/basicoptions.h"
+#include "gromacs/options/options.h"
+#include "gromacs/selection/selection.h"
+#include "gromacs/selection/selectionoption.h"
+#include "gromacs/trajectoryanalysis/analysismodule.h"
+#include "gromacs/trajectoryanalysis/analysissettings.h"
+#include "gromacs/trajectoryanalysis/cmdlinerunner.h"
+#include "gromacs/utility/exceptions.h"
+#include "gromacs/utility/programinfo.h"
+#include "gromacs/utility/stringutil.h"
 
 namespace gmx
 {
@@ -187,6 +184,7 @@ SelectionTester::writeOutput()
 int
 main(int argc, char *argv[])
 {
+    gmx::ProgramInfo::init(argc, argv);
     try
     {
         gmx::SelectionTester module;
@@ -196,7 +194,7 @@ main(int argc, char *argv[])
     }
     catch (const std::exception &ex)
     {
-        fprintf(stderr, "%s", gmx::formatErrorMessage(ex).c_str());
+        gmx::printFatalErrorMessage(stderr, ex);
         return 1;
     }
 }

diff --git a/src/gromacs/utility/errorcodes.cpp b/src/gromacs/utility/errorcodes.cpp
index ecb79cfd5656ab53a6e3f87bbe48f0d5a332f40f..7c20a46e368049663938c28ddbe30a7f2fa3dabb 100644 (file)
--- a/src/gromacs/utility/errorcodes.cpp
+++ b/src/gromacs/utility/errorcodes.cpp
@@ -37,10 +37,10 @@
  */
 #include "gromacs/utility/errorcodes.h"
 
-#include <cstdarg>
-#include <cstdio>
 #include <cstdlib>
 
+#include "gromacs/legacyheaders/thread_mpi/mutex.h"
+
 #include "errorformat.h"
 
 // This has to match the enum in errorcodes.h
@@ -81,19 +81,18 @@ static void standardErrorHandler(int retcode, const char *msg,
                                  const char *file, int line)
 {
     const char *title = getErrorCodeString(retcode);
-    fprintf(stderr, "%s",
-            internal::formatFatalError(title, msg, NULL, file, line).c_str());
+    internal::printFatalError(stderr, title, msg, NULL, file, line);
     std::exit(1);
 }
 
 static ErrorHandlerFunc error_handler = standardErrorHandler;
+static tMPI::mutex handler_mutex;
 
 ErrorHandlerFunc setFatalErrorHandler(ErrorHandlerFunc handler)
 {
-    // TODO: Acquire a mutex here
+    tMPI::lock_guard<tMPI::mutex> lock(handler_mutex);
     ErrorHandlerFunc old_handler = error_handler;
     error_handler = handler;
-    // TODO: Release the mutex here
     return old_handler;
 }
 
@@ -103,9 +102,11 @@ namespace internal
 
 void fatalError(int retcode, const char *msg, const char *file, int line)
 {
-    // TODO: Acquire a mutex here
-    ErrorHandlerFunc handler = error_handler;
-    // TODO: Release the mutex here
+    ErrorHandlerFunc handler = NULL;
+    {
+        tMPI::lock_guard<tMPI::mutex> lock(handler_mutex);
+        handler = error_handler;
+    }
     if (handler != NULL)
     {
         handler(retcode, msg, file, line);

diff --git a/src/gromacs/utility/errorformat.cpp b/src/gromacs/utility/errorformat.cpp
index 871f26607b06b4a9f1cf99699b622e9b5f5c938f..ad9643b08ac533a51b43983cee49f8e168d5c437 100644 (file)
--- a/src/gromacs/utility/errorformat.cpp
+++ b/src/gromacs/utility/errorformat.cpp
@@ -37,7 +37,7 @@
  */
 #include "gromacs/utility/errorformat.h"
 
-#include <string>
+#include <cstdio>
 
 #include "gromacs/legacyheaders/copyrite.h"
 
@@ -51,31 +51,38 @@ namespace gmx
 namespace internal
 {
 
-std::string formatFatalError(const char *title, const char *details,
-                             const char *func, const char *file, int line)
+void printFatalError(FILE *fp, const char *title, const char *details,
+                     const char *func, const char *file, int line)
 {
-    std::string result;
-    result.append("\n-------------------------------------------------------\n");
-    const char *programName = ProgramInfo::getInstance().programName().c_str();
-    result.append(formatString("Program %s, %s\n", programName, GromacsVersion()));
+    // In case ProgramInfo is not initialized and there is an issue with the
+    // initialization, fall back to "GROMACS".
+    const char *programName = "GROMACS";
+    try
+    {
+        programName = ProgramInfo::getInstance().programName().c_str();
+    }
+    catch (const std::exception &)
+    {
+    }
+
+    std::fprintf(fp, "\n-------------------------------------------------------\n");
+    std::fprintf(fp, "Program %s, %s\n", programName, GromacsVersion());
     if (func != NULL)
     {
-        result.append(formatString("In function %s\n", func));
+        std::fprintf(fp, "In function: %s\n", func);
     }
     // TODO: Strip away absolute paths from file names (CMake seems to generate those)
     if (file != NULL)
     {
-        result.append(formatString("Source file %s, line %d\n\n", file, line));
-    }
-    else
-    {
-        result.append("\n");
+        std::fprintf(fp, "Source file %s, line %d\n", file, line);
     }
-    result.append(formatString("%s:\n%s\n", title, details));
-    result.append("For more information and tips for troubleshooting, please check the GROMACS\n"
-                  "website at http://www.gromacs.org/Documentation/Errors");
-    result.append("\n-------------------------------------------------------\n");
-    return result;
+    std::fprintf(fp, "\n");
+    std::fprintf(fp, "%s:\n", title);
+    // TODO: Line wrapping
+    std::fprintf(fp, "%s\n", details);
+    std::fprintf(fp, "For more information and tips for troubleshooting, please check the GROMACS\n"
+                     "website at http://www.gromacs.org/Documentation/Errors");
+    std::fprintf(fp, "\n-------------------------------------------------------\n");
 }
 
 } // namespace internal

diff --git a/src/gromacs/utility/errorformat.h b/src/gromacs/utility/errorformat.h
index bee75fb4882e777fb11370ddfa9c0e92fc1515ad..30da26002163157b07d5c320aeccbb13b95660a3 100644 (file)
--- a/src/gromacs/utility/errorformat.h
+++ b/src/gromacs/utility/errorformat.h
@@ -38,7 +38,7 @@
 #ifndef GMX_UTILITY_ERRORFORMAT_H
 #define GMX_UTILITY_ERRORFORMAT_H
 
-#include <string>
+#include <cstdio>
 
 namespace gmx
 {
@@ -50,10 +50,12 @@ namespace internal
 /*! \internal \brief
  * Formats common headers and footers for error messages.
  *
+ * Does not throw.
+ *
  * \ingroup module_utility
  */
-std::string formatFatalError(const char *title, const char *details,
-                             const char *func, const char *file, int line);
+void printFatalError(FILE *fp, const char *title, const char *details,
+                     const char *func, const char *file, int line);
 
 } // namespace internal
 //! \endcond

diff --git a/src/gromacs/utility/exceptions.cpp b/src/gromacs/utility/exceptions.cpp
index 9074acf66bb1fb73ccc97710d173ce0681ae6428..3244e7d33bed420f44062312abe5e3321f800613 100644 (file)
--- a/src/gromacs/utility/exceptions.cpp
+++ b/src/gromacs/utility/exceptions.cpp
@@ -105,23 +105,37 @@ int NotImplementedError::errorCode() const
  * Global functions
  */
 
-std::string formatErrorMessage(const std::exception &ex)
+void printFatalErrorMessage(FILE *fp, const std::exception &ex)
 {
     const char *title = "Unknown exception";
-    const char *func = NULL;
-    const char *file = NULL;
-    int line = 0;
     const GromacsException *gmxEx = dynamic_cast<const GromacsException *>(&ex);
     // TODO: Also treat common standard exceptions
     if (gmxEx != NULL)
     {
         title = getErrorCodeString(gmxEx->errorCode());
-        func = *boost::get_error_info<boost::throw_function>(*gmxEx);
-        file = *boost::get_error_info<boost::throw_file>(*gmxEx);
-        line = *boost::get_error_info<boost::throw_line>(*gmxEx);
+    }
+    else if (dynamic_cast<const std::bad_alloc *>(&ex) != NULL)
+    {
+        title = "Memory allocation failed";
+    }
+    // We can't call get_error_info directly on ex since our internal boost
+    // needs to be compiled with BOOST_NO_RTTI. So we do the dynamic_cast
+    // here instead.
+    const char *const *funcPtr = NULL;
+    const char *const *filePtr = NULL;
+    const int         *linePtr = NULL;
+    const boost::exception *boostEx = dynamic_cast<const boost::exception *>(&ex);
+    if (boostEx != NULL)
+    {
+        funcPtr = boost::get_error_info<boost::throw_function>(*boostEx);
+        filePtr = boost::get_error_info<boost::throw_file>(*boostEx);
+        linePtr = boost::get_error_info<boost::throw_line>(*boostEx);
     }
     // TODO: Treat errno information in boost exceptions
-    return internal::formatFatalError(title, ex.what(), func, file, line);
+    internal::printFatalError(fp, title, ex.what(),
+                              funcPtr != NULL ? *funcPtr : NULL,
+                              filePtr != NULL ? *filePtr : NULL,
+                              linePtr != NULL ? *linePtr : 0);
 }
 
 } // namespace gmx

diff --git a/src/gromacs/utility/exceptions.h b/src/gromacs/utility/exceptions.h
index 4415f1ab1dedf9620ea3ac73effdc88abad873e2..7f246325ec5ba247ddbdfd409561f5cac0ed231f 100644 (file)
--- a/src/gromacs/utility/exceptions.h
+++ b/src/gromacs/utility/exceptions.h
@@ -39,6 +39,9 @@
 #ifndef GMX_UTILITY_EXCEPTIONS_H
 #define GMX_UTILITY_EXCEPTIONS_H
 
+#include <cstdio>
+#include <cstdlib>
+
 #include <exception>
 #include <string>
 
@@ -283,12 +286,16 @@ if (fp == NULL)
     } while(0)
 
 /*! \brief
- * Formats a standard error message for reporting an error.
+ * Formats a standard fatal error message for reporting an exception.
+ *
+ * Does not throw.  If memory allocation fails or some other error occurs
+ * while formatting the error, tries to print a reasonable alternative message.
  *
  * Normal usage in Gromacs command-line programs is like this:
  * \code
 int main(int argc, char *argv[])
 {
+    gmx::ProgramInfo::init(argc, argv);
     try
     {
         // The actual code for the program
@@ -296,13 +303,13 @@ int main(int argc, char *argv[])
     }
     catch (const std::exception &ex)
     {
-        fprintf(stderr, "%s", gmx::formatErrorMessage(ex).c_str());
+        gmx::printFatalErrorMessage(stderr, ex);
         return 1;
     }
 }
  * \endcode
  */
-std::string formatErrorMessage(const std::exception &ex);
+void printFatalErrorMessage(FILE *fp, const std::exception &ex);
 
 /*! \brief
  * Converts an exception into a return code.
@@ -311,6 +318,38 @@ int translateException(const std::exception &ex);
 
 /*!\}*/
 
+/*! \cond libapi */
+/*! \libinternal \brief
+ * Macro for catching exceptions at C++ -> C boundary.
+ *
+ * This macro is intended for uniform handling of exceptions when C++ code is
+ * called from C code within Gromacs.  Since most existing code is written
+ * using the assumption that fatal errors terminate the program, this macro
+ * implements this behavior for exceptions.  It should only be used in cases
+ * where the error cannot be propagated upwards using return values or such.
+ *
+ * Having this as a macro instead of having the same code in each place makes
+ * it easy to 1) find all such locations in the code, and 2) change the exact
+ * behavior if needed.
+ *
+ * Usage:
+ * \code
+try
+{
+    // C++ code
+}
+GMX_CATCH_ALL_AND_EXIT_WITH_FATAL_ERROR;
+ * \endcode
+ *
+ * \inlibraryapi
+ */
+#define GMX_CATCH_ALL_AND_EXIT_WITH_FATAL_ERROR \
+    catch (const std::exception &ex) { \
+        ::gmx::printFatalErrorMessage(stderr, ex); \
+        std::exit(1); \
+    }
+//! \endcond
+
 } // namespace gmx
 
 #endif

diff --git a/src/gromacs/utility/gmxassert.cpp b/src/gromacs/utility/gmxassert.cpp
index 7eb1d426725abe93bb7a9ea4466d6865dc436d70..2f4f2ed6602ef246c64441a6f6aa843c80b41058 100644 (file)
--- a/src/gromacs/utility/gmxassert.cpp
+++ b/src/gromacs/utility/gmxassert.cpp
@@ -56,11 +56,18 @@ namespace internal
 void assertHandler(const char *condition, const char *msg,
                    const char *func, const char *file, int line)
 {
-    std::string text =
-        formatFatalError("Assertion failed",
-                         formatString("Condition: %s\n%s", condition, msg).c_str(),
-                         func, file, line);
-    std::fprintf(stderr, "%s", text.c_str());
+    try
+    {
+        std::string title = formatString("Condition: %s\n%s", condition, msg);
+        printFatalError(stderr, "Assertion failed", title.c_str(),
+                        func, file, line);
+    }
+    catch (const std::bad_alloc &)
+    {
+        printFatalError(stderr, "Assertion failed",
+                "(memory allocation failed while formatting the error message)",
+                func, file, line);
+    }
     std::abort();
 }
 

diff --git a/src/gromacs/utility/programinfo.cpp b/src/gromacs/utility/programinfo.cpp
index b646b05ee828a642d0c639c7ac18fbeeb2556cb2..b1ef191d5a435553e78af7f1ee583d08a3caab36 100644 (file)
--- a/src/gromacs/utility/programinfo.cpp
+++ b/src/gromacs/utility/programinfo.cpp
@@ -45,13 +45,13 @@
 #include <cstdlib>
 #include <cstring>
 
-#include <new>
 #include <string>
 
 #include <boost/scoped_ptr.hpp>
 
 #include "gromacs/legacyheaders/statutil.h"
 
+#include "gromacs/utility/exceptions.h"
 #include "gromacs/utility/path.h"
 #include "gromacs/utility/stringutil.h"
 
@@ -136,9 +136,9 @@ const ProgramInfo &ProgramInfo::init(const char *realBinaryName,
         g_programInfo.reset(new ProgramInfo(realBinaryName, argc, argv));
         return *g_programInfo;
     }
-    catch (const std::bad_alloc &)
+    catch (const std::exception &ex)
     {
-        // TODO: Report error.
+        printFatalErrorMessage(stderr, ex);
         std::exit(1);
     }
 }

diff --git a/src/programs/g_ana/g_ana.cpp b/src/programs/g_ana/g_ana.cpp
index 574227b24bd94a43b140377efda1f017d0df3d42..48245c5b1aad527fa109d8242ac427bcb5f8ba3e 100644 (file)
--- a/src/programs/g_ana/g_ana.cpp
+++ b/src/programs/g_ana/g_ana.cpp
@@ -58,7 +58,7 @@ main(int argc, char *argv[])
     }
     catch (const std::exception &ex)
     {
-        fprintf(stderr, "%s", gmx::formatErrorMessage(ex).c_str());
+        gmx::printFatalErrorMessage(stderr, ex);
         return 1;
     }
 }

diff --git a/src/testutils/testoptions.cpp b/src/testutils/testoptions.cpp
index 884f2168692d8aee7c166dd4d4010be94a71aeb2..5fbea78a9512b654a7a1612d0d2ca847cbd93739 100644 (file)
--- a/src/testutils/testoptions.cpp
+++ b/src/testutils/testoptions.cpp
@@ -83,9 +83,9 @@ void initTestUtils(const char *dataPath, int *argc, char *argv[])
         }
         swap(commandLine, s_commandLine);
     }
-    catch (const std::bad_alloc &)
+    catch (const std::exception &ex)
     {
-        std::fprintf(stderr, "Out of memory\n");
+        printFatalErrorMessage(stderr, ex);
         std::exit(1);
     }
     ::gmx::setFatalErrorHandler(NULL);