From 65618be0860042bc23ae16eff8a3b426e370d745 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Wed, 13 Sep 2023 12:53:06 -0400 Subject: [PATCH 1/5] Introduce new ERROR and ASSERT macros (to simplify the process of reporting an error and exiting) --- src/utils/error_handling.cpp | 65 +++++++++++++++++++++++++++++++++--- src/utils/error_handling.h | 49 +++++++++++++++++++++++++-- 2 files changed, 107 insertions(+), 7 deletions(-) diff --git a/src/utils/error_handling.cpp b/src/utils/error_handling.cpp index 040c1885b..dc491ebdf 100644 --- a/src/utils/error_handling.cpp +++ b/src/utils/error_handling.cpp @@ -1,12 +1,15 @@ #include "../utils/error_handling.h" #include +#include +#include #include #include +#include #ifdef MPI_CHOLLA - #include -void chexit(int code) + #include "../mpi/mpi_routines.h" +[[noreturn]] void chexit(int code) { if (code == 0) { /*exit normally*/ @@ -20,14 +23,14 @@ void chexit(int code) } } #else /*MPI_CHOLLA*/ -void chexit(int code) +[[noreturn]] void chexit(int code) { /*exit using code*/ exit(code); } #endif /*MPI_CHOLLA*/ -void Check_Configuration(parameters const &P) +void Check_Configuration(parameters const& P) { // General Checks // ============== @@ -56,7 +59,7 @@ void Check_Configuration(parameters const &P) #endif // Only one integrator check // Check the boundary conditions - auto Check_Boundary = [](int const &boundary, std::string const &direction) { + auto Check_Boundary = [](int const& boundary, std::string const& direction) { bool is_allowed_bc = boundary >= 0 and boundary <= 4; std::string const error_message = "WARNING: Possibly invalid boundary conditions for direction: " + direction + @@ -126,3 +129,55 @@ void Check_Configuration(parameters const &P) #endif // MHD } + +// NOLINTNEXTLINE(cert-dcl50-cpp) +[[noreturn]] void Abort_With_Err_(const char* func_name, const char* file_name, int line_num, const char* msg, ...) +{ + // considerations when using MPI: + // - all processes must execute this function to catch errors that happen on + // just one process + // - to handle cases where all processes encounter the same error, we + // pre-buffer the error message (so that the output remains legible) + + // since we are aborting, it's OK that this isn't the most optimized + + // prepare some info for the error message header + const char* santized_func_name = (func_name == nullptr) ? "{unspecified}" : func_name; + + std::string proc_info = +#ifdef MPI_CHOLLA + std::to_string(procID) + " / " + std::to_string(nproc) + " (using MPI)"; +#else + "0 / 1 (NOT using MPI)" +#endif + + // prepare the formatted message + std::vector msg_buf; + if (msg == nullptr) { + msg_buf = std::vector(80); + std::snprintf(msg_buf.data(), msg_buf.size(), "{nullptr encountered instead of error message}"); + } else { + std::va_list args, args_copy; + va_start(args, msg); + va_copy(args_copy, args); + + std::size_t msg_len = std::vsnprintf(nullptr, 0, msg, args) + 1; + va_end(args); + + msg_buf = std::vector(msg_len); + std::vsnprintf(msg_buf.data(), msg_len, msg, args); + va_end(args_copy); + } + + // now write the error and exit + std::fprintf(stderr, + "@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@\n" + "Error occurred in %s on line %d\n" + "Function: %s\n" + "Rank: %s\n" + "Message: %s\n" + "@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@\n", + file_name, line_num, santized_func_name, proc_info.data(), msg_buf.data()); + std::fflush(stderr); // may be unnecessary for stderr + chexit(1); +} \ No newline at end of file diff --git a/src/utils/error_handling.h b/src/utils/error_handling.h index d539f0e50..7fe450cba 100644 --- a/src/utils/error_handling.h +++ b/src/utils/error_handling.h @@ -3,12 +3,57 @@ #include #include "../global/global.h" -void chexit(int code); +[[noreturn]] void chexit(int code); /*! * \brief Check that the Cholla configuration and parameters don't have any significant errors. Mostly compile time * checks. * */ -void Check_Configuration(parameters const &P); +void Check_Configuration(parameters const& P); + +/*! + * \brief helper function that prints an error message & aborts the program (in + * an MPI-safe way). Commonly invoked through a macro. + * + */ +[[noreturn]] void Abort_With_Err_(const char* func_name, const char* file_name, int line_num, const char* msg, ...); + +/*! + * \brief print an error-message (with printf formatting) & abort the program. + * + * This macro should be treated as a function with the signature: + * [[noreturn]] void ERROR(const char* func_name, const char* msg, ...); + * + * - The 1st arg is the name of the function where it's called + * - The 2nd arg is printf-style format argument specifying the error message + * - The remaining args arguments are used to format error message + * + * \note + * the ``msg`` string is part of the variadic args so that there is always + * at least 1 variadic argument (even in cases when ``msg`` doesn't format + * any arguments). There is no way around this until C++ 20. + */ +#define ERROR(func_name, ...) Abort_With_Err_(func_name, __FILE__, __LINE__, __VA_ARGS__) + +/*! + * \brief if the condition is false, print an error-message (with printf + * formatting) & abort the program. + * + * This macro should be treated as a function with the signature: + * [[noreturn]] void ASSERT(bool cond, const char* func_name, const char* msg, ...); + * + * - The 1st arg is a boolean condition. When true, this does noth + * - The 2nd arg is the name of the function where it's called + * - The 3rd arg is printf-style format argument specifying the error message + * - The remaining args arguments are used to format error message + * + * \note + * the behavior is independent of the ``NDEBUG`` macro + */ +#define ASSERT(cond, func_name, ...) \ + if (not(cond)) { \ + Abort_With_Err_(func_name, __FILE__, __LINE__, __VA_ARGS__); \ + } + #endif /*ERROR_HANDLING_CHOLLA_H*/ From f8db7c890331a917238710be82ddc63b4d95b71e Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 14 Sep 2023 17:58:35 -0400 Subject: [PATCH 2/5] start using the ERROR and ASSERT macros in a few places (partially to confirm that they actually work) - moved the check on the value of ``gama``. Previously there was a logical bug in the check and the check was performed before the variable was initialized. --- src/global/global.cpp | 4 +++- src/io/io.cpp | 9 ++++----- src/utils/error_handling.cpp | 12 ++++-------- 3 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/global/global.cpp b/src/global/global.cpp index 2cdb508d1..ee1776922 100644 --- a/src/global/global.cpp +++ b/src/global/global.cpp @@ -12,7 +12,8 @@ #include -#include "../io/io.h" //defines chprintf +#include "../io/io.h" //defines chprintf +#include "../utils/error_handling.h" // defines ASSERT /* Global variables */ Real gama; // Ratio of specific heats @@ -33,6 +34,7 @@ void Set_Gammas(Real gamma_in) { // set gamma gama = gamma_in; + ASSERT(gama > 1.0, "Set_Gammas", "Gamma must be greater than one."); } /*! \fn double get_time(void) diff --git a/src/io/io.cpp b/src/io/io.cpp index c9817cc6d..9edf6c4b4 100644 --- a/src/io/io.cpp +++ b/src/io/io.cpp @@ -2788,11 +2788,10 @@ void Ensure_Outdir_Exists(std::string outdir) // to a directory (it's unclear from docs whether err-code is set in that // case) if (err_code or not std::filesystem::is_directory(without_file_prefix)) { - chprintf( - "something went wrong while trying to create the path to the " - "output-dir: %s\n", - outdir.c_str()); - chexit(1); + ERROR("Ensure_Outdir_Exists", + "something went wrong while trying to create the path to the " + "output-dir: %s", + outdir.c_str()); } } } diff --git a/src/utils/error_handling.cpp b/src/utils/error_handling.cpp index dc491ebdf..c64ee362b 100644 --- a/src/utils/error_handling.cpp +++ b/src/utils/error_handling.cpp @@ -61,11 +61,10 @@ void Check_Configuration(parameters const& P) // Check the boundary conditions auto Check_Boundary = [](int const& boundary, std::string const& direction) { bool is_allowed_bc = boundary >= 0 and boundary <= 4; - std::string const error_message = - "WARNING: Possibly invalid boundary conditions for direction: " + direction + - " flag: " + std::to_string(boundary) + - ". Must select between 0 (no boundary), 1 (periodic), 2 (reflective), 3 (transmissive), 4 (custom), 5 (mpi)."; - assert(is_allowed_bc && error_message.c_str()); + ASSERT(is_allowed_bc, "Check_Configuration", + "WARNING: Possibly invalid boundary conditions for direction: %s flag: %d. " + "Must select between 0 (no boundary), 1 (periodic), 2 (reflective), 3 (transmissive), 4 (custom), 5 (mpi).", + direction.c_str(), boundary); }; Check_Boundary(P.xl_bcnd, "xl_bcnd"); Check_Boundary(P.xu_bcnd, "xu_bcnd"); @@ -85,9 +84,6 @@ void Check_Configuration(parameters const& P) #endif //! PRECISION static_assert(PRECISION == 2, "PRECISION must be 2. Single precision is not currently supported"); - // Check that gamma, the ratio of specific heats, is greater than 1 - assert(::gama <= 1.0 and "Gamma must be greater than one."); - // MHD Checks // ========== #ifdef MHD From 0b25556f741c43a1c4b59886c3dc3c979612d0fd Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 15 Sep 2023 10:50:49 -0400 Subject: [PATCH 3/5] improve the ergonomics of ERROR and ASSERT --- src/global/global.cpp | 2 +- src/io/io.cpp | 8 ++++---- src/utils/error_handling.cpp | 2 +- src/utils/error_handling.h | 32 ++++++++++++++++++++++---------- 4 files changed, 28 insertions(+), 16 deletions(-) diff --git a/src/global/global.cpp b/src/global/global.cpp index ee1776922..65b35a83f 100644 --- a/src/global/global.cpp +++ b/src/global/global.cpp @@ -34,7 +34,7 @@ void Set_Gammas(Real gamma_in) { // set gamma gama = gamma_in; - ASSERT(gama > 1.0, "Set_Gammas", "Gamma must be greater than one."); + ASSERT(gama > 1.0, "Gamma must be greater than one."); } /*! \fn double get_time(void) diff --git a/src/io/io.cpp b/src/io/io.cpp index 9edf6c4b4..88e460e3b 100644 --- a/src/io/io.cpp +++ b/src/io/io.cpp @@ -2788,10 +2788,10 @@ void Ensure_Outdir_Exists(std::string outdir) // to a directory (it's unclear from docs whether err-code is set in that // case) if (err_code or not std::filesystem::is_directory(without_file_prefix)) { - ERROR("Ensure_Outdir_Exists", - "something went wrong while trying to create the path to the " - "output-dir: %s", - outdir.c_str()); + ERROR( + "something went wrong while trying to create the path to the " + "output-dir: %s", + outdir.c_str()); } } } diff --git a/src/utils/error_handling.cpp b/src/utils/error_handling.cpp index c64ee362b..04439c6c7 100644 --- a/src/utils/error_handling.cpp +++ b/src/utils/error_handling.cpp @@ -61,7 +61,7 @@ void Check_Configuration(parameters const& P) // Check the boundary conditions auto Check_Boundary = [](int const& boundary, std::string const& direction) { bool is_allowed_bc = boundary >= 0 and boundary <= 4; - ASSERT(is_allowed_bc, "Check_Configuration", + ASSERT(is_allowed_bc, "WARNING: Possibly invalid boundary conditions for direction: %s flag: %d. " "Must select between 0 (no boundary), 1 (periodic), 2 (reflective), 3 (transmissive), 4 (custom), 5 (mpi).", direction.c_str(), boundary); diff --git a/src/utils/error_handling.h b/src/utils/error_handling.h index 7fe450cba..2406b4f49 100644 --- a/src/utils/error_handling.h +++ b/src/utils/error_handling.h @@ -19,14 +19,27 @@ void Check_Configuration(parameters const& P); */ [[noreturn]] void Abort_With_Err_(const char* func_name, const char* file_name, int line_num, const char* msg, ...); +/* __CHOLLA_PRETTY_FUNC__ is a magic constant like __LINE__ or __FILE__ that + * provides the name of the current function. + * - The C++11 standard requires that __func__ is provided on all platforms, but + * that only provides limited information (just the name of the function). + * - Where available, we prefer to use compiler-specific features that provide + * more information about the function (like the scope of the function & the + * the function signature). + */ +#ifdef __GNUG__ + #define __CHOLLA_PRETTY_FUNC__ __PRETTY_FUNCTION__ +#else + #define __CHOLLA_PRETTY_FUNC__ __func__ +#endif + /*! * \brief print an error-message (with printf formatting) & abort the program. * * This macro should be treated as a function with the signature: - * [[noreturn]] void ERROR(const char* func_name, const char* msg, ...); + * [[noreturn]] void ERROR(const char* msg, ...); * - * - The 1st arg is the name of the function where it's called - * - The 2nd arg is printf-style format argument specifying the error message + * - The 1st arg is printf-style format argument specifying the error message * - The remaining args arguments are used to format error message * * \note @@ -34,26 +47,25 @@ void Check_Configuration(parameters const& P); * at least 1 variadic argument (even in cases when ``msg`` doesn't format * any arguments). There is no way around this until C++ 20. */ -#define ERROR(func_name, ...) Abort_With_Err_(func_name, __FILE__, __LINE__, __VA_ARGS__) +#define ERROR(...) Abort_With_Err_(__CHOLLA_PRETTY_FUNC__, __FILE__, __LINE__, __VA_ARGS__) /*! * \brief if the condition is false, print an error-message (with printf * formatting) & abort the program. * * This macro should be treated as a function with the signature: - * [[noreturn]] void ASSERT(bool cond, const char* func_name, const char* msg, ...); + * [[noreturn]] void ASSERT(bool cond, const char* msg, ...); * * - The 1st arg is a boolean condition. When true, this does noth - * - The 2nd arg is the name of the function where it's called - * - The 3rd arg is printf-style format argument specifying the error message + * - The 2nd arg is printf-style format argument specifying the error message * - The remaining args arguments are used to format error message * * \note * the behavior is independent of the ``NDEBUG`` macro */ -#define ASSERT(cond, func_name, ...) \ - if (not(cond)) { \ - Abort_With_Err_(func_name, __FILE__, __LINE__, __VA_ARGS__); \ +#define ASSERT(cond, ...) \ + if (not(cond)) { \ + Abort_With_Err_(__CHOLLA_PRETTY_FUNC__, __FILE__, __LINE__, __VA_ARGS__); \ } #endif /*ERROR_HANDLING_CHOLLA_H*/ From 000cfbdfd93a4f15e8af70ecd9eb13944565998c Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Tue, 19 Sep 2023 11:15:01 -0400 Subject: [PATCH 4/5] addressed PR comments --- src/utils/error_handling.cpp | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/utils/error_handling.cpp b/src/utils/error_handling.cpp index 04439c6c7..4488a7fa4 100644 --- a/src/utils/error_handling.cpp +++ b/src/utils/error_handling.cpp @@ -5,7 +5,6 @@ #include #include #include -#include #ifdef MPI_CHOLLA #include "../mpi/mpi_routines.h" @@ -140,28 +139,35 @@ void Check_Configuration(parameters const& P) // prepare some info for the error message header const char* santized_func_name = (func_name == nullptr) ? "{unspecified}" : func_name; - std::string proc_info = #ifdef MPI_CHOLLA - std::to_string(procID) + " / " + std::to_string(nproc) + " (using MPI)"; + std::string proc_info = std::to_string(procID) + " / " + std::to_string(nproc) + " (using MPI)"; #else - "0 / 1 (NOT using MPI)" + std::string proc_info = "0 / 1 (NOT using MPI)"; #endif // prepare the formatted message - std::vector msg_buf; + std::string msg_buf; if (msg == nullptr) { - msg_buf = std::vector(80); - std::snprintf(msg_buf.data(), msg_buf.size(), "{nullptr encountered instead of error message}"); + msg_buf = "{nullptr encountered instead of error message}"; } else { std::va_list args, args_copy; va_start(args, msg); va_copy(args_copy, args); - std::size_t msg_len = std::vsnprintf(nullptr, 0, msg, args) + 1; + std::size_t bufsize_without_terminator = std::vsnprintf(nullptr, 0, msg, args); va_end(args); - msg_buf = std::vector(msg_len); - std::vsnprintf(msg_buf.data(), msg_len, msg, args); + // NOTE: starting in C++17 it's possible to mutate msg_buf by mutating msg_buf.data() + + // we initialize a msg_buf with size == bufsize_without_terminator (filled with ' ' chars) + // - msg_buf.data() returns a ptr with msg_buf.size() + 1 characters. We are allowed to + // mutate any of the first msg_buf.size() characters. The entry at + // msg_buf.data()[msg_buf.size()] is initially '\0' (& it MUST remain equal to '\0') + // - the 2nd argument of std::vsnprintf is the size of the output buffer. We NEED to + // include the terminator character in this argument, otherwise the formatted message + // will be truncated + msg_buf = std::string(bufsize_without_terminator, ' '); + std::vsnprintf(msg_buf.data(), bufsize_without_terminator + 1, msg, args_copy); va_end(args_copy); } From 3ebd636bccfa3898a1fd02ee42c8ff0f097f374e Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Tue, 19 Sep 2023 11:38:44 -0400 Subject: [PATCH 5/5] Rename the ``ASSERT`` and ``ERROR`` to ``CHOLLA_ASSERT`` and ``CHOLLA_ERROR`` --- src/global/global.cpp | 2 +- src/io/io.cpp | 2 +- src/utils/error_handling.cpp | 9 +++++---- src/utils/error_handling.h | 8 ++++---- 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/global/global.cpp b/src/global/global.cpp index 65b35a83f..2aa5792fe 100644 --- a/src/global/global.cpp +++ b/src/global/global.cpp @@ -34,7 +34,7 @@ void Set_Gammas(Real gamma_in) { // set gamma gama = gamma_in; - ASSERT(gama > 1.0, "Gamma must be greater than one."); + CHOLLA_ASSERT(gama > 1.0, "Gamma must be greater than one."); } /*! \fn double get_time(void) diff --git a/src/io/io.cpp b/src/io/io.cpp index 88e460e3b..09ffd0d17 100644 --- a/src/io/io.cpp +++ b/src/io/io.cpp @@ -2788,7 +2788,7 @@ void Ensure_Outdir_Exists(std::string outdir) // to a directory (it's unclear from docs whether err-code is set in that // case) if (err_code or not std::filesystem::is_directory(without_file_prefix)) { - ERROR( + CHOLLA_ERROR( "something went wrong while trying to create the path to the " "output-dir: %s", outdir.c_str()); diff --git a/src/utils/error_handling.cpp b/src/utils/error_handling.cpp index 4488a7fa4..5a7bad073 100644 --- a/src/utils/error_handling.cpp +++ b/src/utils/error_handling.cpp @@ -60,10 +60,11 @@ void Check_Configuration(parameters const& P) // Check the boundary conditions auto Check_Boundary = [](int const& boundary, std::string const& direction) { bool is_allowed_bc = boundary >= 0 and boundary <= 4; - ASSERT(is_allowed_bc, - "WARNING: Possibly invalid boundary conditions for direction: %s flag: %d. " - "Must select between 0 (no boundary), 1 (periodic), 2 (reflective), 3 (transmissive), 4 (custom), 5 (mpi).", - direction.c_str(), boundary); + CHOLLA_ASSERT(is_allowed_bc, + "WARNING: Possibly invalid boundary conditions for direction: %s flag: %d. Must " + "select between 0 (no boundary), 1 (periodic), 2 (reflective), 3 (transmissive), " + "4 (custom), 5 (mpi).", + direction.c_str(), boundary); }; Check_Boundary(P.xl_bcnd, "xl_bcnd"); Check_Boundary(P.xu_bcnd, "xu_bcnd"); diff --git a/src/utils/error_handling.h b/src/utils/error_handling.h index 2406b4f49..4db749881 100644 --- a/src/utils/error_handling.h +++ b/src/utils/error_handling.h @@ -37,7 +37,7 @@ void Check_Configuration(parameters const& P); * \brief print an error-message (with printf formatting) & abort the program. * * This macro should be treated as a function with the signature: - * [[noreturn]] void ERROR(const char* msg, ...); + * [[noreturn]] void CHOLLA_ERROR(const char* msg, ...); * * - The 1st arg is printf-style format argument specifying the error message * - The remaining args arguments are used to format error message @@ -47,14 +47,14 @@ void Check_Configuration(parameters const& P); * at least 1 variadic argument (even in cases when ``msg`` doesn't format * any arguments). There is no way around this until C++ 20. */ -#define ERROR(...) Abort_With_Err_(__CHOLLA_PRETTY_FUNC__, __FILE__, __LINE__, __VA_ARGS__) +#define CHOLLA_ERROR(...) Abort_With_Err_(__CHOLLA_PRETTY_FUNC__, __FILE__, __LINE__, __VA_ARGS__) /*! * \brief if the condition is false, print an error-message (with printf * formatting) & abort the program. * * This macro should be treated as a function with the signature: - * [[noreturn]] void ASSERT(bool cond, const char* msg, ...); + * [[noreturn]] void CHOLLA_ASSERT(bool cond, const char* msg, ...); * * - The 1st arg is a boolean condition. When true, this does noth * - The 2nd arg is printf-style format argument specifying the error message @@ -63,7 +63,7 @@ void Check_Configuration(parameters const& P); * \note * the behavior is independent of the ``NDEBUG`` macro */ -#define ASSERT(cond, ...) \ +#define CHOLLA_ASSERT(cond, ...) \ if (not(cond)) { \ Abort_With_Err_(__CHOLLA_PRETTY_FUNC__, __FILE__, __LINE__, __VA_ARGS__); \ }