From dbc6ac486bf1df5df9cd73302d2eceb36eaf6868 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sun, 11 Oct 2026 18:25:52 +0200 Subject: [PATCH] fix(msan): reports print with fast unwinding; five suites run again Five suites (grammar_regression grammar_labels pipeline lang_contract grammar_probe_e) were excluded from the MSan lane for "stack-overflow under instrumentation". That was never deep recursion. MSan aborted while REPORTING real uninitialized reads: - every abort was on the main thread. MSan's "T" is the OS thread id and equalled the pid in all five x86-64 aborts (CI job 91868259558); the old block read it as a worker-thread ordinal, so the worker-stack knob, MSAN_ORIGINS and CBM_WORKERS=1 could never change anything; - with the default slow unwinder (the image's MSan-instrumented libunwind) a report exhausted the main thread's 256 MiB stack, and the overflow handler then failed with "nested bug in the same thread"; - with fast_unwind_on_fatal=1 the same test prints the use-of-uninitialized-value report instead (measured on one tree and test: slow unwind aborts at the stack limit, fast unwind reports wit/scanner.c:13). The two reads are fixed in the previous commits of this series, so: - MSAN_OPTIONS defaults to fast_unwind_on_fatal=1 (the build already keeps frame pointers); - MSAN_EXCLUDE is down to cli, whose install-path failure is a separate, still-undiagnosed cause; - the known-red block, the header (it still claimed CI runs without exclusions) and the MSAN_ORIGINS and stack comments say what is true; - the partial-lane warning and the exclude-name typo check lived in a branch behind `elif true` that never ran. They now run in the per-suite path, the check as a case match instead of a grep -q pipe that pipefail can misreport; - the comments in scripts/ci/msan-lane.sh and above the test-msan job in _test.yml no longer say the five suites overflow. Signed-off-by: Martin Vogel --- .github/workflows/_test.yml | 11 ++- scripts/ci/msan-lane.sh | 15 ++-- scripts/msan.sh | 154 ++++++++++++++++-------------------- 3 files changed, 83 insertions(+), 97 deletions(-) diff --git a/.github/workflows/_test.yml b/.github/workflows/_test.yml index 2f8c2af76..1b04d42d5 100644 --- a/.github/workflows/_test.yml +++ b/.github/workflows/_test.yml @@ -370,16 +370,19 @@ jobs: # artifact — the local ladder cannot answer that, having no faithful x86-64 # emulation. It has now answered it, and the answer SPLIT the list: # - # - the five deep-recursion suites overflow their thread stacks on x86-64 - # too, so that limit is NOT architectural; + # - five suites aborted with "stack-overflow ... nested bug" on both + # architectures. That was not deep recursion: it was MSan crashing while + # it reported real uninitialized reads (reporting with the slow unwinder + # exhausted the main thread's stack). scripts/msan.sh now unwinds with + # frame pointers, the + # reports print, the reads are fixed, and the five suites run again; # - `cli` does not overflow here at all — it fails in the install path for # an unrelated, still-undiagnosed reason; # - `incremental` was a shadow-memory RSS artifact, now fixed in the test # itself rather than skipped, so it stays in the lane. # # Per-cause detail lives in scripts/msan.sh, which is the single authoritative - # list both venues now share. Zero use-of-uninitialized-value findings were - # reported across the lane on either architecture. + # list both venues now share. test-msan: if: ${{ inputs.lanes == 'all' || contains(fromJSON(inputs.lanes), 'msan') }} runs-on: ubuntu-latest diff --git a/scripts/ci/msan-lane.sh b/scripts/ci/msan-lane.sh index 311258596..c8e4f3a10 100755 --- a/scripts/ci/msan-lane.sh +++ b/scripts/ci/msan-lane.sh @@ -38,13 +38,14 @@ run_suite() { # excluded suite and the cause it is excluded for). # # This leg used to force it empty to settle whether the local arm64 - # exclusions were an aarch64 artifact. It answered that: the five - # deep-recursion suites overflow on x86-64 too, `cli` fails for an unrelated - # install-path reason, and `incremental` was a shadow-memory RSS artifact - # that is now fixed in the test rather than skipped. Keeping the override - # would re-red the gate for causes already recorded, so the question is - # closed and the venues share one list. scripts/msan.sh still warns loudly - # that the lane is partial, which is the honest signal to keep. + # exclusions were an aarch64 artifact. They were not: `cli` fails for an + # install-path reason, `incremental` was a shadow-memory RSS artifact (now + # fixed in the test), and the five suites that "overflowed" on both + # architectures were hiding real uninitialized reads whose reports crashed + # under the slow unwinder. With fast unwinding (scripts/msan.sh) those reports + # print, the reads are fixed, and the five suites are back in the lane. + # Only `cli` is still excluded, and scripts/msan.sh warns that the lane is + # partial while anything is. echo "=== MSan suite (exclusions per scripts/msan.sh) ===" docker run --rm -v "$PWD:/src" -w /src "$IMAGE" } diff --git a/scripts/msan.sh b/scripts/msan.sh index 954608890..20bb8fafe 100755 --- a/scripts/msan.sh +++ b/scripts/msan.sh @@ -9,13 +9,9 @@ # the clang-analyzer lane do not cover dynamically. halt_on_error stays ON: # a finding is a bug (or an interceptor gap to triage), never board data. # -# VENUES: the CI test-msan job (x86-64) is AUTHORITATIVE and runs with no -# exclusions. The local container is arm64, where deep-recursion suites -# overflow their thread stacks under instrumentation — MSan's shadow/stack -# handling is materially better supported on x86-64, so that limitation may -# be architectural, and the local ladder has no faithful x86-64 emulation to -# decide it. An accepted venue divergence for this lane specifically: the -# exclusions below are the LOCAL default only, and CI overrides them away. +# VENUES: the CI test-msan job (x86-64, authoritative) and the local arm64 +# container (test-infrastructure/docker-compose.yml, service test-msan) both +# run this script with the same exclusion list below. # # Usage: scripts/msan.sh [suite ...] (default: full suite) set -euo pipefail @@ -33,12 +29,10 @@ fi # one cannot be shadowed); its headers live under the MSan prefix. # MSAN_ORIGINS: 2 = full origin chains (best reports, heaviest frames), # 1 = immediate origin only, 0 = none. Detection is IDENTICAL at every level; -# only report quality differs, so lowering it is the first lever to try when -# frame inflation overflows a deep-recursion suite. -# Default 1 on this lane: identical DETECTION at every level (only report -# depth differs), and the frame savings are what lets the deep-recursion -# grammar suites fit their stacks under instrumentation. Export -# MSAN_ORIGINS=2 locally when chasing a specific report's origin chain. +# only report quality differs. +# Default 1 on this lane: the immediate origin is enough to locate a read, and +# the lighter frames keep the lane cheaper. Export MSAN_ORIGINS=2 locally when +# chasing a specific report's origin chain. MSAN_ORIGINS="${MSAN_ORIGINS:-1}" if [ "$MSAN_ORIGINS" = "0" ]; then MSAN_ORIGIN_FLAG="" @@ -70,11 +64,20 @@ make -j"$(nproc)" -f Makefile.cbm build/msan/test-runner \ CXX_STDLIB_FLAGS="-stdlib=libc++ -nostdinc++ -isystem $MSAN_PREFIX/include/c++/v1" \ CXX_STDLIB="-L$MSAN_PREFIX/lib -Wl,-rpath,$MSAN_PREFIX/lib -lc++ -lc++abi" -export MSAN_OPTIONS="${MSAN_OPTIONS:-halt_on_error=1:print_stats=0}" -# Origin tracking inflates every frame; the grammar-corpus suites drive the -# deepest parser recursion in the tree. Raise what can be raised (main-thread -# stack via RLIMIT_STACK, worker stacks via the sanitized-build-only knob in -# cbm_thread_create) — see the exclusion note below for what this does NOT fix. +# fast_unwind_on_fatal=1: a report's stack is walked with frame pointers (this +# build keeps them, -fno-omit-frame-pointer) instead of the default slow +# unwinder, which in this image is the MSan-instrumented libunwind. With the +# slow unwinder, reporting an uninitialized read exhausted the main thread's +# stack, so the report never printed and MSan aborted with "stack-overflow" +# followed by "nested bug in the same thread" instead. That disguise kept five +# suites excluded as deep recursion while they hid a real read (the WIT +# scanner's create function returned nothing; see the grammar MANIFEST.md). +# Measured on the same tree and test: slow unwind aborts at the 256 MiB stack +# limit, fast unwind prints the use-of-uninitialized-value report. +export MSAN_OPTIONS="${MSAN_OPTIONS:-halt_on_error=1:print_stats=0:fast_unwind_on_fatal=1}" +# Instrumented frames are several times larger than plain ones, so give the +# stacks headroom: the main thread via RLIMIT_STACK, worker threads via the +# sanitized-build-only knob in cbm_thread_create. ulimit -s 262144 2>/dev/null || true export CBM_THREAD_STACK_MB="${CBM_THREAD_STACK_MB:-256}" export LD_LIBRARY_PATH="$MSAN_PREFIX/lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" @@ -102,22 +105,22 @@ fi echo "=== MSan lane: $(clang --version | head -1) ===" -# KNOWN RED — THREE DISTINCT CAUSES, whitelisted per cause (O10) +# KNOWN RED — whitelisted per cause (O10). Only (B) is still excluded. # -# The x86-64 CI leg deliberately runs this lane WITHOUT these exclusions, to -# settle which limits are architectural. It has now done so, and it SPLIT the -# seven suites an earlier version of this block lumped together as one cause. -# That earlier claim -- "seven suites abort with stack-overflow" -- was wrong -# for two of them and is corrected here. -# -# (A) STACK-OVERFLOW under instrumentation — FIVE suites: -# grammar_regression grammar_labels pipeline lang_contract grammar_probe_e -# Each aborts with "MemorySanitizer: stack-overflow" followed by "nested bug -# in the same thread, aborting". The fault is always the same pc (inside -# MSan's memset interceptor -- the instruction that happens to touch the -# guard page, not the recursion source) on a pipeline worker thread. It -# reproduces on BOTH arm64 (local) and x86-64 (CI logged 5 overflows), so it -# is NOT the aarch64 shadow-mapping artifact an earlier note here claimed. +# (A) "STACK-OVERFLOW" — FIXED, no longer excluded. Five suites +# (grammar_regression grammar_labels pipeline lang_contract +# grammar_probe_e) aborted on both arm64 and x86-64 with "MemorySanitizer: +# stack-overflow" followed by "nested bug in the same thread, aborting". +# It was never deep recursion. MSan crashed while REPORTING real +# uninitialized reads (see fast_unwind_on_fatal above): the WIT scanner's +# create function returned nothing (the four grammar suites), and +# githistory_coupling_limits_output left its commits' timestamps +# uninitialized (pipeline). Both are fixed and the suites are back in the +# lane. The crashing thread was the MAIN thread: MSan's "T" is the OS +# thread id, and every abort showed T equal to the pid. An earlier +# version of this block read it as a worker-thread ordinal, which is why +# the worker-stack knob, MSAN_ORIGINS and CBM_WORKERS=1 never changed +# anything and why it proposed a byte-measuring recursion guard. # # (B) INSTALL/ACTIVATION failures — cli. NOT an overflow. On x86-64 the suite # runs to completion: 253 passed, 5 failed, every failure in the install or @@ -126,10 +129,9 @@ echo "=== MSan lane: $(clang --version | head -1) ===" # suite is green on every other venue. MSan reported ZERO # use-of-uninitialized-value here, so excluding it costs no uninit coverage # -- which is the only thing this lane exists to provide. -# NOT DIAGNOSED, and honestly so: the local lane is arm64, where these -# suites hit (A) long before reaching this code, so there is no faithful -# venue to iterate in and each attempt costs a ~30min CI round trip. -# Recorded as a follow-up rather than guessed at from a log. +# NOT DIAGNOSED yet. Recorded as a follow-up rather than guessed at from a +# log; with (A) understood, the local arm64 container is a usable venue +# for it (scripts/msan.sh cli). # # (C) RSS BUDGET — incremental. FIXED rather than excluded, so it is no longer # in the list below: the budget assertion is now skipped under @@ -139,51 +141,45 @@ echo "=== MSan lane: $(clang --version | head -1) ===" # instead would blind the guard on the platforms where it does work. The # suite stays IN this lane and keeps its uninitialized-read coverage. # -# WHAT WAS TRIED for (A) — each disproven by measurement, do not repeat: -# 1. RLIMIT_STACK 8 -> 64 -> 256 MiB, and `ulimit -s unlimited`. No effect; -# the crashing thread is not the main thread. -# 2. CBM_THREAD_STACK_MB at 256 MiB and at 1024 MiB (the cap). The knob is -# verified compiled in (CBM_SANITIZED_BUILD is defined for this lane) and -# the floor is applied in cbm_thread_create for both the default and -# explicit-size paths. The fault address did not move by a single byte -# between 256 MiB and 1024 MiB -- a 4x stack increase changing nothing is -# what rules out "the stack is merely too small". -# 3. MSAN_ORIGINS 2 -> 1 -> 0. Detection is identical at every level and 0 -# gives the smallest frames; same thread, same address. Frame inflation is -# not the trigger. -# 4. One suite per process (the sharding below). It removed the cumulative -# thread-ordinal effect an earlier note suspected, and thousands of tests -# now run before the wall, but the deep suites still abort. -# 5. CBM_WORKERS=1, to push the recursion onto the main thread where the -# rlimit does apply. Still a worker thread, still overflows. +# WHY (B) IS EXCLUDED RATHER THAN LEFT RED: the lane is gating. A permanently +# red gate teaches everyone to ignore it and hides the findings the other +# suites produce, which is the entire reason this lane exists. The exclusion is +# narrow, named, and expires when the install failure is diagnosed. It is not a +# claim that cli is covered, and the lane says so when it runs (below). # -# WHAT THIS POINTS AT (the follow-up, not a guess to act on blindly): the -# recursion guards this tree does have -- see the stack_overflow_a/b/c suites, -# which pass -- bound DEPTH, while the resource actually exhausted is BYTES. -# Instrumented frames are several times larger, so the byte budget is gone -# before the depth counter trips. If that is right, the fix is a guard that -# measures remaining stack rather than counted depth, and it would make these -# suites pass under every sanitizer rather than papering over one lane. -# -# WHY (A) AND (B) ARE EXCLUDED RATHER THAN LEFT RED: the lane is gating. A -# permanently red gate teaches everyone to ignore it, and it hides the -# uninitialized-read findings the other ~130 suites DO produce -- which is the -# entire reason this lane exists. Each exclusion is narrow, named, and expires -# with its own fix: (A) when the recursion guard measures bytes, (B) when the -# install failure is diagnosed on a venue that can run it. Neither is a claim -# that those suites are covered. -MSAN_EXCLUDE="${MSAN_EXCLUDE-grammar_regression grammar_labels pipeline cli lang_contract grammar_probe_e}" +# LESSON FROM (A): an MSan abort that is not a use-of-uninitialized-value +# report is a reporting failure until proven otherwise. Look for the read +# first (fast unwinding prints it), not for a stack limit. +MSAN_EXCLUDE="${MSAN_EXCLUDE-cli}" if [ "$#" -gt 0 ]; then ./build/msan/test-runner "$@" -elif true; then +else # One suite per process: a fresh process per suite keeps one suite's # thread/allocator state from reaching the next, and makes a failure name # exactly one suite. Suite enumeration comes from --list-suites, whose # completeness the sharding union guard already proves; the known-red set # documented above is skipped by name and nothing else is. + all_suites="$(./build/msan/test-runner --list-suites)" + # Fail loudly if an entry matches no suite: a typo (or a TEST name given + # where a SUITE name is required) would otherwise exclude nothing silently. + # A case match, not `printf | grep -q`, which pipefail can turn into a + # false failure when grep exits early. + for e in $MSAN_EXCLUDE; do + case $'\n'"$all_suites"$'\n' in + *$'\n'"$e"$'\n'*) ;; + *) + echo "FATAL: MSAN_EXCLUDE entry '$e' is not a suite name" >&2 + exit 1 + ;; + esac + done + if [ -n "$MSAN_EXCLUDE" ]; then + echo "WARNING: running a PARTIAL sanitizer lane — excluded: $MSAN_EXCLUDE" >&2 + echo "WARNING: a green result here does NOT mean the tree is MSan-clean." >&2 + fi fails="" - for suite in $(./build/msan/test-runner --list-suites); do + for suite in $all_suites; do case " $MSAN_EXCLUDE " in *" $suite "*) echo "=== msan: $suite SKIPPED (known-red, see the block above) ===" @@ -197,18 +193,4 @@ elif true; then echo "=== MSan lane FAILED suites:$fails ===" >&2 exit 1 fi -else - echo "WARNING: running a PARTIAL sanitizer lane — excluded: $MSAN_EXCLUDE" >&2 - echo "WARNING: a green result here does NOT mean the tree is MSan-clean." >&2 - excl_pattern="$(printf '%s\n' $MSAN_EXCLUDE | tr '\n' '|' | sed 's/|$//')" - suites="$(./build/msan/test-runner --list-suites | grep -Evx "$excl_pattern")" - # Fail loudly if an entry matched no suite: a typo (or a TEST name given - # where a SUITE name is required) would otherwise exclude nothing silently. - for e in $MSAN_EXCLUDE; do - ./build/msan/test-runner --list-suites | grep -qx "$e" || { - echo "FATAL: MSAN_EXCLUDE entry '$e' is not a suite name" >&2; exit 1; } - done - echo "--- excluded: $MSAN_EXCLUDE (see the comment in $0) ---" - # shellcheck disable=SC2086 # deliberate word-splitting of the suite list - ./build/msan/test-runner $suites fi