From df986810ff563401db71315937575394ada0741e Mon Sep 17 00:00:00 2001 From: Weaselbot Date: Mon, 26 Oct 2026 20:34:45 -0400 Subject: [PATCH] Give the SHOW_MEMORY instrumentation a single definition per function 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. --- ConflictSet.cpp | 61 ++++++++++++++++++++++++++++++++++++---------- ShowMemoryTest.cpp | 26 ++++++++++++++++++++ 2 files changed, 74 insertions(+), 13 deletions(-) diff --git a/ConflictSet.cpp b/ConflictSet.cpp index 414728e..193b9f0 100644 --- a/ConflictSet.cpp +++ b/ConflictSet.cpp @@ -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"); diff --git a/ShowMemoryTest.cpp b/ShowMemoryTest.cpp index 5d3d078..440cf7f 100644 --- a/ShowMemoryTest.cpp +++ b/ShowMemoryTest.cpp @@ -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(); }