Collected bugfixes#5038

akohlmey · 2026-06-13T22:47:19Z

Summary

This pull request collects multiple small bugfixes that correct issues reported on GitHub
or found by adding tests to the unittest tree

Related Issue(s)

MatSci post: https://matsci.org/t/63775
Fixes #3543
Fixes #4537
https://github.com/lammps/lammps/security/code-scanning/615
Fix flipped forward/backward flag in fft_3d_1d_only_kokkos
KOKKOS: fix TransformView need_sync_device() under SINGLE_DEVICE

Author(s)

Axel Kohlmeyer, Temple U

Licensing

By submitting this pull request, I agree, that my contribution will be included in LAMMPS and redistributed under either the GNU General Public License version 2 (GPL v2) or the GNU Lesser General Public License version 2.1 (LGPL v2.1).

Artificial Intelligence (AI) Tools Usage

Part of the changes were generated by Claude Opus 4.8 but revised afterwards.

Backward Compatibility

Binary restarts for fix gjf and fix press/berendsen are not compatible.
If thermo_modify lost is set to warn or ignore LAMMPS can continue when atoms are lost during change_box and produces a usable data file with write_data.

Implementation Notes

The following individual changes are included:

CMake dead code removal and related doc cleanup. Try harder to find a compatible omp.h with Clang and HipCC and similar compilers.
Reorder code in variable.cpp to avoid use-after-free issues
Fix multiple memory leaks reported by ctest -T memcheck
Update valgrind suppressions
Silence a bunch of CodeQL alerts about integer overflows when assigning to size_t
Add missing components to allow for clean restarting of fix gjf and fix press/langevin
Recompute timestep dependent constants during setup() in fix press/langevin to correctly handle changes of timestep in input or from restart (and not run with incorrect settings).
Correct the protection level of several memory_usage() functions from (incorrect) protected: to the correct public:.
Multiple QEq styles would converge charges with incorrect sign when operating on a subset of atoms while the rest carry fixed charges. FixQEq::init_storage() initialized both the
s and t auxiliary vectors to the current charge for all atoms. Initializing s to the fixed charge of out-of-group atoms is correct, but doing the same for t -- which only encodes the group's charge-neutrality response -- not and creates problems when applyin the charge neutrality constraint leading to the incorrect sign in the test example. Initializing t to 0 for all atoms outside the fix group addresses fixes the bug. This was tested by comparing the results from fix qeq/dynamic and fix qeq/fire (which produce consistent results due to differences in their algorithm) against the affected fixes and consistent signs were obtained (the magnitude is different as expected).
The change_box command could silently leave atoms outside a non-periodic boundary. This would then produce an unexpected "Atom count is inconsistent, cannot write data file" error.
The fix contains two sets of changes:
- change_box now deletes atoms that are outside non-periodic(!) box boundaries (i.e. "lost"), in a similar fashion as read_data does. If atoms are "lost", the thermo_modify lost policy is applied: a) error (the default) aborts immediately during change_box (so the error happens where it is caused not later), b) warn prints a warning and continues, c) ignore continues silently
- if write_data encounters an inconsistent atom count it will apply the same thermo_modify policies, i.e. error out with the default setting or use the recomputed number of atoms with a warning or silently otherwise. This way, it will always create a data file with a consistent atoms entry in the header.

Post Submission Checklist

The feature or features in this pull request is complete
Licensing information is complete
Corresponding author information is complete
The source code follows the LAMMPS formatting guidelines
Suitable new documentation files and/or updates to the existing docs are included
The added/updated documentation is integrated and tested with the documentation build system
The feature has been verified to work with the conventional build system
The feature has been verified to work with the CMake based build system
Suitable tests have been added to the unittest tree.

