From 1b1abdba720cfa787f8ecdbd3692db01bc306846 Mon Sep 17 00:00:00 2001 From: Pascal Merz Date: Sat, 20 Oct 2018 17:27:58 +0200 Subject: [PATCH] Failproof signal conversion When introducing the signal handlers, we decided to use scoped enums to define the different simulation signals (changes Ia90778, I05ca7a, I5dec05). In the SimulationSignal objects which actually get reduced, these are stored as signed char. When the signals get handled, these signed char are converted back to the scoped enums. Currently, this is done using a static_cast. Although in the current code, the signals get handled immediately after being reduced, and signals are only set by master, there is no formal guarantee that this is always true. If the signals get reduced multiple times, or by different ranks, the signed char stored in the SimulationSignal object could get larger than +1 / -1. In the old code, this would not lead to problems, as it was just checked that the unsigned char in the SimulationSignal was != 0 (or <0 / >0, for the stop signal). In the new code, the signal handling would fail in this case, without proper error message - the static_cast will not fail, but the following comparison to the enum values will always fail. This commit introduces a conversion function for each of the enums and explicity enum values for the StopSignal to return to the failproof behavior of the old code. Refs #1793 Change-Id: Iac8ea3946effba1797e16d8c918c7d49ce4dc828 --- src/gromacs/mdlib/checkpointhandler.cpp | 18 +++++++++++++++--- src/gromacs/mdlib/checkpointhandler.h | 2 +- src/gromacs/mdlib/resethandler.cpp | 17 +++++++++++++++-- src/gromacs/mdlib/resethandler.h | 2 +- src/gromacs/mdlib/stophandler.h | 29 ++++++++++++++++++++++++++--- 5 files changed, 58 insertions(+), 10 deletions(-) diff --git a/src/gromacs/mdlib/checkpointhandler.cpp b/src/gromacs/mdlib/checkpointhandler.cpp index cbe69ff722..e2346eba83 100644 --- a/src/gromacs/mdlib/checkpointhandler.cpp +++ b/src/gromacs/mdlib/checkpointhandler.cpp @@ -48,6 +48,18 @@ namespace gmx { +/*! \brief Convert signed char (as used by SimulationSignal) to CheckpointSignal enum + * + * Expected values are + * \p sig == 0 -- no signal + * \p sig >= 1 -- signal received + */ +static inline CheckpointSignal convertToCheckpointSignal(signed char sig) +{ + GMX_ASSERT(sig >= 0, "Unexpected checkpoint signal < 0 received"); + return sig >= 1 ? CheckpointSignal::doCheckpoint : CheckpointSignal::noSignal; +} + CheckpointHandler::CheckpointHandler( compat::not_null signal, bool simulationsShareState, @@ -74,8 +86,8 @@ void CheckpointHandler::setSignalImpl( gmx_walltime_accounting_t walltime_accounting) const { const double secondsSinceStart = walltime_accounting_get_time_since_start(walltime_accounting); - if (static_cast(signal_.set) == CheckpointSignal::noSignal && - static_cast(signal_.sig) == CheckpointSignal::noSignal && + if (convertToCheckpointSignal(signal_.set) == CheckpointSignal::noSignal && + convertToCheckpointSignal(signal_.sig) == CheckpointSignal::noSignal && (checkpointingPeriod_ == 0 || secondsSinceStart >= numberOfNextCheckpoint_ * checkpointingPeriod_ * 60.0)) { signal_.sig = static_cast(CheckpointSignal::doCheckpoint); @@ -85,7 +97,7 @@ void CheckpointHandler::setSignalImpl( void CheckpointHandler::decideIfCheckpointingThisStepImpl( bool bNS, bool bFirstStep, bool bLastStep) { - checkpointThisStep_ = (((static_cast(signal_.set) == CheckpointSignal::doCheckpoint && + checkpointThisStep_ = (((convertToCheckpointSignal(signal_.set) == CheckpointSignal::doCheckpoint && (bNS || neverUpdateNeighborlist_)) || (bLastStep && writeFinalCheckpoint_)) && !bFirstStep); diff --git a/src/gromacs/mdlib/checkpointhandler.h b/src/gromacs/mdlib/checkpointhandler.h index 361c4e73be..c9f9cfe5ac 100644 --- a/src/gromacs/mdlib/checkpointhandler.h +++ b/src/gromacs/mdlib/checkpointhandler.h @@ -73,7 +73,7 @@ namespace gmx */ enum class CheckpointSignal { - noSignal, doCheckpoint + noSignal = 0, doCheckpoint = 1 }; /*! \libinternal diff --git a/src/gromacs/mdlib/resethandler.cpp b/src/gromacs/mdlib/resethandler.cpp index 1d947e6cd9..17bd7df8e3 100644 --- a/src/gromacs/mdlib/resethandler.cpp +++ b/src/gromacs/mdlib/resethandler.cpp @@ -58,6 +58,19 @@ namespace gmx { + +/*! \brief Convert signed char (as used by SimulationSignal) to ResetSignal enum + * + * Expected values are + * \p sig == 0 -- no signal + * \p sig >= 1 -- signal received + */ +static inline ResetSignal convertToResetSignal(signed char sig) +{ + GMX_ASSERT(sig >= 0, "Unexpected reset signal < 0 received"); + return sig >= 1 ? ResetSignal::doResetCounters : ResetSignal::noSignal; +} + ResetHandler::ResetHandler( compat::not_null signal, bool simulationsShareState, @@ -132,8 +145,8 @@ bool ResetHandler::resetCountersImpl( gmx_wallcycle_t wcycle, gmx_walltime_accounting_t walltime_accounting) { - /* Reset either if signal has been passed, */ - if (static_cast(signal_.set) == ResetSignal::doResetCounters || + /* Reset either if signal has been passed, or if reset step has been reached */ + if (convertToResetSignal(signal_.set) == ResetSignal::doResetCounters || step_rel == wcycle_get_reset_counters(wcycle)) { if (pme_loadbal_is_active(pme_loadbal)) diff --git a/src/gromacs/mdlib/resethandler.h b/src/gromacs/mdlib/resethandler.h index de9fc77d8e..11a0c462f4 100644 --- a/src/gromacs/mdlib/resethandler.h +++ b/src/gromacs/mdlib/resethandler.h @@ -89,7 +89,7 @@ namespace gmx */ enum class ResetSignal { - noSignal, doResetCounters + noSignal = 0, doResetCounters = 1 }; /*! \libinternal diff --git a/src/gromacs/mdlib/stophandler.h b/src/gromacs/mdlib/stophandler.h index 2fc971f98c..59680e757f 100644 --- a/src/gromacs/mdlib/stophandler.h +++ b/src/gromacs/mdlib/stophandler.h @@ -86,9 +86,32 @@ namespace gmx */ enum class StopSignal { - noSignal, stopAtNextNSStep, stopImmediately + noSignal = 0, stopAtNextNSStep = 1, stopImmediately = -1 }; +/*! \brief Convert signed char (as used by SimulationSignal) to StopSignal enum + * + * * Expected values are + * \p sig == 0 -- no signal + * \p sig >= 1 -- stop at next NS + * \p sig <= -1 -- stop asap + */ +static inline StopSignal convertToStopSignal(signed char sig) +{ + if (sig <= -1) + { + return StopSignal::stopImmediately; + } + else if (sig >= 1) + { + return StopSignal::stopAtNextNSStep; + } + else // sig == 0 + { + return StopSignal::noSignal; + } +} + /*! \libinternal * \brief Class handling the stop signal * @@ -151,8 +174,8 @@ class StopHandler final */ bool stoppingAfterCurrentStep(bool bNS) const { - return static_cast(signal_.set) == StopSignal::stopImmediately || - (static_cast(signal_.set) == StopSignal::stopAtNextNSStep && + return convertToStopSignal(signal_.set) == StopSignal::stopImmediately || + (convertToStopSignal(signal_.set) == StopSignal::stopAtNextNSStep && (bNS || neverUpdateNeighborlist_)); } -- 2.11.4.GIT