internal_addWrites / internal_setOldestVersion contain a SHOW_MEMORY-only cross-check that aborts when the per-instance byte total differs from a process-wide counter:
// ConflictSet.cpp:5544-5553 (identical code at SkipList.cpp:913-921,
// and in internal_setOldestVersion at ConflictSet.cpp:5557-5566 / SkipList.cpp:924-933)
voidinternal_addWrites(ConflictSet::Impl*impl,constConflictSet::WriteRange*writes,intcount,int64_twriteVersion){mallocBytesDelta=0;impl->addWrites(writes,count,writeVersion);impl->totalBytes+=mallocBytesDelta;impl->memory_bytes.set(impl->totalBytes);#if SHOW_MEMORY
// Testing-only cross-check of the accounting.
if(impl->totalBytes!=mallocBytes){abort();// GCOVR_EXCL_LINE
}#endif
}
impl->totalBytes is per instance — it only accumulates the mallocBytesDelta observed during this instance's API calls. mallocBytes is process-wide: an inline int64_t in Internal.h:99 that safe_malloc/safe_free (Internal.h:116-155) bump for every allocation, whoever makes it.
So the equality only holds if the instance is the only live user of safe_malloc in the process. As soon as a program has two live ConflictSet objects — completely ordinary, valid API usage; nothing in include/ConflictSet.h says otherwise — the first addWrites/setOldestVersion on either of them abort()s.
The comment in ShowMemoryTest.cpp:4-5 ("Only one ConflictSet may exist at a time, since the accounting is process-wide") documents this restriction only for that one test; it is not a documented precondition of the library, and the repo's own test_conflict_set.py::test_update_zero_should_commit/conflict tests do not honour it (they deliberately keep two instances alive), so they abort in a SHOW_MEMORY build.
Reproduction
repro.cpp:
#include"ConflictSet.h"#include<cstdio>usingnamespaceweaselab;intmain(){ConflictSeta{0};ConflictSetb{5};// a second, independent ConflictSet
ConflictSet::WriteRangew{{(constuint8_t*)"k",1},{nullptr,0}};a.addWrites(&w,1,1);// <-- aborts here in a SHOW_MEMORY build
ConflictSet::ReadRanger{{(constuint8_t*)"k",1},{nullptr,0},0};ConflictSet::Resultres=ConflictSet::Commit;a.check(&r,&res,1);printf("result=%d\n",(int)res);return0;}
Both shipped implementations, same program (aarch64, gcc 16.1.1, commit e23018c):
Backtrace and values of both quantities at the failing check:
$ gdb -batch -ex 'b ConflictSet.cpp:5550' -ex run -ex 'p impl->totalBytes' -ex 'p mallocBytes' --args ./repro_radix_sm
Breakpoint 1, internal_addWrites (impl=0x466030, ...) at ConflictSet.cpp:5550
5550 if (impl->totalBytes != mallocBytes) {
$1 = 3456 # this instance
$2 = 6840 # this instance + the 3384 bytes owned by the second instance
$ gdb -batch -ex run -ex bt --args ./repro_radix_sm
#3 abort ()
at ConflictSet.cpp:5551
#4 weaselab::ConflictSet::addWrites (...) at ConflictSet.cpp:5660
#5 main () at repro.cpp:9
Note the abort happens on instance a because of bytes owned by instance b — the failure depends only on unrelated state elsewhere in the process.
Impact
With -DSHOW_MEMORY=1 (the documented way to enable the instrumentation; .pre-commit-config.yaml forbids committing #define SHOW_MEMORY 1, and CMakeLists.txt:399-401 passes -DSHOW_MEMORY=1 for conflict_set_show_memory_test), the library aborts on valid input for any program that keeps more than one ConflictSet alive. That is the normal shape of an application using this library per data store, and it also silently breaks the instrumentation for anything but the single-instance case.
The repo's own test suite fails this way. Building the project with the instrumentation and running the script tests:
$ cmake -S . -B build-sm -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS=-DSHOW_MEMORY=1
$ ninja -C build-sm && ctest --test-dir build-sm -R script_test_update_zero
1/4 Test #5748: script_test_update_zero_should_commit ................Subprocess aborted
2/4 Test #5749: script_test_update_zero_should_commit_valgrind .......Subprocess aborted
3/4 Test #5750: script_test_update_zero_should_conflict ..............Subprocess aborted
4/4 Test #5751: script_test_update_zero_should_conflict_valgrind .....Subprocess aborted
0% tests passed, 4 tests failed out of 4
with the abort in the other implementation's copy of the same check, since DebugConflictSet creates both instances up front:
Process terminating with default action of signal 6 (SIGABRT)
at raise ()
by abort ()
by internal_addWrites(...) (SkipList.cpp:919)
by ConflictSet_addWrites (SkipList.cpp:1019)
(The same suite passes 100% without SHOW_MEMORY, so this is not a pre-existing test failure but a direct consequence of the cross-check.)
Suggested fix
Make the cross-check compare like with like, e.g.:
snapshot the process-wide total when the instance is created and compare mallocBytes - snapshot against impl->totalBytes; or
keep the accounting per-instance (move mallocBytes/peakMallocBytes into Impl/WriteContext) instead of using a process-wide inline counter; or
drop the abort and rely on ShowMemoryTest.cpp's existing checkFullyReleased() (getBytes() == 0 after destruction) checks, which are correct for multiple instances.
A regression test as small as repro.cpp above (or simply making the existing script tests run against a SHOW_MEMORY library build) would catch this.
## Summary
`internal_addWrites` / `internal_setOldestVersion` contain a `SHOW_MEMORY`-only cross-check that aborts when the *per-instance* byte total differs from a *process-wide* counter:
```c++
// ConflictSet.cpp:5544-5553 (identical code at SkipList.cpp:913-921,
// and in internal_setOldestVersion at ConflictSet.cpp:5557-5566 / SkipList.cpp:924-933)
void internal_addWrites(ConflictSet::Impl *impl, const ConflictSet::WriteRange *writes,
int count, int64_t writeVersion) {
mallocBytesDelta = 0;
impl->addWrites(writes, count, writeVersion);
impl->totalBytes += mallocBytesDelta;
impl->memory_bytes.set(impl->totalBytes);
#if SHOW_MEMORY
// Testing-only cross-check of the accounting.
if (impl->totalBytes != mallocBytes) {
abort(); // GCOVR_EXCL_LINE
}
#endif
}
```
`impl->totalBytes` is **per instance** — it only accumulates the `mallocBytesDelta` observed during *this* instance's API calls. `mallocBytes` is **process-wide**: an `inline int64_t` in `Internal.h:99` that `safe_malloc`/`safe_free` (`Internal.h:116-155`) bump for *every* allocation, whoever makes it.
So the equality only holds if the instance is the only live user of `safe_malloc` in the process. As soon as a program has **two live `ConflictSet` objects** — completely ordinary, valid API usage; nothing in `include/ConflictSet.h` says otherwise — the first `addWrites`/`setOldestVersion` on either of them `abort()`s.
The comment in `ShowMemoryTest.cpp:4-5` ("Only one ConflictSet may exist at a time, since the accounting is process-wide") documents this restriction only for that one test; it is not a documented precondition of the library, and the repo's own `test_conflict_set.py::test_update_zero_should_commit/conflict` tests do not honour it (they deliberately keep two instances alive), so they abort in a `SHOW_MEMORY` build.
## Reproduction
`repro.cpp`:
```c++
#include "ConflictSet.h"
#include <cstdio>
using namespace weaselab;
int main() {
ConflictSet a{0};
ConflictSet b{5}; // a second, independent ConflictSet
ConflictSet::WriteRange w{{(const uint8_t *)"k", 1}, {nullptr, 0}};
a.addWrites(&w, 1, 1); // <-- aborts here in a SHOW_MEMORY build
ConflictSet::ReadRange r{{(const uint8_t *)"k", 1}, {nullptr, 0}, 0};
ConflictSet::Result res = ConflictSet::Commit;
a.check(&r, &res, 1);
printf("result=%d\n", (int)res);
return 0;
}
```
Both shipped implementations, same program (aarch64, gcc 16.1.1, commit `e23018c`):
```
$ g++ -std=c++20 -O1 -g -Iinclude -I. -isystem third_party/valgrind \
-o repro_radix repro.cpp ConflictSet.cpp
$ g++ ... -DSHOW_MEMORY=1 -o repro_radix_sm repro.cpp ConflictSet.cpp
$ g++ ... -o repro_skip repro.cpp SkipList.cpp
$ g++ ... -DSHOW_MEMORY=1 -o repro_skip_sm repro.cpp SkipList.cpp
repro_radix -> rc=0, prints "result=1" (Conflict, correct)
repro_skip -> rc=0, prints "result=1" (Conflict, correct)
repro_radix_sm -> rc=-6 (SIGABRT)
repro_skip_sm -> rc=-6 (SIGABRT)
```
Backtrace and values of both quantities at the failing check:
```
$ gdb -batch -ex 'b ConflictSet.cpp:5550' -ex run -ex 'p impl->totalBytes' -ex 'p mallocBytes' --args ./repro_radix_sm
Breakpoint 1, internal_addWrites (impl=0x466030, ...) at ConflictSet.cpp:5550
5550 if (impl->totalBytes != mallocBytes) {
$1 = 3456 # this instance
$2 = 6840 # this instance + the 3384 bytes owned by the second instance
$ gdb -batch -ex run -ex bt --args ./repro_radix_sm
#3 abort ()
at ConflictSet.cpp:5551
#4 weaselab::ConflictSet::addWrites (...) at ConflictSet.cpp:5660
#5 main () at repro.cpp:9
```
Note the abort happens on instance `a` because of bytes owned by instance `b` — the failure depends only on unrelated state elsewhere in the process.
## Impact
* With `-DSHOW_MEMORY=1` (the documented way to enable the instrumentation; `.pre-commit-config.yaml` forbids committing `#define SHOW_MEMORY 1`, and `CMakeLists.txt:399-401` passes `-DSHOW_MEMORY=1` for `conflict_set_show_memory_test`), the library aborts on valid input for any program that keeps more than one `ConflictSet` alive. That is the normal shape of an application using this library per data store, and it also silently breaks the instrumentation for anything but the single-instance case.
* The repo's own test suite fails this way. Building the project with the instrumentation and running the script tests:
```
$ cmake -S . -B build-sm -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS=-DSHOW_MEMORY=1
$ ninja -C build-sm && ctest --test-dir build-sm -R script_test_update_zero
1/4 Test #5748: script_test_update_zero_should_commit ................Subprocess aborted
2/4 Test #5749: script_test_update_zero_should_commit_valgrind .......Subprocess aborted
3/4 Test #5750: script_test_update_zero_should_conflict ..............Subprocess aborted
4/4 Test #5751: script_test_update_zero_should_conflict_valgrind .....Subprocess aborted
0% tests passed, 4 tests failed out of 4
```
with the abort in the *other* implementation's copy of the same check, since `DebugConflictSet` creates both instances up front:
```
Process terminating with default action of signal 6 (SIGABRT)
at raise ()
by abort ()
by internal_addWrites(...) (SkipList.cpp:919)
by ConflictSet_addWrites (SkipList.cpp:1019)
```
(The same suite passes 100% without `SHOW_MEMORY`, so this is not a pre-existing test failure but a direct consequence of the cross-check.)
## Suggested fix
Make the cross-check compare like with like, e.g.:
* snapshot the process-wide total when the instance is created and compare `mallocBytes - snapshot` against `impl->totalBytes`; or
* keep the accounting per-instance (move `mallocBytes`/`peakMallocBytes` into `Impl`/`WriteContext`) instead of using a process-wide `inline` counter; or
* drop the abort and rely on `ShowMemoryTest.cpp`'s existing `checkFullyReleased()` (`getBytes() == 0` after destruction) checks, which are correct for multiple instances.
A regression test as small as `repro.cpp` above (or simply making the existing script tests run against a `SHOW_MEMORY` library build) would catch this.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Summary
internal_addWrites/internal_setOldestVersioncontain aSHOW_MEMORY-only cross-check that aborts when the per-instance byte total differs from a process-wide counter:impl->totalBytesis per instance — it only accumulates themallocBytesDeltaobserved during this instance's API calls.mallocBytesis process-wide: aninline int64_tinInternal.h:99thatsafe_malloc/safe_free(Internal.h:116-155) bump for every allocation, whoever makes it.So the equality only holds if the instance is the only live user of
safe_mallocin the process. As soon as a program has two liveConflictSetobjects — completely ordinary, valid API usage; nothing ininclude/ConflictSet.hsays otherwise — the firstaddWrites/setOldestVersionon either of themabort()s.The comment in
ShowMemoryTest.cpp:4-5("Only one ConflictSet may exist at a time, since the accounting is process-wide") documents this restriction only for that one test; it is not a documented precondition of the library, and the repo's owntest_conflict_set.py::test_update_zero_should_commit/conflicttests do not honour it (they deliberately keep two instances alive), so they abort in aSHOW_MEMORYbuild.Reproduction
repro.cpp:Both shipped implementations, same program (aarch64, gcc 16.1.1, commit
e23018c):Backtrace and values of both quantities at the failing check:
Note the abort happens on instance
abecause of bytes owned by instanceb— the failure depends only on unrelated state elsewhere in the process.Impact
-DSHOW_MEMORY=1(the documented way to enable the instrumentation;.pre-commit-config.yamlforbids committing#define SHOW_MEMORY 1, andCMakeLists.txt:399-401passes-DSHOW_MEMORY=1forconflict_set_show_memory_test), the library aborts on valid input for any program that keeps more than oneConflictSetalive. That is the normal shape of an application using this library per data store, and it also silently breaks the instrumentation for anything but the single-instance case.with the abort in the other implementation's copy of the same check, since
DebugConflictSetcreates both instances up front:(The same suite passes 100% without
SHOW_MEMORY, so this is not a pre-existing test failure but a direct consequence of the cross-check.)Suggested fix
Make the cross-check compare like with like, e.g.:
mallocBytes - snapshotagainstimpl->totalBytes; ormallocBytes/peakMallocBytesintoImpl/WriteContext) instead of using a process-wideinlinecounter; orShowMemoryTest.cpp's existingcheckFullyReleased()(getBytes() == 0after destruction) checks, which are correct for multiple instances.A regression test as small as
repro.cppabove (or simply making the existing script tests run against aSHOW_MEMORYlibrary build) would catch this.This mostly doesn't matter, but I guess we can put a comment somewhere saying that SHOW_MEMORY assumes one conflict set at a time