From 648db957059418a95139f737ae9418eb4e1947cf Mon Sep 17 00:00:00 2001 From: Daniel Arndt Date: Tue, 14 Aug 2018 17:07:58 +0200 Subject: [PATCH] Avoid slicing when throwing in Assert --- include/deal.II/base/exceptions.h | 116 +++++++++--------- source/base/exceptions.cc | 113 ++++++++--------- .../parameter_handler_19.debug.output | 8 +- tests/tests.h | 5 +- 4 files changed, 111 insertions(+), 131 deletions(-) diff --git a/include/deal.II/base/exceptions.h b/include/deal.II/base/exceptions.h index 9f01ac2012..a929f1d5c3 100644 --- a/include/deal.II/base/exceptions.h +++ b/include/deal.II/base/exceptions.h @@ -1020,6 +1020,15 @@ namespace StandardExceptions */ namespace deal_II_exceptions { + /** + * Setting this variable to false will disable deal.II's exception mechanism + * to abort the problem. The Assert() macro will throw the exception instead + * and the AssertNothrow() macro will just print the error message. This + * variable should not be changed directly. Use disable_abort_on_exception() + * instead. + */ + extern bool allow_abort_on_exception; + /** * Set a string that is printed upon output of the message indicating a * triggered Assert statement. This string, which is printed in @@ -1080,18 +1089,14 @@ namespace deal_II_exceptions namespace internals { /** - * Conditionally abort the program. - * - * Depending on whether deal_II_exceptions::disable_abort_on_exception was - * called, this function either aborts the program flow by printing the - * error message provided by @p exc and calling std::abort(), or - * throws @p exc instead. + * Abort the program by printing the + * error message provided by @p exc and calling std::abort(). */ [[noreturn]] void - abort(const ExceptionBase &exc); + internal_abort(const ExceptionBase &exc) noexcept; /** - * An enum describing how to treat an exception in issue_error. + * An enum describing how to treat an exception in issue_error_noreturn. */ enum ExceptionHandling { @@ -1100,24 +1105,21 @@ namespace deal_II_exceptions * deal_II_exceptions::disable_abort_on_exception has been called: in * that case the program will throw an exception. */ - abort_on_exception, + try_abort_on_exception, /** * Throw the exception normally. */ - throw_on_exception, - /** - * Call std::abort as long as - * deal_II_exceptions::disable_abort_on_exception has not been called: - * if it has, then just print a description of the exception to deallog. - */ - abort_nothrow_on_exception + throw_on_exception }; /** * This routine does the main work for the exception generation mechanism * used in the Assert and AssertThrow macros: as the - * name implies, this function either ends with a call to abort - * or throwing an exception. + * name implies, this function either ends throwing an exception (if + * @p handling is throw_on_exception, or @p handling is try_abort_exception + * and deal_II_exceptions::disable_abort_on_exception is false) or with a + * call to abort (if @p handling is try_abort_exception and + * deal_II_exceptions::disable_abort_on_exception is true). * * The actual exception object (the last argument) is typically an unnamed * object created in place; because we modify it, we can't take it by @@ -1141,14 +1143,21 @@ namespace deal_II_exceptions switch (handling) { - case abort_on_exception: - dealii::deal_II_exceptions::internals::abort(e); + case try_abort_on_exception: + { + if (dealii::deal_II_exceptions::allow_abort_on_exception) + internals::internal_abort(e); + else + { + // We are not allowed to abort, so just throw the error: + throw e; + } + } case throw_on_exception: throw e; // this function should never return (and AssertNothrow can); // something must have gone wrong in the error handling code for us // to get this far, so throw an exception. - case abort_nothrow_on_exception: default: throw ::dealii::StandardExceptions::ExcInternalError(); } @@ -1169,8 +1178,7 @@ namespace deal_II_exceptions */ template void - issue_error_nothrow(ExceptionHandling, - const char * file, + issue_error_nothrow(const char * file, int line, const char * function, const char * cond, @@ -1224,30 +1232,30 @@ namespace deal_II_exceptions */ #ifdef DEBUG # ifdef DEAL_II_HAVE_BUILTIN_EXPECT -# define Assert(cond, exc) \ - { \ - if (__builtin_expect(!(cond), false)) \ - ::dealii::deal_II_exceptions::internals::issue_error_noreturn( \ - ::dealii::deal_II_exceptions::internals::abort_on_exception, \ - __FILE__, \ - __LINE__, \ - __PRETTY_FUNCTION__, \ - #cond, \ - #exc, \ - exc); \ +# define Assert(cond, exc) \ + { \ + if (__builtin_expect(!(cond), false)) \ + ::dealii::deal_II_exceptions::internals::issue_error_noreturn( \ + ::dealii::deal_II_exceptions::internals::try_abort_on_exception, \ + __FILE__, \ + __LINE__, \ + __PRETTY_FUNCTION__, \ + #cond, \ + #exc, \ + exc); \ } # else /*ifdef DEAL_II_HAVE_BUILTIN_EXPECT*/ -# define Assert(cond, exc) \ - { \ - if (!(cond)) \ - ::dealii::deal_II_exceptions::internals::issue_error_noreturn( \ - ::dealii::deal_II_exceptions::internals::abort_on_exception, \ - __FILE__, \ - __LINE__, \ - __PRETTY_FUNCTION__, \ - #cond, \ - #exc, \ - exc); \ +# define Assert(cond, exc) \ + { \ + if (!(cond)) \ + ::dealii::deal_II_exceptions::internals::issue_error_noreturn( \ + ::dealii::deal_II_exceptions::internals::try_abort_on_exception, \ + __FILE__, \ + __LINE__, \ + __PRETTY_FUNCTION__, \ + #cond, \ + #exc, \ + exc); \ } # endif /*ifdef DEAL_II_HAVE_BUILTIN_EXPECT*/ #else @@ -1279,28 +1287,14 @@ namespace deal_II_exceptions { \ if (__builtin_expect(!(cond), false)) \ ::dealii::deal_II_exceptions::internals::issue_error_nothrow( \ - ::dealii::deal_II_exceptions::internals:: \ - abort_nothrow_on_exception, \ - __FILE__, \ - __LINE__, \ - __PRETTY_FUNCTION__, \ - #cond, \ - #exc, \ - exc); \ + __FILE__, __LINE__, __PRETTY_FUNCTION__, #cond, #exc, exc); \ } # else /*ifdef DEAL_II_HAVE_BUILTIN_EXPECT*/ # define AssertNothrow(cond, exc) \ { \ if (!(cond)) \ ::dealii::deal_II_exceptions::internals::issue_error_nothrow( \ - ::dealii::deal_II_exceptions::internals:: \ - abort_nothrow_on_exception, \ - __FILE__, \ - __LINE__, \ - __PRETTY_FUNCTION__, \ - #cond, \ - #exc, \ - exc); \ + __FILE__, __LINE__, __PRETTY_FUNCTION__, #cond, #exc, exc); \ } # endif /*ifdef DEAL_II_HAVE_BUILTIN_EXPECT*/ #else diff --git a/source/base/exceptions.cc b/source/base/exceptions.cc index 752980fca0..753f144b29 100644 --- a/source/base/exceptions.cc +++ b/source/base/exceptions.cc @@ -57,12 +57,12 @@ namespace deal_II_exceptions show_stacktrace = false; } - bool abort_on_exception = true; + bool allow_abort_on_exception = true; void disable_abort_on_exception() { - abort_on_exception = false; + allow_abort_on_exception = false; } } // namespace deal_II_exceptions @@ -397,61 +397,60 @@ namespace StandardExceptions } // namespace StandardExceptions #endif // DEAL_II_WITH_MPI -namespace +namespace deal_II_exceptions { - [[noreturn]] void - internal_abort(const ExceptionBase &exc) noexcept + namespace internals { - // first print the error - std::cerr << exc.what() << std::endl; - - // then bail out. if in MPI mode, bring down the entire - // house by asking the MPI system to do a best-effort - // operation at also terminating all of the other MPI - // processes. this is useful because if only one process - // runs into an assertion, then that may lead to deadlocks - // if the others don't recognize this, or at the very least - // delay their termination until they realize that their - // communication with the job that died times out. - // - // Unlike std::abort(), MPI_Abort() unfortunately doesn't break when - // running inside a debugger like GDB, so only use this strategy if - // absolutely necessary and inform the user how to use a debugger. + [[noreturn]] void + internal_abort(const ExceptionBase &exc) noexcept + { + // first print the error + std::cerr << exc.what() << std::endl; + + // then bail out. if in MPI mode, bring down the entire + // house by asking the MPI system to do a best-effort + // operation at also terminating all of the other MPI + // processes. this is useful because if only one process + // runs into an assertion, then that may lead to deadlocks + // if the others don't recognize this, or at the very least + // delay their termination until they realize that their + // communication with the job that died times out. + // + // Unlike std::abort(), MPI_Abort() unfortunately doesn't break when + // running inside a debugger like GDB, so only use this strategy if + // absolutely necessary and inform the user how to use a debugger. #ifdef DEAL_II_WITH_MPI - int is_initialized; - MPI_Initialized(&is_initialized); - if (is_initialized) - { - // do the same as in Utilities::MPI::n_mpi_processes() here, - // but without error checking to not throw again. - const int n_proc = Utilities::MPI::n_mpi_processes(MPI_COMM_WORLD); - if (n_proc > 1) - { - std::cerr - << "Calling MPI_Abort now.\n" - << "To break execution in a GDB session, execute 'break MPI_Abort' before " - << "running. You can also put the following into your ~/.gdbinit:\n" - << " set breakpoint pending on\n" - << " break MPI_Abort\n" - << " set breakpoint pending auto" << std::endl; - - MPI_Abort(MPI_COMM_WORLD, - /* return code = */ 255); - } - } + int is_initialized; + MPI_Initialized(&is_initialized); + if (is_initialized) + { + // do the same as in Utilities::MPI::n_mpi_processes() here, + // but without error checking to not throw again. + const int n_proc = Utilities::MPI::n_mpi_processes(MPI_COMM_WORLD); + if (n_proc > 1) + { + std::cerr + << "Calling MPI_Abort now.\n" + << "To break execution in a GDB session, execute 'break MPI_Abort' before " + << "running. You can also put the following into your ~/.gdbinit:\n" + << " set breakpoint pending on\n" + << " break MPI_Abort\n" + << " set breakpoint pending auto" << std::endl; + + MPI_Abort(MPI_COMM_WORLD, + /* return code = */ 255); + } + } #endif - std::abort(); - } -} // namespace + std::abort(); + } + + -namespace deal_II_exceptions -{ - namespace internals - { void do_issue_error_nothrow(const ExceptionBase &exc) noexcept { - if (dealii::deal_II_exceptions::abort_on_exception) + if (dealii::deal_II_exceptions::allow_abort_on_exception) internal_abort(exc); else { @@ -464,21 +463,9 @@ namespace deal_II_exceptions - [[noreturn]] void - abort(const ExceptionBase &exc) { - if (dealii::deal_II_exceptions::abort_on_exception) - internal_abort(exc); - else - { - // We are not allowed to abort, so just throw the error: - throw exc; - } - } - - - #ifdef DEAL_II_WITH_CUDA - std::string get_cusparse_error_string(const cusparseStatus_t error_code) + std::string + get_cusparse_error_string(const cusparseStatus_t error_code) { switch (error_code) { diff --git a/tests/parameter_handler/parameter_handler_19.debug.output b/tests/parameter_handler/parameter_handler_19.debug.output index 3af2027657..813cf501f6 100644 --- a/tests/parameter_handler/parameter_handler_19.debug.output +++ b/tests/parameter_handler/parameter_handler_19.debug.output @@ -7,12 +7,12 @@ An error occurred in file in function The violated condition was: subsection_path.size() != 0 Additional information: - (none) + You can't leave a subsection if you are already at the top level of the subsection hierarchy. -------------------------------------------------------- DEAL:: DEAL::* parse_input with missing 'end': -DEAL::ExcUnbalancedSubsections (filename, paths_message.str()) +DEAL::ExcUnbalancedSubsections(filename, paths_message.str()) There are unequal numbers of 'subsection' and 'end' statements in the parameter file . DEAL:: DEAL::Exception @@ -22,14 +22,14 @@ An error occurred in file in function The violated condition was: subsection_path.size() != 0 Additional information: - (none) + You can't leave a subsection if you are already at the top level of the subsection hierarchy. -------------------------------------------------------- DEAL:: DEAL::* Check non empty path before parse_input() DEAL:: DEAL::* Check parse_input() catches messing with path: -DEAL::ExcUnbalancedSubsections (filename, paths_message.str()) +DEAL::ExcUnbalancedSubsections(filename, paths_message.str()) There are unequal numbers of 'subsection' and 'end' statements in the parameter file . Path before loading input: subsection test diff --git a/tests/tests.h b/tests/tests.h index 25ee9f2714..a3e2266953 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -601,7 +601,6 @@ struct MPILogInitAll DEAL_II_NAMESPACE_OPEN namespace deal_II_exceptions { - extern bool abort_on_exception; extern bool show_stacktrace; } // namespace deal_II_exceptions DEAL_II_NAMESPACE_CLOSE @@ -620,8 +619,8 @@ new_tbb_assertion_handler(const char *file, std::cerr << "Detailed description: " << comment << std::endl; // Reenable abort and stacktraces: - deal_II_exceptions::abort_on_exception = true; - deal_II_exceptions::show_stacktrace = true; + deal_II_exceptions::allow_abort_on_exception = true; + deal_II_exceptions::show_stacktrace = true; // And abort with a deal.II exception: Assert(false, ExcMessage("TBB Exception, see above")); -- 2.39.5