Fix memory based on PR "less event data" (#2407)

Fix bug in test suite found by ASAN/TSAN. --------- Co-authored-by: boeschf <[email protected]>
arbor-sim · Sep 19, 2024 · 502f385 · 502f385
1 parent 9c7312e
commit 502f385
Show file tree

Hide file tree

Showing 5 changed files with 51 additions and 46 deletions.
diff --git a/.github/workflows/sanitize.yml b/.github/workflows/sanitize.yml
@@ -5,6 +5,9 @@ on:
     branches: [ master ]
     paths-ignore: 'doc/**'
 
+  pull_request:
+    branches: [ master ]
+
   schedule:
     - cron: '0 2 * * 0' # run at 2 AM every sunday
 
@@ -61,7 +64,7 @@ jobs:
           mkdir build
           cd build
           export SAN="-fsanitize=${{ matrix.sanitizer }} -fno-omit-frame-pointer"
-          cmake .. -GNinja -DCMAKE_BUILD_TYPE=debug -DCMAKE_CXX_FLAGS="$SAN" -DCMAKE_C_FLAGS="$SAN" -DCMAKE_EXE_LINKER_FLAGS="$SAN" -DCMAKE_MODULE_LINKER_FLAGS="$SAN" -DCMAKE_CXX_COMPILER=$CXX -DCMAKE_C_COMPILER=$CC -DARB_VECTORIZE=${{ matrix.simd }} -DARB_WITH_MPI=OFF -DARB_WITH_PYTHON=ON -DPython3_EXECUTABLE=`which python`
+          cmake .. -GNinja -DCMAKE_BUILD_TYPE=debug -DCMAKE_CXX_FLAGS="$SAN" -DCMAKE_C_FLAGS="$SAN" -DCMAKE_EXE_LINKER_FLAGS="$SAN" -DCMAKE_MODULE_LINKER_FLAGS="$SAN" -DARB_WITH_ASSERTIONS=ON -DCMAKE_CXX_COMPILER=$CXX -DCMAKE_C_COMPILER=$CC -DARB_VECTORIZE=${{ matrix.simd }} -DARB_WITH_MPI=OFF -DARB_WITH_PYTHON=ON -DPython3_EXECUTABLE=`which python`
           ninja -j4 -v tests examples pyarb
           cd -
       - name: Run unit tests

diff --git a/arbor/backends/event.hpp b/arbor/backends/event.hpp
@@ -40,7 +40,7 @@ struct deliverable_event {
     target_handle handle;
 
     deliverable_event() = default;
-    deliverable_event(time_type time, target_handle handle, float weight):
+    constexpr deliverable_event(time_type time, target_handle handle, float weight) noexcept:
         time(time), weight(weight), handle(handle) {}
 
     ARB_SERDES_ENABLE(deliverable_event, time, weight, handle);

diff --git a/arbor/backends/event_stream_base.hpp b/arbor/backends/event_stream_base.hpp
@@ -71,30 +71,32 @@ struct event_stream_base {
                        const std::vector<std::size_t>& divs,
                        const timestep_range& steps,
                        std::unordered_map<unsigned, EventStream>& streams) {
-        auto n_steps = steps.size();
+        arb_assert(lanes.size() < divs.size());
 
+        auto n_steps = steps.size();
         std::unordered_map<unsigned, std::vector<std::size_t>> dt_sizes;
         for (auto& [k, v]: streams) {
             v.clear();
             dt_sizes[k].resize(n_steps, 0);
         }
 
         auto cell = 0;
-        for (auto& lane: lanes) {
+        for (const auto& lane: lanes) {
             auto div = divs[cell];
+            ++cell;
             arb_size_type step = 0;
-            for (auto evt: lane) {
+            for (const auto& evt: lane) {
                 auto time = evt.time;
                 auto weight = evt.weight;
                 auto target = evt.target;
                 while(step < n_steps && time >= steps[step].t_end()) ++step;
                 // Events coinciding with epoch's upper boundary belong to next epoch
                 if (step >= n_steps) break;
-                auto& handle = handles[div + target];
+                arb_assert(div + target < handles.size());
+                const auto& handle = handles[div + target];
                 streams[handle.mech_id].ev_data_.push_back({handle.mech_index, weight});
                 dt_sizes[handle.mech_id][step]++;
             }
-            ++cell;
         }
 
         for (auto& [id, stream]: streams) {

diff --git a/test/unit/test_probe.cpp b/test/unit/test_probe.cpp
@@ -358,61 +358,60 @@ void run_expsyn_g_cell_probe_test(context ctx) {
     //  * A multiplicity that matches the number of probes sharing a CV if synapses
     //    were coalesced.
     //  * A raw handle that sees an event sent to the corresponding target,
+    std::vector gids = {0u, 1u};
+    auto policy = cv_policy_fixed_per_branch(3);
 
-    cv_policy policy = cv_policy_fixed_per_branch(3);
-
-    auto m  = make_y_morphology();
-    arb::decor d;
-    d.set_default(policy);
-
+    arb::decor decor;
+    decor.set_default(policy);
     std::unordered_map<cell_lid_type, mlocation> expsyn_target_loc_map;
-
     unsigned n_expsyn = 0;
     for (unsigned bid = 0; bid<3u; ++bid) {
         for (unsigned j = 0; j<10; ++j) {
             auto idx = (bid*10+j)*2;
             mlocation expsyn_loc{bid, 0.1*j};
-            d.place(expsyn_loc, synapse("expsyn"), "syn"+std::to_string(idx));
+            decor.place(expsyn_loc, synapse("expsyn"), "syn"+std::to_string(idx));
             expsyn_target_loc_map[2*n_expsyn] = expsyn_loc;
-            d.place(mlocation{bid, 0.1*j+0.05}, synapse("exp2syn"), "syn"+std::to_string(idx+1));
+            decor.place(mlocation{bid, 0.1*j+0.05}, synapse("exp2syn"), "syn"+std::to_string(idx+1));
             ++n_expsyn;
         }
     }
 
-    std::vector<cable_cell> cells(2, arb::cable_cell(m, d));
+    std::vector cells(2, arb::cable_cell(make_y_morphology(), decor));
 
-    auto run_test = [&](bool coalesce_synapses) {
-        cable1d_recipe rec(cells, coalesce_synapses);
+    // Weight for target (gid, lid)
+    auto weight = [](auto gid, auto tgt) -> float { return tgt + 100*gid; };
 
-        rec.add_probe(0, "expsyn-g", cable_probe_point_state_cell{"expsyn", "g"});
-        rec.add_probe(1, "expsyn-g", cable_probe_point_state_cell{"expsyn", "g"});
+    // Manually send an event to each expsyn synapse and integrate for a tiny time step.
+    // Set up one stream per cell.
+    std::vector events(gids.size(), pse_vector{});
+    for (auto gid: gids) {
+        for (auto target_id: util::keys(expsyn_target_loc_map)) {
+            events[gid].emplace_back(target_id, 0., weight(gid, target_id));
+        }
+    }
 
-        fvm_cell lcell(*ctx);
-        auto fvm_info = lcell.initialize({0, 1}, rec);
-        const auto& probe_map = fvm_info.probe_map;
-        const auto& targets = lcell.target_handles_;
+    // Independently get cv geometry to compute CV indices.
+    cv_geometry geom;
+    for (const auto& cell: cells) {
+        append(geom, cv_geometry(cell, policy.cv_boundary_points(cell)));
+    }
 
-        // Send an event to each expsyn synapse with a weight = target+100*cell_gid, and
-        // integrate for a tiny time step.
-        std::vector<pse_vector> events;
-        events.resize(targets.size());
-        for (unsigned gid: {0u, 1u}) {
-            for (auto target_id: util::keys(expsyn_target_loc_map)) {
-                events[gid].emplace_back(target_id, 0., float(target_id+100*gid));
-            }
+    // Actual test;
+    auto run_test = [&](bool coalesce_synapses) {
+        cable1d_recipe rec(cells, coalesce_synapses);
+        for (auto gid: gids) {
+            rec.add_probe(gid, "expsyn-g", cable_probe_point_state_cell{"expsyn", "g"});
         }
 
-        auto lanes = util::subrange_view(events, 0, events.size());
-        (void)lcell.integrate({1e-5, 1e-5}, lanes, {});
-
-        // Independently get cv geometry to compute CV indices.
+        fvm_cell lcell(*ctx);
+        auto fvm_info = lcell.initialize(gids, rec);
+        const auto& probe_map = fvm_info.probe_map;
 
-        cv_geometry geom(cells[0], policy.cv_boundary_points(cells[0]));
-        append(geom, cv_geometry(cells[1], policy.cv_boundary_points(cells[1])));
+        (void)lcell.integrate({1e-5, 1e-5}, util::subrange_view(events, 0, events.size()), {});
 
         ASSERT_EQ(2u, probe_map.size());
-        for (unsigned i: {0u, 1u}) {
-            const auto* h_ptr = std::get_if<fvm_probe_multi>(&probe_map.data_on({i, "expsyn-g"}).front()->info);
+        for (auto gid: gids) {
+            const auto* h_ptr = std::get_if<fvm_probe_multi>(&probe_map.data_on({gid, "expsyn-g"}).front()->info);
             ASSERT_TRUE(h_ptr);
 
             const auto* m_ptr = std::get_if<std::vector<cable_probe_point_info>>(&h_ptr->metadata);
@@ -424,21 +423,22 @@ void run_expsyn_g_cell_probe_test(context ctx) {
             ASSERT_EQ(h.raw_handles.size(), m.size());
             ASSERT_EQ(n_expsyn, m.size());
 
+            const auto n_targets = lcell.target_handles_.size();
             std::vector<double> expected_coalesced_cv_value(geom.size());
-            std::vector<double> expected_uncoalesced_value(targets.size());
+            std::vector<double> expected_uncoalesced_value(n_targets);
 
-            std::vector<double> target_cv(targets.size(), (unsigned)-1);
+            std::vector<double> target_cv(n_targets, (unsigned)-1);
             std::unordered_map<arb_size_type, unsigned> cv_expsyn_count;
 
             for (unsigned j = 0; j<n_expsyn; ++j) {
                 ASSERT_EQ(1u, expsyn_target_loc_map.count(m[j].target));
                 EXPECT_EQ(expsyn_target_loc_map.at(m[j].target), m[j].loc);
 
-                auto cv = geom.location_cv(i, m[j].loc, cv_prefer::cv_nonempty);
+                auto cv = geom.location_cv(gid, m[j].loc, cv_prefer::cv_nonempty);
                 target_cv[j] = cv;
                 ++cv_expsyn_count[cv];
 
-                double event_weight = m[j].target+100*i;
+                double event_weight = weight(gid, m[j].target);
                 expected_uncoalesced_value[j] = event_weight;
                 expected_coalesced_cv_value[cv] += event_weight;
             }

diff --git a/test/unit/test_synapses.cpp b/test/unit/test_synapses.cpp
@@ -152,7 +152,7 @@ TEST(synapses, syn_basic_state) {
     std::vector<pse_vector> events{{{0, 0.0, 3.14f}, {1, 0.0, 1.41f}, {2, 0.0, 2.71f}, {3, 0.0, 0.07f}}};
     auto lanes = event_lane_subrange(events.begin(), events.end());
     std::vector<target_handle> handles{{0, 1}, {0, 3}, {1, 0}, {1, 2}};
-    std::vector<size_t> divs{0};
+    std::vector<size_t> divs{0, handles.size()};
     state.begin_epoch(lanes, {}, dts, handles, divs);
     state.mark_events();