change_box could leave atoms outside a non-periodic boundary without detecting them. Its move_atoms path uses Irregular::migrate_atoms(), which clamps out-of-box atoms onto the nearest proc (via comm->coord2proc) instead of removing them, unlike comm->exchange() during a run. So the existing lost-atom check never fired, atom->natoms was left inconsistent, and a subsequent write_data wrote a corrupted data file (header atom count != number of Atoms-section lines) when thermo_modify lost was "warn" or "ignore" (matsci.org/t/63775). change_box now culls atoms outside non-periodic boundaries after the box change (as a reneighbor would via comm->exchange), rebuilds the atom map, and applies the thermo_modify lost policy: "error" (default) aborts with a lost-atoms error, "warn" warns and continues, "ignore" is silent. atom->natoms is reset so the system stays self-consistent. write_data applies the same policy when the recomputed atom count differs from atom->natoms: "error" aborts (unchanged), while "warn"/"ignore" reset atom->natoms so the written data file is always self-consistent instead of having a header count that disagrees with the body. Docs for both commands updated with a versionchanged note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

When fix qeq/point (and the other s/t-method variants qeq/shielded, qeq/slater, qeq/ctip, which share this base-class code) is applied to a subset of the atoms while the remaining atoms carry fixed charges, the equilibrated charges came out with the wrong sign and the total energy increased rather than decreased. FixQEq::init_storage() initialized both the s and the t auxiliary vectors to the current per-atom charge for *all* atoms. Initializing s to the fixed charge of atoms outside the fix group is intentional: it injects their charge as the external field felt by the group atoms through the sparse matrix-vector product. The t vector, however, only encodes the charge-neutrality (Lagrange-multiplier) response of the group; giving out-of-group atoms their fixed charge there pollutes the neutralization (u = sum_s/sum_t) and flips the sign of the result. Initialize t to 0 for atoms outside the fix group. For the common case where qeq acts on all atoms this is a no-op (verified: examples/qeq output unchanged); it only affects the subset case, where qeq/point now matches qeq/dynamic and the energy is correctly minimized. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

fix gjf was not restartable: two pieces of per-step state were lost across a write_restart/read_restart cycle. - The Marsaglia RNG state, so the stochastic trajectory could not be reproduced after a restart. - The per-atom half-step velocity array lv. In the default "vel vhalf" mode the on-site velocity needed to continue the integration is kept in lv (atom->v only stores the half-step velocity used for output), so without checkpointing lv the first post-restart step started from a zeroed on-site velocity and the trajectory diverged at full scale. Add restart_global with write_restart()/restart() for the per-proc RNG state (as in fix temp/csvr and the BROWNIAN fixes), and restart_peratom with pack_restart()/unpack_restart() for lv, setting lv_allocated on restart so the restored lv is used on the first step. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…not init() The GJF barostat coefficients gjfa/gjfb depend on the timestep, but were computed in Fix::init(). A "timestep" command issued before a run only updates update->dt during run setup, which happens after init() runs, so the coefficients could be built from a stale dt while the integrator advanced with the new one. This makes the barostat trajectory depend on the timestep history rather than the active timestep (e.g. a run after a "timestep" change, or a restart that re-derives dt, diverges). Recompute gjfa/gjfb in setup() (via reset_dt()), where update->dt is final. p_mass/p_alpha (dt-independent) stay in init(). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

