Give the SHOW_MEMORY instrumentation a single definition per function
CI / pre-commit (pull_request) Successful in 2m8s
CI / test (arm64, -DCMAKE_BUILD_TYPE=Debug -DMSAN_TOOLCHAIN_PATH=/opt/msan, 22, https://minio.weaselab.dev/public/aarch64/msan-toolchain-22.1.8.tar.zst, debug-arm64, ubuntu-latest-arm64) (pull_request) Successful in 3m49s
CI / test (amd64, -DCMAKE_BUILD_TYPE=Debug -DMSAN_TOOLCHAIN_PATH=/opt/msan, 21, https://minio.weaselab.dev/public/x86_64/msan-toolchain-21.1.8.tar.zst, debug, ubuntu-latest-amd64) (pull_request) Successful in 3m54s
CI / release (arm64, , ubuntu-latest-arm64) (pull_request) Successful in 3m25s
CI / test (amd64, -DCMAKE_CXX_FLAGS=-DUSE_64_BIT=1, 21, , 64-bit-versions, ubuntu-latest-amd64) (pull_request) Successful in 3m22s
CI / test (amd64, -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++, 21, , gcc, ubuntu-latest-amd64) (pull_request) Successful in 3m18s
CI / test (amd64, -DUSE_SIMD_FALLBACK=ON, 21, , simd-fallback, ubuntu-latest-amd64) (pull_request) Successful in 3m25s
CI / release (amd64, -DMSAN_TOOLCHAIN_PATH=/opt/msan, ubuntu-latest-amd64) (pull_request) Successful in 5m50s
CI / coverage (pull_request) Failing after 3m10s

The coverage job compiles ConflictSet.cpp twice: once without SHOW_MEMORY,
like the rest of the build, and once with it for the SHOW_MEMORY test. Each
config previously saw a *different* definition of the accounting helpers:
no-op `constexpr` stubs near the top of the file when SHOW_MEMORY was off,
and the real definitions at the bottom when it was on.

gcovr reads the .gcda files for a source file and merges them, and its
default (strict) function merge mode asserts that a function name always
maps to one line. Since `addNode`, `removeNode`, `addKey`, both `removeKey`
overloads and `getSearchPathLength` were reported on two different lines,
report generation aborted with

    AssertionError: Got function getSearchPathLength(Node*) on multiple
    lines: 99, 6002.

Define those functions in exactly one place instead, and compile the
accounting out from within the bodies. Then every configuration reports each
function on the same line and the coverage data can be merged. The empty
calls are inlined away: the Release shared library contains no calls to
them.

With the merge fixed, the accounting lines that only exist in the SHOW_MEMORY
build show up in the report, so the coverage job's 100% line requirement
covers them. Extend the SHOW_MEMORY test to grow a node through every node
type (so `getNodeSize` covers every case) and to erase entries one at a time
via `setOldestVersion` (so `removeKey` and the `internal_setOldestVersion`
cross-check are covered). The remaining `abort()`s in the accounting are
defensive checks that can never be taken on a passing run; mark them
GCOVR_EXCL_LINE like the rest of the unreachable code in this file.
This commit is contained in:
weaselbot committed 2026-10-26 20:34:45 -04:00
1 parent bba328f79f
commit df986810ff
2 files changed
+74 -13

No files matched your search

+48 -13
View File
@@ -80,7 +80,10 @@ using namespace weaselab;
#define assume assert
#endif
#if SHOW_MEMORY
// The SHOW_MEMORY instrumentation. Declared unconditionally so that each of
// these has a single definition in every configuration (see the definitions at
// the end of this file), rather than a no-op stub standing in for the real
// thing.
void addNode(struct Node *);
void removeNode(struct Node *);
void addKey(struct Node *);
@@ -90,14 +93,6 @@ void removeKey(struct Node *);
// `parent` pointers invalid.
void removeKey(struct Node *, int64_t searchPathLength);
int64_t getSearchPathLength(struct Node *);
#else
constexpr void addNode(struct Node *) {}
constexpr void removeNode(struct Node *) {}
constexpr void addKey(struct Node *) {}
constexpr void removeKey(struct Node *) {}
constexpr void removeKey(struct Node *, int64_t) {}
constexpr int64_t getSearchPathLength(struct Node *) { return 0; }
#endif
// ==================== BEGIN IMPLEMENTATION ====================
@@ -5492,8 +5487,9 @@ void internal_addWrites(ConflictSet::Impl *impl,
impl->totalBytes += mallocBytesDelta;
impl->memory_bytes.set(impl->totalBytes);
#if SHOW_MEMORY
// Testing-only cross-check of the accounting.
if (impl->totalBytes != mallocBytes) {
abort();
abort(); // GCOVR_EXCL_LINE
}
#endif
}
@@ -5504,8 +5500,9 @@ void internal_setOldestVersion(ConflictSet::Impl *impl, int64_t oldestVersion) {
impl->totalBytes += mallocBytesDelta;
impl->memory_bytes.set(impl->totalBytes);
#if SHOW_MEMORY
// Testing-only cross-check of the accounting.
if (impl->totalBytes != mallocBytes) {
abort();
abort(); // GCOVR_EXCL_LINE
}
#endif
}
@@ -5971,6 +5968,14 @@ checkMaxVersion(Node *root, Node *node, InternalVersionT oldestVersion,
} // namespace
// The SHOW_MEMORY instrumentation. Each of these functions is defined exactly
// once, with the accounting compiled out when SHOW_MEMORY is disabled. Defining
// them once (rather than defining no-op stubs when SHOW_MEMORY is disabled)
// keeps every configuration of this file reporting each function on the same
// line, which merging coverage from objects compiled with different options
// requires: gcovr, merging the SHOW_MEMORY test with the rest of the build,
// otherwise aborts with "Got function ... on multiple lines". The optimizer
// removes the resulting empty calls.
#if SHOW_MEMORY
int64_t nodeBytes = 0;
@@ -5994,12 +5999,15 @@ int64_t getNodeSize(struct Node *n) {
return sizeof(Node48);
case Type_Node256:
return sizeof(Node256);
default:
abort();
default: // GCOVR_EXCL_LINE
abort(); // GCOVR_EXCL_LINE
}
}
#endif
int64_t getSearchPathLength(Node *n) {
#if SHOW_MEMORY
assert(n != nullptr);
int64_t result = 0;
for (;;) {
@@ -6011,9 +6019,14 @@ int64_t getSearchPathLength(Node *n) {
n = n->parent;
}
return result;
#else
(void)n;
return 0;
#endif
}
void addNode(Node *n) {
#if SHOW_MEMORY
nodeBytes += getNodeSize(n);
partialCapacityBytes += n->getCapacity();
if (nodeBytes > peakNodeBytes) {
@@ -6022,14 +6035,22 @@ void addNode(Node *n) {
if (partialCapacityBytes > peakPartialCapacityBytes) {
peakPartialCapacityBytes = partialCapacityBytes;
}
#else
(void)n;
#endif
}
void removeNode(Node *n) {
#if SHOW_MEMORY
nodeBytes -= getNodeSize(n);
partialCapacityBytes -= n->getCapacity();
#else
(void)n;
#endif
}
void addKey(Node *n) {
#if SHOW_MEMORY
if (!n->entryPresent) {
++totalKeys;
keyBytes += getSearchPathLength(n);
@@ -6040,22 +6061,36 @@ void addKey(Node *n) {
peakKeyBytes = keyBytes;
}
}
#else
(void)n;
#endif
}
void removeKey(Node *n) {
#if SHOW_MEMORY
if (n->entryPresent) {
--totalKeys;
keyBytes -= getSearchPathLength(n);
}
#else
(void)n;
#endif
}
void removeKey(Node *n, int64_t searchPathLength) {
#if SHOW_MEMORY
if (n->entryPresent) {
--totalKeys;
keyBytes -= searchPathLength;
}
#else
(void)n;
(void)searchPathLength;
#endif
}
#if SHOW_MEMORY
struct __attribute__((visibility("default"))) PeakPrinter {
~PeakPrinter() {
printf("--- radix_tree ---\n");
+26
View File
@@ -86,4 +86,30 @@ int main() {
assert(checkPoint(cs, "other/999", version) == ConflictSet::Conflict);
}
checkFullyReleased();
// Grow a node through every node type, so that the accounting for each node
// type's size is exercised, then erase the entries one at a time so that
// nodes are released individually rather than as part of a whole tree.
{
ConflictSet cs{0};
std::string key(1, '\0');
for (int i = 0; i < 256; ++i) {
key[0] = char(i);
writePoint(cs, key, i + 1);
}
for (int i = 0; i < 256; ++i) {
key[0] = char(i);
assert(checkPoint(cs, key, i) == ConflictSet::Conflict);
}
// Garbage collection erases entries that can only fail with TooOld anyway.
// Doing it one version at a time walks the tree, erasing, downsizing and
// releasing nodes as it goes.
for (int i = 0; i < 256; ++i) {
cs.setOldestVersion(i + 1);
}
key[0] = '\0';
assert(checkPoint(cs, key, 0) == ConflictSet::TooOld);
}
checkFullyReleased();
}