diff --git a/src/core/bond_breakage/actions.hpp b/src/core/bond_breakage/actions.hpp index d4b4658c76..e94421d5e7 100644 --- a/src/core/bond_breakage/actions.hpp +++ b/src/core/bond_breakage/actions.hpp @@ -23,6 +23,8 @@ #include #include +#include +#include namespace BondBreakage { @@ -91,3 +93,13 @@ template <> struct hash { } }; } // namespace std + +namespace BondBreakage { + +// Variant holding any of the actions +using Action = std::variant; + +// Set of actions +using ActionSet = std::unordered_set; + +} // namespace BondBreakage diff --git a/src/core/bond_breakage/bond_breakage.cpp b/src/core/bond_breakage/bond_breakage.cpp index 296eec5282..11279a81c3 100644 --- a/src/core/bond_breakage/bond_breakage.cpp +++ b/src/core/bond_breakage/bond_breakage.cpp @@ -45,12 +45,6 @@ namespace BondBreakage { -// Variant holding any of the actions -using Action = std::variant; - -// Set of actions -using ActionSet = std::unordered_set; - /** Add a particle+bond combination to the breakage queue */ void BondBreakage::queue_breakage(int particle_id, BondPartners const &bond_partners, @@ -72,9 +66,8 @@ static auto gather_global_queue(Queue const &local_queue) { } /** @brief Constructs the actions to take for a breakage queue entry */ -static ActionSet actions_for_breakage(CellStructure const &cell_structure, - QueueEntry const &e, - BreakageSpec const &spec) { +ActionSet actions_for_breakage(CellStructure const &cell_structure, + QueueEntry const &e, BreakageSpec const &spec) { auto is_angle_bond = [](auto const &bond_partners) { return bond_partners[1]; }; // optional for second partner engaged @@ -121,7 +114,12 @@ static ActionSet actions_for_breakage(CellStructure const &cell_structure, auto vs = cell_structure.get_local_particle(e.particle_id); auto p1 = cell_structure.get_local_particle(*(e.bond_partners[0])); auto p2 = cell_structure.get_local_particle(*(e.bond_partners[1])); - if (p1 and p2) { + // The global breakage queue is broadcast to every rank, but only the rank + // that holds the central virtual site builds the action; on a rank where + // the central particle is absent (legitimate multi-rank locality), return + // an empty action set instead of dereferencing a null pointer. This mirrors + // the central-particle guard of the pair branch above. + if (vs and p1 and p2) { if (not vs->is_virtual()) { runtimeErrorMsg() << "The REVERT_BIND_AT_POINT_OF_COLLISION bond " "breakage action has to be configured for the " diff --git a/src/core/bond_breakage/bond_breakage.hpp b/src/core/bond_breakage/bond_breakage.hpp index dfdb7a5e93..a751a28c32 100644 --- a/src/core/bond_breakage/bond_breakage.hpp +++ b/src/core/bond_breakage/bond_breakage.hpp @@ -21,6 +21,7 @@ #include +#include "bond_breakage/actions.hpp" #include "system/System.hpp" #include @@ -32,6 +33,8 @@ #include #include +class CellStructure; + namespace BondBreakage { /** Stores one or two bond partners for pair/angle bonds */ @@ -67,6 +70,17 @@ struct QueueEntry { /** @brief Record bonds broken during a time step. */ using Queue = std::vector; +/** + * @brief Construct the set of delete actions for a single breakage queue entry. + * + * Each MPI rank receives the broadcast global queue and only builds actions + * for the particles it actually holds locally; referenced particles that are + * not resolvable on this rank (@ref CellStructure::get_local_particle returns + * @c nullptr) yield an empty action set. Exposed (non-static) for unit testing. + */ +ActionSet actions_for_breakage(CellStructure const &cell_structure, + QueueEntry const &e, BreakageSpec const &spec); + class BondBreakage { Queue m_queue; diff --git a/src/core/unit_tests/bond_breakage_test.cpp b/src/core/unit_tests/bond_breakage_test.cpp index cbf23e7f53..bf75fd5011 100644 --- a/src/core/unit_tests/bond_breakage_test.cpp +++ b/src/core/unit_tests/bond_breakage_test.cpp @@ -21,7 +21,28 @@ #define BOOST_TEST_DYN_LINK #include +#include "config/config.hpp" + +#include "EspressoCoreGlobalConfig.hpp" + #include "bond_breakage/actions.hpp" +#include "bond_breakage/bond_breakage.hpp" + +#include "Particle.hpp" +#include "PropagationMode.hpp" +#include "cell_system/CellStructure.hpp" +#include "cell_system/CellStructureType.hpp" +#include "system/System.hpp" + +#include +#include +#include + +struct GlobalConfig : public EspressoCoreGlobalConfig { + ~GlobalConfig() { ::System::reset_system(); } +}; + +BOOST_TEST_GLOBAL_CONFIGURATION(GlobalConfig); BOOST_AUTO_TEST_CASE(test_actions_equality) { { @@ -76,3 +97,63 @@ BOOST_AUTO_TEST_CASE(test_actions_hash_value) { BOOST_CHECK((Action{1, 2}.hash_value() != Action{1, 0}.hash_value())); } } + +#ifdef ESPRESSO_VIRTUAL_SITES_RELATIVE +// Add a virtual particle with the given id to the cell structure. +static void add_virtual_particle(CellStructure &cell_structure, int id) { + Particle p; + p.id() = id; + p.propagation() = PropagationMode::TRANS_VS_RELATIVE; + p.vs_relative().to_particle_id = id + 100; + cell_structure.add_particle(std::move(p)); +} + +/** + * Regression test for the unguarded null dereference of the central particle + * (@c vs) in the REVERT_BIND_AT_POINT_OF_COLLISION angle-bond branch of + * @ref BondBreakage::actions_for_breakage (bug-sweep #26). + * + * In a multi-rank run the global breakage queue is broadcast to every rank, so + * each rank runs @c actions_for_breakage for every entry, including entries + * whose central virtual-site particle is not local/ghost on this rank. We + * reproduce that locality condition on a single rank by populating a cell + * structure with the two partner particles but NOT the central particle: + * @c get_local_particle(central_id) returns @c nullptr while the partners + * resolve. The buggy code dereferences @c vs->is_virtual() unconditionally + * (segfault); the fixed code must return an empty action set. + */ +BOOST_AUTO_TEST_CASE(actions_for_breakage_angle_missing_central_particle) { + auto system = System::System::create(); + System::set_system(system); + system->set_cell_structure_topology(CellStructureType::NSQUARE); + auto &cell_structure = *system->cell_structure; + + auto const central_id = 0; // never added -> not resolvable on this rank + auto const partner_id_1 = 1; + auto const partner_id_2 = 2; + + // both partners are present, the central virtual site is absent + add_virtual_particle(cell_structure, partner_id_1); + add_virtual_particle(cell_structure, partner_id_2); + + BOOST_REQUIRE(cell_structure.get_local_particle(central_id) == nullptr); + BOOST_REQUIRE(cell_structure.get_local_particle(partner_id_1) != nullptr); + BOOST_REQUIRE(cell_structure.get_local_particle(partner_id_2) != nullptr); + + BondBreakage::QueueEntry entry{ + central_id, + {{std::optional{partner_id_1}, std::optional{partner_id_2}}}, + /* bond_type */ 0}; + BondBreakage::BreakageSpec spec{ + /* breakage_length */ 0., + BondBreakage::ActionType::REVERT_BIND_AT_POINT_OF_COLLISION}; + + // Must not dereference the null central particle; an absent-on-rank central + // particle is a legitimate locality condition, so no action is produced. + auto const actions = + BondBreakage::actions_for_breakage(cell_structure, entry, spec); + BOOST_CHECK_EQUAL(actions.size(), 0u); + + System::reset_system(); +} +#endif // ESPRESSO_VIRTUAL_SITES_RELATIVE