Skip to content

oze: 未初期化 ScanRange union を修正 + -Werror=maybe-uninitialized を有効化 - #44

Merged
thawk105 merged 5 commits into
masterfrom
werror-maybe-uninitialized
May 13, 2026
Merged

oze: 未初期化 ScanRange union を修正 + -Werror=maybe-uninitialized を有効化#44
thawk105 merged 5 commits into
masterfrom
werror-maybe-uninitialized

Conversation

@thawk105

Copy link
Copy Markdown
Owner

#43 Phase 1 のサブタスク。-Wmaybe-uninitialized (1 件) を潰し、同 PR で ccbench_add_protocol()-Werror=maybe-uninitialized を追加する。

修正内容

cc/oze/include/transaction.hh:228uint64_t updated; が未初期化のまま宣言され、その下の if/else で 上位 32 bit または下位 32 bit だけに |= していた。残り 32 bit はスタックのガベージのまま compare_exchange で書き戻されていた。

-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<uint64_t>(right) & 0xffffffff;
 } else {
   updated |= static_cast<uint64_t>(max) & 0xffffffff;
 }
 if (left < min) {
   updated |= static_cast<uint64_t>(left) << 32;
 } else {
   updated |= static_cast<uint64_t>(min) << 32;
 }

なぜ単なる hygiene warning ではなく実バグなのか

ScanRangecc/oze/transaction.cc:851 の insert-validation で 挿入キーが scan された bounding box に入るか をチェックする fast-path として使われている (upper-32 = min, lower-32 = max)。これが monotonically growing であることが前提:

new_min = min(current_min, left)
new_max = max(current_max, right)
scan_range = (new_min << 32) | new_max

updated がガベージのままだと:

  • correctness 違反: 見かけの min が実値より小さく、max が実値より大きく見えるパターンでは fast-path は通過するが OK。だが逆に見かけの min がガベージで大きくなり、見かけの max がガベージで小さくなるパターンでは prefix < min || max < prefix の判定が誤って真になり、本来 ScanHistory チェックすべき挿入がスキップされて phantom anomaly につながる可能性
  • perf 劣化のみのパターン: ガベージで範囲が広がりすぎると fast-path が一切短絡せず ScanHistory の線形走査に毎回落ちる

いずれもスタック内容依存。-Wmaybe-uninitialized がまさにこれを拾った。

論文との整合性

Oze PVLDB 論文 (Nemoto et al., PVLDB vol.18 p2321 / extended version arXiv:2210.04179) は phantom 防止を scan history (txid + predicates) を挿入側が validation phase でチェック することで実現すると記述。論文に ScanRange の記述はない — 実装側で ScanHistory の線形走査の前段に置かれた bounding-box フィルタ に相当する性能最適化。論文の正当性契約 (ScanHistory のロジック) は本修正で変更していない。

CMake 側

ccbench_add_protocol() に以下を追加:

target_compile_options(${target} PRIVATE
  -Werror=maybe-uninitialized)

これが #43 ロードマップの最初の promotion。以後の Phase で同様のパターンで一個ずつ -Werror=<flag> を足していく。

Test plan

  • rm -rf build && cmake -S . -B build -DCMAKE_BUILD_TYPE=Debug && cmake --build build -j で 34/34 ビルド成功
  • cmake -S . -B build-release -DCMAKE_BUILD_TYPE=Release -DENABLE_SANITIZER=OFF && cmake --build build-release -j で 34/34 ビルド成功
  • CI 緑

thawk105 added 5 commits May 13, 2026 00:39
…alized

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.
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.
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.
…padding

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.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant