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.) 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(); 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 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/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); } 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}")