fix(tf): avoid empty AMix neighbor mapping pointers#5791
Conversation
Pass a neighbor-index mapping only when periodic coordinate expansion actually populated it. Non-PBC, external, and tensor-provided neighbor-list modes now pass nullptr instead of forming an invalid pointer from an empty vector. Add direct CPU coverage for the standard external-pointer AMix path and the NVNMD no-PBC path, including neighbor indices, types, masks, valid entries, and padding. Fixes deepmodeling#5653. Coding-Agent: Codex Codex-Version: codex-cli 0.144.1 Model: gpt-5.6-sol Reasoning-Effort: xhigh
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe AMix CPU paths now pass ChangesAMix mapping safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5791 +/- ##
==========================================
- Coverage 79.85% 78.44% -1.41%
==========================================
Files 1022 1053 +31
Lines 117351 120891 +3540
Branches 4313 4383 +70
==========================================
+ Hits 93706 94830 +1124
- Misses 22101 24503 +2402
- Partials 1544 1558 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Correct fix. Forming &idx_mapping[0] on the empty mapping vector (non-PBC/external-nlist modes) is UB and aborts under a hardened libstdc++ (_GLIBCXX_ASSERTIONS), even though release builds tolerate it because the pointer is never dereferenced. Passing idx_mapping.data() only when b_nlist_map is set, else nullptr, is safe: I confirmed use_nei_info_cpu (neighbor_list.cc) dereferences nlist_map exclusively inside the if(b_nlist_map) branch, so nullptr in the false case is never touched. Both the standard AMix and NVNMD AMix call sites are fixed. One non-blocking note: the new tests exercise the two affected fallbacks (good coverage) and abort pre-fix under a debug/hardened libstdc++, but deepmd CI runs only -fsanitize=leak, which won't catch this UB -- so they won't guard against reintroduction in CI unless a _GLIBCXX_ASSERTIONS or ASAN job is added. LGTM.
Dismissing to re-review through the /code-review skill per process.
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Re-reviewed via the code-review skill. The fix is correct and complete: forming &idx_mapping[0] on the empty mapping vector is UB (aborts under a hardened libstdc++), and passing idx_mapping.data() only when b_nlist_map is set else nullptr is safe -- use_nei_info_cpu (neighbor_list.cc) dereferences nlist_map exclusively inside if(b_nlist_map). Both AMix call sites are fixed and there are no missed siblings (the other &idx_mapping[0] sites are wrapped in if(b_nlist_map){}). Two non-blocking notes: (1) the NVNMD site's b_nlist_map==true branch is only covered indirectly by test_nvnmd_entrypoints; a direct PBC NVNMD AMix test would make that explicit. (2) The new tests are coverage-only -- pre-fix and post-fix produce identical output in a release build, and CI runs only -fsanitize=leak, so a reintroduction of this exact UB wouldn't fail in CI without a _GLIBCXX_ASSERTIONS/ASan job. Neither blocks; the fix itself is provably correct. LGTM.
|
Reviewed the two optional follow-ups. The direct PBC NVNMD path is currently covered indirectly by the entrypoint suite, while a focused operator fixture would require the NVNMD reference/config setup; the UB regression itself is now covered by the affected fallback tests. I left the CI sanitizer-policy expansion out of this focused fix. Coding agent: Codex |
Exercise the periodic NVNMD AMix remapping branch directly and run both affected AMix paths in the C++ sanitizer matrix with libstdc++ bounds assertions enabled. Coding-Agent: Codex Codex-Version: codex-cli 0.144.4 Model: gpt-5.6-sol Reasoning-Effort: xhigh
|
I addressed both non-blocking coverage notes in
The focused PBC test passed locally. Ruff, workflow formatting, and repository hooks also passed; the hardened execution is delegated to CI. Coding agent: Codex |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test_cc.yml:
- Around line 90-95: Add the standard AMix test `test_external_nlist_amix` to
the pytest invocation in the “Test AMix neighbor mapping with hardened
libstdc++” workflow step, alongside the existing NVNMD tests, while preserving
the LD_LIBRARY_PATH configuration and current test selection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b6bd332d-25cd-4a8f-8eab-fa58d62af6b6
📒 Files selected for processing (2)
.github/workflows/test_cc.ymlsource/tests/tf/test_prod_env_mat.py
Run the standard external-neighbor-list AMix test alongside both NVNMD mapping cases in the libstdc++ assertions job. Coding-Agent: Codex Codex-Version: codex-cli 0.144.4 Model: gpt-5.6-sol Reasoning-Effort: xhigh
Pull request was converted to draft
Keep the AMix pointer fix scoped to implementation and regression tests. The existing leak-check matrix should not be repurposed as a global libstdc++ assertions job. Coding-Agent: Codex Codex-Version: codex-cli 0.144.4 Model: gpt-5.6-sol Reasoning-Effort: xhigh
Summary
Why existing tests missed this
The ordinary AMix CPU no-PBC and PBC modes now use a batch fast path, so the affected legacy fallback is reached primarily by the external pointer-based neighbor-list mode. There was no direct test for that AMix path. The NVNMD AMix path also had no direct no-PBC test.
In release libstdc++, taking element zero from an empty vector can appear to work when the callee does not dereference the pointer, so higher-level numerical tests do not reliably expose the undefined behavior. The defect is diagnosed by static analysis or a libstdc++ debug/sanitizer build. The added tests ensure both affected call sites remain exercised and validate their neighbor indices, types, masks, valid entries, and padding.
Validation
Closes #5653.
Coding agent: Codex
Codex version: codex-cli 0.144.1
Model: gpt-5.6-sol
Reasoning effort: xhigh
Summary by CodeRabbit
Bug Fixes
Tests