From 80f04a661a887904e78ea605ce44f791eb08f74d Mon Sep 17 00:00:00 2001 From: thawk105 Date: Wed, 13 May 2026 00:39:23 +0000 Subject: [PATCH 1/5] oze: fix uninitialized ScanRange union + enable -Werror=maybe-uninitialized MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 of the phased -Werror cleanup tracked in #43. Promotes one warning class to error at a time and fixes its offenders in the same PR. The fix ======= cc/oze/include/transaction.hh:228 declared `uint64_t updated;` without an initializer and then only `|=`'d into the upper or lower 32 bits in two separate if/else branches. The other 32 bits stayed garbage, so the value written back to ScanRange[index] via compare_exchange_strong was not the intended `union(current [min,max], new [left,right])`. Why this is an actual bug, not just a hygiene warning ----------------------------------------------------- ScanRange is the fast-path bounding box read by the insert-validation path at cc/oze/transaction.cc:851 — it splits the 64-bit atomic into upper-32 = min, lower-32 = max and short-circuits the ScanHistory linear walk if the inserted key falls outside [min, max]. Maintaining that bounding box correctly requires it to grow monotonically: new_min = min(current_min, left) new_max = max(current_max, right) scan_range = (new_min << 32) | new_max With `updated` uninitialized, the OR with garbage can: - shrink the apparent min and grow the apparent max, masking inserts that should have triggered the ScanHistory check (correctness: potential phantom anomaly), or - skew the range so the fast-path never short-circuits (perf only). Either way, the behavior depends on stack contents, which is exactly what -Wmaybe-uninitialized flagged. Initializing `updated = 0` lets the two `|=` branches build the intended `(min << 32) | max` layout. Paper cross-check ----------------- The Oze PVLDB paper (Nemoto et al., PVLDB vol.18 p2321; extended version at arxiv:2210.04179) describes phantom prevention via a per- transaction scan history (txid + predicates) checked by inserters in the validation phase. ScanRange is not in the paper — it is an implementation-side fast-path filter in front of that scan history. The fix preserves the paper's correctness contract (ScanHistory is unchanged) while making the fast-path do what the surrounding code clearly intends. CMake side ========== - `ccbench_add_protocol()` now adds `-Werror=maybe-uninitialized` to every protocol target. This is the first promotion in #43's phased rollout; the comment about deferring -Werror is replaced with one explaining the phased-promotion pattern. - Verified: Debug+ASan and Release both build all 34 binaries clean. --- cc/oze/include/transaction.hh | 5 ++++- cmake/ProtocolHelpers.cmake | 12 +++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/cc/oze/include/transaction.hh b/cc/oze/include/transaction.hh index 3a179357..9ee458f7 100644 --- a/cc/oze/include/transaction.hh +++ b/cc/oze/include/transaction.hh @@ -225,7 +225,10 @@ public: uint32_t max = expected & 0xffffffff; uint32_t left = *reinterpret_cast(left_key.data()); uint32_t right = *reinterpret_cast(right_key.data()); - uint64_t updated; + // Must start at 0: the |= branches below only write half the + // word each (upper 32b in one, lower 32b in the other), so + // any uninitialized garbage would leak into the result. + uint64_t updated = 0; if (max < right) { updated |= static_cast(right) & 0xffffffff; } else { diff --git a/cmake/ProtocolHelpers.cmake b/cmake/ProtocolHelpers.cmake index c81b6a77..d90233a4 100644 --- a/cmake/ProtocolHelpers.cmake +++ b/cmake/ProtocolHelpers.cmake @@ -42,11 +42,13 @@ function(ccbench_add_protocol name) ${_universal_defs} ${_extra_defs}) - # NOTE: deliberately do NOT call set_compile_options(${target}) here. - # The old per-protocol CMakeLists never enabled -Wall -Wextra -Werror, - # and the existing source tree has accumulated warnings under those - # flags. Turning Werror on here would be a separate cleanup task — - # this issue is just about declarative target wiring. + # Phased -Werror cleanup (see #43). The full -Wall -Wextra -Werror via + # set_compile_options() is still gated on Phase 5 — we promote one + # warning class to error at a time as the existing offenders get + # fixed. Append the next entry here when its source-level cleanup + # PR lands. + target_compile_options(${target} PRIVATE + -Werror=maybe-uninitialized) set_property(TARGET ${target} PROPERTY CCBENCH_PROTOCOL "${name}") set_property(TARGET ${target} PROPERTY CCBENCH_WORKLOAD "${wl}") From e230955c68776fcaafce2f58d654d79155d025a2 Mon Sep 17 00:00:00 2001 From: thawk105 Date: Wed, 13 May 2026 00:43:02 +0000 Subject: [PATCH 2/5] cicada: init pre_ver=nullptr to satisfy GCC 13 -Wmaybe-uninitialized MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CI gcc (13.x) flagged cc/cicada/transaction.cc:489 even though the runtime path that reads pre_ver via compare_exchange_strong is only reachable after the while-loop has assigned to it. GCC 13's flow analysis can't prove that across the four-way condition guarding the read, so it warns. Initialize to nullptr to make the false positive go away — no semantic change. Local gcc 11.4 did not catch this, so the previous PR turned the build red on CI only. --- cc/cicada/transaction.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cc/cicada/transaction.cc b/cc/cicada/transaction.cc index de543f3e..03682023 100644 --- a/cc/cicada/transaction.cc +++ b/cc/cicada/transaction.cc @@ -486,7 +486,12 @@ bool TxExecutor::validation() { if ((*itr).op_ == OpType::INSERT) { continue; } - Version *expected(nullptr), *ver, *pre_ver; + // pre_ver is only read on the else branch below, which is reachable + // only after the while-loop has assigned to it — but GCC 13 cannot + // prove that across the (op != RMW && op != DELETE && !WRITE_LATEST_ONLY + // && ver != expected) condition. Initialize to silence -Wmaybe-uninitialized + // without changing runtime behavior. + Version *expected(nullptr), *ver, *pre_ver = nullptr; for (;;) { if ((*itr).op_ == OpType::RMW || (*itr).op_ == OpType::DELETE || WRITE_LATEST_ONLY) { ver = expected = (*itr).rcdptr_->ldAcqLatest(); From 279abe0e10e11a1e3a6b544fb560cb079e2307cf Mon Sep 17 00:00:00 2001 From: thawk105 Date: Wed, 13 May 2026 00:49:18 +0000 Subject: [PATCH 3/5] mocc: init threshold sentinel to satisfy GCC 13 -Wmaybe-uninitialized MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same pattern as the cicada fix: GCC 13 cannot prove that `threshold` is always written before the RLL_ loop reads it on line 757. At runtime it is — either by the per-violation assignment in the CLL_ scan or by the explicit `if (vioctr == 0) threshold = (Tuple*)-1` afterwards — but the static analysis gives up. Initialize at declaration with the same sentinel (max pointer) the explicit guard would set, and leave the existing guard in place so the intent of the default value stays visible at its original site. --- cc/mocc/transaction.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cc/mocc/transaction.cc b/cc/mocc/transaction.cc index e2b21f3e..2d8e9863 100644 --- a/cc/mocc/transaction.cc +++ b/cc/mocc/transaction.cc @@ -621,7 +621,12 @@ Status TxExecutor::delete_record(Storage s, std::string_view key) { void TxExecutor::lock(Tuple *tuple, bool mode) { unsigned int vioctr = 0; - Tuple* threshold; + // Sentinel "no violation found yet" (max pointer). The CLL_ scan + // below overwrites this on the first violation; the explicit + // `if (vioctr == 0) threshold = (Tuple*)-1` afterwards confirms the + // default. Initializing here keeps GCC 13's -Wmaybe-uninitialized + // happy without changing runtime semantics. + Tuple* threshold = (Tuple*)-1; bool upgrade = false; #ifdef RWLOCK From ab5fc7a95e92831b2b48bc7fedaad29fd8aa9575 Mon Sep 17 00:00:00 2001 From: thawk105 Date: Wed, 13 May 2026 00:52:40 +0000 Subject: [PATCH 4/5] silo: zero LogRecord before init to silence -Wmaybe-uninitialized on padding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LogRecord::computeChkSum() casts `this` to `int*` and sums every int chunk of the object — including any trailing struct padding after val_[VAL_SIZE]. On GCC 13 that padding triggers -Werror=maybe-uninitialized at silo/transaction.cc:447 where a LogRecord is constructed. Zero the entire object in both constructors before assigning members. std::string_view is trivially copyable, so a memset-then-assign sequence is well-defined for that member. --- cc/silo/include/log.hh | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/cc/silo/include/log.hh b/cc/silo/include/log.hh index 5a665b03..8a459eb1 100644 --- a/cc/silo/include/log.hh +++ b/cc/silo/include/log.hh @@ -28,9 +28,17 @@ public: std::string_view key_; char val_[VAL_SIZE]; - LogRecord() : tid_(0), key_("") {} + // computeChkSum() below sums every int-sized chunk of *this*, including + // any trailing struct padding after val_, so both constructors zero + // the entire object first to make those reads well-defined. + LogRecord() { + memset(this, 0, sizeof(LogRecord)); + } - LogRecord(uint64_t tid, std::string_view key, char *val) : tid_(tid), key_(key) { + LogRecord(uint64_t tid, std::string_view key, char *val) { + memset(this, 0, sizeof(LogRecord)); + tid_ = tid; + key_ = key; memcpy(this->val_, val, VAL_SIZE); } From f4e2800830f876852ee7bbbf80a5f2e6bb0e7310 Mon Sep 17 00:00:00 2001 From: thawk105 Date: Wed, 13 May 2026 00:57:02 +0000 Subject: [PATCH 5/5] CLAUDE.md: verify -Werror promotions on the CI compiler (GCC 13) locally This PR turned CI red three times in a row (cicada pre_ver, mocc threshold, silo LogRecord padding) because the devcontainer's GCC 11 is more permissive about -Wmaybe-uninitialized than CI's GCC 13. Each round was a one-line fix surfaced only by CI, which is the wrong loop: cheap to verify locally, expensive to spin CI runners for. Document the mismatch and the two reasonable workflows (PPA install of gcc-13, or a one-shot ubuntu:24.04 docker run), and call out that multiple consecutive CI fixups for a single warning promotion is a signal you skipped this step. --- CLAUDE.md | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 2e356aad..93ccad2e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -134,6 +134,32 @@ if (stat != Status::OK) return false; // do not skip this - **Debug+ASan** (the default top-level Debug build) is the right mode for correctness work — most TPC-C bugs we have caught (use-after-free in `get_and_update_*`, the `cast_to` assertion, the gcRecord UAF) showed up there first and were invisible under pure Release. - **Release** is for benchmark numbers only. CI builds Release without sanitizer (`-DENABLE_SANITIZER=OFF`) — it does not run binaries, just verifies they compile. +### Compiler-version mismatch with CI + +CI runs on `ubuntu-latest` (currently Ubuntu 24.04 → **GCC 13**). The default devcontainer ships **GCC 11**. The two compilers disagree on `-Wmaybe-uninitialized` (and likely other flow-sensitive warnings): GCC 13 catches false-positive-prone cases that GCC 11 lets through. + +When working on the phased `-Werror` cleanup (#43) — or anything else that promotes a warning to error — **verify on GCC 13 locally before pushing**. Three rounds of CI red on PR #44 (`-Wmaybe-uninitialized`) were avoidable by doing this once. Options: + +```sh +# Option 1: PPA (one-time setup, fastest iteration afterwards) +sudo add-apt-repository -y ppa:ubuntu-toolchain-r/test +sudo apt-get update && sudo apt-get install -y gcc-13 g++-13 +CC=gcc-13 CXX=g++-13 cmake -S . -B build-gcc13 -DCMAKE_BUILD_TYPE=Release -DENABLE_SANITIZER=OFF +cmake --build build-gcc13 -j +``` + +```sh +# Option 2: Docker (no host changes; slower because deps re-install each run) +docker run --rm -v "$PWD":/ccbench -w /ccbench ubuntu:24.04 bash -c ' + apt-get update && apt-get install -y --no-install-recommends \ + $(cat build_tools/ubuntu.deps) build-essential pkg-config && \ + cmake -S . -B build-gcc13 -DCMAKE_BUILD_TYPE=Release -DENABLE_SANITIZER=OFF && \ + cmake --build build-gcc13 -j +' +``` + +Pushing a `-Werror=` promotion without a GCC 13 build first counts as "didn't actually verify." + ## Repository layout - [include/](include/) — shared headers (atomics, rwlock, zipf, masstree wrapper, etc.)