fix press/langevin was not restartable: a run continued from a restart did not reproduce the original stochastic barostat trajectory. Two pieces of state were lost across write_restart/read_restart: - the Langevin piston RNG state (without it the random piston forces, and thus the box dilation, diverge at full scale); and - the current piston force f_piston, which seeds f_old_piston on the next step. Add restart_global with write_restart()/restart() saving f_piston and the per-proc RNG state (replicated, but gathered per-rank for robustness). The piston momentum p_deriv is deliberately zeroed at each run start (init()), so it is intentionally not checkpointed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Two leaks reported by valgrind memcheck on the KOKKOS force-style unit tests (issue lammps#4537): - AtomKokkos::~AtomKokkos() never freed the per-atom dipole array: every other dual view was destroyed, but the destroy_kokkos(k_mu, mu) call was missing, leaking the mu row-pointer array on every atom-vec teardown. - AtomVecHybridKokkos had no destructor, so the stylesKK array allocated with new[] in process_args() leaked on every atom-vec creation (atom_style setup and each read_restart). Add a destructor that deletes it -- the array only holds cast copies of the AtomVec pointers owned and deleted by the AtomVecHybrid base class, so there is no double free -- and initialize the pointer to nullptr in the constructor. Verified under valgrind (--gtest_filter='*kokkos_omp') on mol-pair-lj_cut_dipole_cut and fix-timestep-nve_sphere_dipole: 131 KB of definite+possible losses drop to zero, with no other change in the report. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nii2NLXtpCD2oqS4rrYAX

The ctest memcheck runs report false positives from MPI/PMI startup and the Hydra process manager that the current suppressions do not catch -- e.g. the libfabric OFI/psmx3 sendto stack now has extra ofi_* frames so it no longer matches the older, more specific entry. Add four deliberately generic suppressions, each anchored on a single stable frame and wildcarded otherwise so they keep matching across MPICH/libfabric updates: - sendto on uninitialised bytes from the OFI/psmx3 network setup - all allocations made under PMPI_Init (freed only at MPI_Finalize) - uninitialised-value conditional jumps in the Hydra PMI event loop - write of uninitialised bytes from the Hydra PMI socket I/O All are outside LAMMPS. Verified the previously unsuppressed sendto false positive in test_text_file_reader is now suppressed (issue lammps#4537). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nii2NLXtpCD2oqS4rrYAX

…d quotes When split_words() hits an unterminated quote, the inner scan loop exits with c already pointing at the terminating NUL. The code then did an unconditional `c = *++buf`, advancing one byte past the end of the string buffer -- an invalid read of size 1, caught by valgrind in test_utils' split_words_partially_quoted case. Guard both quote handlers with `if (c) c = *++buf;` so we never read past the terminator; the normal (terminated-quote) case is unchanged since c is then the closing quote. Found via ctest memcheck (issue lammps#4537). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nii2NLXtpCD2oqS4rrYAX

CompiledExpression::operator= overwrote each operation[] pointer with a fresh clone() without first deleting the operation the target already held, leaking those objects (the destructor deletes them, so assignment must too). valgrind flagged this in the test_lepton Lepton_Exception case. Delete the existing operations before re-cloning, with a self-assignment guard so the delete does not free the source operations. In the bundled lib/lepton copy. Found via ctest memcheck (issue lammps#4537). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nii2NLXtpCD2oqS4rrYAX

FFTW's planner reads past the end of internal SIMD scratch buffers in fftw_plan_awake() (reached from fftw_destroy_plan() when LAMMPS tears down a PPPM/FFT3d plan), which valgrind reports as an invalid read of size 8. This is internal to FFTW and a known false positive, not a LAMMPS defect. Add a generic FFTW.supp anchored on fftw_plan_awake. Verified the previously unsuppressed read in test_kspace_group_group_slab is now suppressed (issue lammps#4537). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nii2NLXtpCD2oqS4rrYAX

The KnownAnswer_Kokkos_Sine test created an FFT with create_serial_fft() but never deleted it. TearDown() intentionally only nulls the void* fft member and leaves deletion to each test method (as the roundtrip/delta tests do), so this one leaked the entire FFT plan (208 direct + 5409 indirect bytes). Add the matching delete at the end of the test. Found via ctest memcheck (issue lammps#4537). The remaining valgrind noise in this test is the MPI_Init false positive and Kokkos-runtime possibly-lost blocks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nii2NLXtpCD2oqS4rrYAX

PairReaxFFKokkos::Deallocate_Lookup_Tables() skipped freeing the per-type row arrays LR[i] for unmapped types via an `if (map[i] == -1) continue;` in the outer loop. But Init_Lookup_Tables() allocates LR[i] for every type 0..ntypes (i == 0 is always unmapped), so those rows and the LR pointer array leaked when pair style reaxff/kk ran with long-range tabulation enabled. Move the map[] guard into the inner loop so it only gates the per-pair spline arrays (which exist only for mapped pairs) and free LR[i] unconditionally, matching the CPU Deallocate_Lookup_Tables(); also null api->system->LR after freeing it. Verified with test_pair_style atomic-pair-reaxff_tabulate.yaml under valgrind (kokkos_omp): 2688 bytes definitely lost -> 0 (issue lammps#4537). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nii2NLXtpCD2oqS4rrYAX

PairUF3Kokkos::destroy_3d()/destroy_4d() released the legacy cut_3b/min_cut_3b arrays (built by the base PairUF3::allocate() with memory->create()) using memory->sfree(), which frees only the outermost pointer and leaks the nested plane pointers and contiguous data block. Use memory->destroy() so all levels of the array are freed. The array reference is nulled afterwards, so the base ~PairUF3() memory->destroy() remains a safe no-op (no double free). Verified with test_pair_style manybody-pair-uf3.yaml under valgrind (kokkos_omp): ~2.6 KB direct + 58 KB indirect lost -> 0 (issue lammps#4537). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nii2NLXtpCD2oqS4rrYAX

The python-driven unit tests run the system python3 interpreter under valgrind. CPython and NumPy intentionally do not release all interpreter and module state at Py_Finalize (type objects, dtype/ufunc registrations, interned data), so valgrind reports them as leaks -- in a separate process from the LAMMPS code, hence false positives (no LAMMPS frames appear in any of these stacks). Add generic suppressions anchored on the CPython allocators (_PyMem_Raw*, _PyObject_GC_*, _PyObject_*alloc) so they survive CPython/NumPy version changes. Verified: `python3 -c "import numpy"` reports ~6.5 KB direct + ~29 KB indirect lost without these and 0 with them (issue lammps#4537). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nii2NLXtpCD2oqS4rrYAX

PairLdd::coeff() called allocate_species() unconditionally without freeing a previous allocation. When coeff() runs more than once (e.g. the force-style "extract" unit test re-initializes the pair), the earlier per-species arrays and the indicator/potential objects created by the first read_file() leaked -- valgrind reported ~98 bytes direct + ~2.3 KB indirect across the whole LDD family of tests (LD-constant/linear/mdpd, LDINDSET1/2, ...). Factor the per-species cleanup out of the destructor into deallocate_species() and call it at the start of coeff() (using the still-current nelements) so a repeated pair_coeff frees the prior arrays and indicator/potential objects first. The destructor now reuses the same helper. Verified with test_pair_style mol-pair-ldd-LD-constant.yaml under valgrind: ~2.4 KB lost -> 0, tests pass (issue lammps#4537). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nii2NLXtpCD2oqS4rrYAX

…e ldd" This reverts commit f96ad45.

PairBondVal::~PairBondVal() did not free the per-type arrays r0 and energy0, and PairBondValVec::~PairBondValVec() did not free r0 -- all allocated in the respective allocate() but missing from the destructor, so they leaked once coeff() ran. Add the missing memory->destroy() calls. Found via ctest memcheck (issue lammps#4537). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nii2NLXtpCD2oqS4rrYAX

…etup PairTersoffTableOMP::allocatePreLoops() allocated the per-thread pre-loop arrays without first freeing any previous allocation, so a repeated setup (e.g. the force-style "extract" test re-initializes the pair) leaked the prior arrays (~270 KB). The base PairTersoffTable::allocatePreLoops() already calls deallocatePreLoops() first; do the same in the OMP override. Found via ctest memcheck (issue lammps#4537). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nii2NLXtpCD2oqS4rrYAX

The python-driven unit tests run the system python3 + numpy under valgrind; NumPy's C-extension import (ufunc/dtype registration) and the CPython bytecode interpreter produce many leaks and uninitialised-value errors internal to those libraries. Add suppressions anchored on _PyImport_RunModInitFunc and _PyEval_EvalFrameDefault for the leak and uninitialised-value error kinds. Verified an `import numpy` + array workload drops from 100 unsuppressed errors (plus leaks) to 0 (issue lammps#4537). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nii2NLXtpCD2oqS4rrYAX

PairMorseSoft::nlambda is an int, but write_restart_settings() wrote it with sizeof(double) (emitting 4 uninitialised bytes into the restart file, flagged by valgrind) and read_restart_settings() read sizeof(double) into the int (4 bytes past it). Use sizeof(int), consistent with the following MPI_Bcast(MPI_INT). Note: this changes the (previously malformed) restart layout for this style. Found via ctest memcheck (issue lammps#4537). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nii2NLXtpCD2oqS4rrYAX

FixGLD::init_s_gld() only set the per-atom GLD state s_gld[] for atoms in the fix's group; for atoms outside the group it was left uninitialised and then packed into restart files (valgrind: uninitialised bytes written by WriteRestart). Zero s_gld for the non-group atoms. Found via ctest memcheck (issue lammps#4537). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nii2NLXtpCD2oqS4rrYAX

AtomVec::setup_fields() did `threads = new bool[ngrow]` without freeing a previous allocation; since the destructor only frees the last one, calling setup_fields() again leaked the earlier array. Delete it first. Found via ctest memcheck (issue lammps#4537). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nii2NLXtpCD2oqS4rrYAX

…MPS-GUI

The 1d-only timing path in FFT3dKokkos used flag == -1 to select the forward transform and flag == 1 to trigger inverse scaling, the opposite of the convention used everywhere else: fft_3d_kokkos() (the real compute path), the FORWARD=1/BACKWARD=-1 enum, and the KSPACE CPU fft_1d_only() in fft3d.cpp. Align the MKL, MKL_GPU, FFTW3/ NVPL, and KISS branches, and the scaling condition, with that convention. Co-authored-by: stanmoore1 <stanmoore1@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0168JQi3bP6s84dZcvoGxyQy

Under SINGLE_DEVICE the DualView edge is inert and modify_host() records legacy->device staleness on the HostKK<->Host edge (modified_legacy_hostkk), which need_sync_device() did not consult. It could therefore return false while the device (aliased to the Kokkos host mirror under SINGLE_DEVICE) was stale, so a gate such as atom_vec_*_kokkos.cpp's sync_pinned(Device, ...) would skip the required pinned-copy transform and the kernel would read stale data. OR in (SINGLE_DEVICE && modified_legacy_hostkk) so the predicate is consistent with need_sync_host() and with sync_device()'s own SINGLE_DEVICE delegation to sync_hostkk(). GPU / !SINGLE_DEVICE builds are unchanged: the added term folds to false at compile time. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: stanmoore1 <stanmoore1@gmail.com> Claude-Session: https://claude.ai/code/session_01Bqe95w49ywSEaMYT1tnP91

akohlmey and others added 9 commits June 13, 2026 17:39

make access protection for memory_usage() function consistent

36d9e93

utils.h is a "guaranteed" include

0b34d50

remove comment and code about flag that never existed

27f619a

unittest: add fix-timestep tests for gjf and press/langevin

54c4c8e

akohlmey self-assigned this Jun 13, 2026

akohlmey added the bug label Jun 13, 2026

akohlmey added this to LAMMPS Pull Requests Jun 13, 2026

akohlmey moved this to In Progress in LAMMPS Pull Requests Jun 13, 2026

temporarily disable tests that need updates form PR lammps#5035

c5369c1

akohlmey marked this pull request as ready for review June 14, 2026 04:05

akohlmey requested review from Bibobu, athomps, jtclemm and sjplimp as code owners June 14, 2026 04:05

akohlmey moved this from In Progress to Ready for Review or Merge in LAMMPS Pull Requests Jun 14, 2026

akohlmey added bugfix ready_for_merge and removed bug labels Jun 14, 2026

akohlmey added this to the Feature Release June 2026 milestone Jun 14, 2026

akohlmey added 6 commits June 16, 2026 09:12

avoid use after free warning by reordering code

1c21179

avoid warnings about integer overflows when assinging to a size_t type.

61b2108

must nullify deleted struct since we test against it being null

19a25f3

reorder operations to avoid use-after-free warning

2433d66

upgrade upload action to the latest version

5fa951d

Merge remote-tracking branch 'github/develop' into collected-bugfixes

303115c

akohlmey and others added 26 commits June 26, 2026 12:25

correct mass

eb09601

update log files for current LAMMPS version and change of mass.

ccad88a

record date of suppression update

40cc484

Revert "BOCS/ldd: fix memory leak on repeated pair_coeff in pair styl…

298b404

…e ldd" This reverts commit f96ad45.

avoid deprecation warning.

060b5c8

remove dead branch

9fb09d2

fix memory leak on failed plugin loads, sync with version used in LAM…

e769671

…MPS-GUI

Merge branch 'develop' into collected-bugfixes

70ccd92

stanmoore1 approved these changes Jul 1, 2026

View reviewed changes

akohlmey merged commit 4557f7f into lammps:develop Jul 1, 2026
9 checks passed

github-project-automation Bot moved this from Ready for Review or Merge to Done in LAMMPS Pull Requests Jul 1, 2026

akohlmey deleted the collected-bugfixes branch July 1, 2026 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Collected bugfixes#5038