From 2dd281ad49c263ccfafd17d8747888531a733f2a Mon Sep 17 00:00:00 2001 From: Dan Negrut Date: Mon, 10 Aug 2026 13:50:43 -0500 Subject: [PATCH 1/2] Fix DualArray resize leaving grown device tail uninitialized (pinned variant) In the pinned-memory DualArray, resize(n, val) filled only the host vector: resizeHost(n, val) applies the fill, then resizeDevice(n) either reallocates and copies forward pre-existing elements only, or returns early when capacity already suffices. Either way the device range [old_size, n) was never written, while all 104 call sites pass a fill value in the reasonable belief that it defines the new elements. The same asymmetry existed in the no-value flavor, whose host side is value-initialized by std::vector::resize. Consequence in practice: contactEventArraysResize grows the per-contact arrays mid-simulation once the true contact count exceeds the initial one-slot-per-sphere sizing. Candidate pairs demoted to NOT_A_CONTACT in calculateContactForces never have their contactPointGeometryA/B written (the demotion is local to the kernel by design, so slots below the live count legitimately go unwritten in a step), and forceToAcc consumed uninitialized device memory for those slots: cross(garbage, 0) is 0 for finite garbage but NaN when the bits decode to NaN or Inf, and the NaN enters angular acceleration via atomicAdd. The kT-side velocity health check cannot see it because a CUB max reduction discards NaN operands (IEEE comparisons against NaN are false), so affected runs corrupted silently and then stalled: 6 of 40 async bed-settling runs blew up, with 60-190 s control steps and GB-scale memory growth. Evidence: compute-sanitizer --tool initcheck on a settling-bed repro attributes the reads to forceToAcc by name. Stock totals are large and run-dependent (two async runs: 23,357 and 15,226 total errors; the run counted by class had 14,893 device-side uninitialized reads). With this fix, and DEME_INIT_CNT_MULTIPLIER kept at 1, device-side uninitialized reads are 0 in all three configurations measured (async, synchronous with a fixed contact margin, synchronous without); each reports 20 total errors, all belonging to a separate pre-existing host-side defect in DEMSolverScratchData::allocateScratchSpace that this change neither causes nor closes. memcheck reports no invalid access attributable to this change. It did surface, once in five runs of the fixed build, an out-of-bounds read in rearrangeContactWildcards; that array is dispensed by DeviceVectorPool (built on DeviceArray) while this change touches DualArray only, and the defect is reported upstream separately. Two independent checks corroborate the mechanism and the neutrality of the fix. Avoiding growth altogether via an 8x initial contact buffer drove failures from 6/40 to 0/40 (Fisher exact p = 0.026), which is the mechanism confirmed by a route that does not touch this code. And settled-bed physics is unchanged within measurement resolution: 41 interleaved runs, solid fraction difference 1.2 sigma against a ~0.3% single-run scatter. The fix pushes the freshly filled host tail to the device after resizeDevice, in both resize flavors, using the existing partial toDevice(start, count). Only the grown tail is copied: device data in [0, old_size) remains authoritative and untouched. The managed-memory variant needs no code change (one allocation serves both sides); its copy-pasted "fills host values only" comment is corrected. Co-Authored-By: Claude Opus 5 --- src/core/utils/DataMigrationHelper.hpp | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/src/core/utils/DataMigrationHelper.hpp b/src/core/utils/DataMigrationHelper.hpp index 219a66e8..d036ccd6 100644 --- a/src/core/utils/DataMigrationHelper.hpp +++ b/src/core/utils/DataMigrationHelper.hpp @@ -231,15 +231,31 @@ class DualArray : private NonCopyable { void resize(size_t n) { assert(m_host_vec_ptr == m_pinned_vec.get() && "resize() requires internal host ownership"); + const size_t old_size = size(); resizeHost(n); resizeDevice(n); + // New elements must be defined on BOTH residences. resizeHost value-initialises + // the host tail, but resizeDevice never initialises grown elements: on + // reallocation it copies forward only pre-existing data, and when capacity + // already suffices it returns without touching memory (so the tail would hold + // stale bytes). Push the host tail down so device [old_size, n) is defined. + if (n > old_size) { + toDevice(old_size, n - old_size); + } } - // This resize flavor fills host values only! void resize(size_t n, const T& val) { assert(m_host_vec_ptr == m_pinned_vec.get() && "resize() requires internal host ownership"); + const size_t old_size = size(); resizeHost(n, val); resizeDevice(n); + // Same as resize(n) above: without this push, the device tail [old_size, n) is + // undefined on both growth paths while every caller reasonably believes it holds + // `val`. Only the grown tail is copied; device data in [0, old_size) stays + // authoritative and is never overwritten by host state here. + if (n > old_size) { + toDevice(old_size, n - old_size); + } } void resizeHost(size_t n) { @@ -515,7 +531,9 @@ class DualArray : private NonCopyable { resizeDevice(n); } - // This resize flavor fills host values only! + // Managed memory is a single allocation seen by host and device alike, so the + // host-side fill in resizeHost already defines what kernels read; no explicit + // host-to-device push is needed here (contrast with the pinned variant). void resize(size_t n, const T& val) { assert(m_host_vec_ptr == m_pinned_vec.get() && "resize() requires internal host ownership"); resizeHost(n, val); From 08330b503776ce4a373cc0d61266bc927f06b157 Mon Sep 17 00:00:00 2001 From: Dan Negrut Date: Mon, 10 Aug 2026 13:50:43 -0500 Subject: [PATCH 2/2] Document resize preconditions and shrink semantics (audit follow-up) Comment-only. States the two invariants the tail-push contract relies on, raised in code review: (1) resize() assumes device [0, old_size) is already defined and does not repair a host-only prefix created by growing through resizeHost() directly (no in-tree caller does; verified by grep); (2) shrink and same-size calls return without touching device memory, so capacity >= logical size is the invariant and bytes past the new logical size are dead. Co-Authored-By: Claude Opus 5 (1M context) --- src/core/utils/DataMigrationHelper.hpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/core/utils/DataMigrationHelper.hpp b/src/core/utils/DataMigrationHelper.hpp index d036ccd6..07eb33d3 100644 --- a/src/core/utils/DataMigrationHelper.hpp +++ b/src/core/utils/DataMigrationHelper.hpp @@ -239,6 +239,12 @@ class DualArray : private NonCopyable { // reallocation it copies forward only pre-existing data, and when capacity // already suffices it returns without touching memory (so the tail would hold // stale bytes). Push the host tail down so device [old_size, n) is defined. + // Precondition: device [0, old_size) is already defined, which holds whenever all + // prior growth went through resize(); this method does not repair a host-only + // prefix created by growing through resizeHost() directly (no in-tree caller + // does that). Shrink and same-size calls return without touching device memory: + // device capacity >= logical size is the invariant, and device bytes beyond the + // new logical size are dead, not cleared. if (n > old_size) { toDevice(old_size, n - old_size); } @@ -252,7 +258,9 @@ class DualArray : private NonCopyable { // Same as resize(n) above: without this push, the device tail [old_size, n) is // undefined on both growth paths while every caller reasonably believes it holds // `val`. Only the grown tail is copied; device data in [0, old_size) stays - // authoritative and is never overwritten by host state here. + // authoritative and is never overwritten by host state here. The same + // preconditions as resize(n) apply: an already-defined device prefix, and + // shrink/same-size calls leave device memory untouched. if (n > old_size) { toDevice(old_size, n - old_size); }