Fix latent memory error exposed by fixed_hierarchy_test#668
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR rewrites ChangesFixed hierarchy conversion
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 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 |
|
21 fixed, 0 new since branch point (5e6a365) ✅ 21 CodeQL alerts resolved since the previous PR commit
✅ 21 CodeQL alerts resolved since the branch point
Review the full CodeQL report for details. |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #668 +/- ##
==========================================
- Coverage 83.55% 83.02% -0.54%
==========================================
Files 170 172 +2
Lines 6283 6446 +163
Branches 706 744 +38
==========================================
+ Hits 5250 5352 +102
- Misses 810 854 +44
- Partials 223 240 +17
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Clang-Tidy Check ResultsFound 5374 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
coverage-upload was missing setup in its needs list and was missing ref: and repository: in its checkout step. Without those, on a workflow_dispatch run where main advances between the coverage job starting and the coverage-upload job starting (which happened in PR #668), the upload job checks out a different commit than the one coverage was actually measured on. Codecov then tries to upload for that wrong commit SHA, which it has no record of, producing the 404.
|
The change looks fine to me, but I don't understand Claude's explanation. The vector of identifiers was moved into the layer path so how can the layer path be sharing memory with anything? |
See the attached ASAN log for |
greenc-FNAL
left a comment
greenc-FNAL
left a comment
There was a problem hiding this comment.
Fix looks good. I spent a few minutes coming up with a regression test using the sanitizer—testing now, but it's up to you whether you want to wait for it or have me put in a specific PR.
|
I suspect clang-tidy will complain about GPT 5.5 identified the cause as a bug in GCC 15.1's libstdc++ which was fixed in 15.2. |
|
AI-generated analysis of the ASan trace: The crash does not look like For This matches a plausible libstdc++ 15.1 issue in the vector range-constructor path: storage is allocated before |
knoepfel
force-pushed
the
fix-latent-memory-error
branch
from
09c5d41 to
9157a71
Compare
Okay, thanks for the cross-check, @beojan. When merging, I'll adjust the commit message to refer to the GCC 15.1 limitation. |
Clang-Tidy Check Results1 new issue(s) introduced by this patch (5374 total). New issues on modified lines:
All issues by check:
See inline comments for details. Comment |
…ith no layers
An empty inner vector {} causes layer_path to be constructed from an empty
std::vector<identifier>, which immediately throws in validate(). The exception was
unwinding through std::ranges::to<std::vector<layer_path>> while the partially-constructed
vector is being destroyed — at that point the layer_path destructor runs on memory that
was moved-from or is otherwise in an invalid state.
Co-authored-by: Beojan Stanislaus <beojan@gmail.com>
…arding reference)
knoepfel
force-pushed
the
fix-latent-memory-error
branch
from
9157a71 to
999aabc
Compare
|
@beojan, clang-tidy objected to the latest implementation (see #668 (comment)). Commit 999aabc is a fix. |
Clang-Tidy Check ResultsFound 5438 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
3 participants
I encountered a bug in the implementation of
convert_vector_vector_stringwhen creating afixed_hierarchyobject. I ran the address sanitizer on the test and submitted its output to Claude Sonnet 4.6, which said:Because this is a memory error, it does not necessarily manifest on all platforms/compiler combinations. I encountered it by running on Alma Linux 9 with GCC 15.1; I did not encounter it running on macOS 26.5.1 with Apple clang 21.0.0.
This PR fixes the bug.
Code
convert_vector_vector_stringinphlex/model/fixed_hierarchy.cppto construct thestd::vector<layer_path>with an explicit loop andreserve()instead of astd::rangestransform/to pipeline.std::stringintoidentifier(std::move(str))before building eachlayer_path.layer_pathvalidation failure.Reliability
fixed_hierarchy{{}}, whereLayer paths cannot be emptyis thrown and partially built objects are now destroyed safely.