[BUG] With NDEBUG, early-returning without SkipWithError leads to indefinite retries

[adrianroos]

Describe the bug
With NDEBUG, early-returning without SkipWithError leads to indefinite retries

System
Which OS, compiler, and compiler version are you using:

• OS: 6.5.13-1rodete2-amd64 (building for Android cuttlefish)
• Compiler and version: clang --version: Android (11368308, +pgo, +bolt, +lto, +mlgo, based on r510928) clang version 18.0.0 (https://android.googlesource.com/toolchain/llvm-project 477610d4d0d988e69dbc3fae4fe86bff3f07f2b5)

To reproduce
Steps to reproduce the behavior:

1. Run https://cs.android.com/android/_/android/platform/hardware/interfaces/+/f850de6732287b3c5539a56751d21a4a1b170f42:vibrator/bench/benchmark.cpp;bpv=1;bpt=0;drc=e29acd8194c6df2fc81613cebdb2dc4507ec4c1b
2. Benchmark prints the context, then hangs

Note how the benchmark functions early return before reaching the measure loop. Also note that Android compiles with NDEBUG set.

Minimal repro case (compile with -DNDEBUG=1)

static void BM_Foobar(benchmark::State& state) {
  return;
}
// Register the function as a benchmark
BENCHMARK(BM_Foobar);

Expected behavior

Prints an error & skips / aborts, instead of retrying indefinitely.

Root cause

benchmark/src/benchmark_runner.cc

Line 133 in b04cec1

BM_CHECK(st.skipped() || st.iterations() >= st.max_iterations)

does not trigger an abort if NDEBUG is set; instead, the benchmark function gets retried indefinitely.

Verbose logs:

-- LOG(2): Running VibratorBench_V1_0/on for 1
-- LOG(2): Ran in 0/0
-- LOG(2): Running VibratorBench_V1_0/on for 1
-- LOG(2): Ran in 0/0
-- LOG(2): Running VibratorBench_V1_0/on for 1
-- LOG(2): Ran in 0/0
-- LOG(2): Running VibratorBench_V1_0/on for 1
-- LOG(2): Ran in 0/0
-- LOG(2): Running VibratorBench_V1_0/on for 1
-- LOG(2): Ran in 0/0
-- LOG(2): Running VibratorBench_V1_0/on for 1
-- LOG(2): Ran in 0/0
-- LOG(2): Running VibratorBench_V1_0/on for 1
-- LOG(2): Ran in 0/0
-- LOG(2): Running VibratorBench_V1_0/on for 1

[adrianroos]

(root cause of non-public https://issuetracker.google.com/issues/302845046)

[dmah42]

if run in DEBUG i assume the CHECK fires, so you know this is invalid behaviour from the POV of the library, right?

[adrianroos]

Yes, well understood. However, Android's benchmarks all run with NDEBUG in CI, and not having any indication that something is wrong with the benchmark makes it rather hard to diagnose the issue.

I expected a benchmarking library to report an error when the benchmark violates this invariant, even with NDEBUG.

[dmah42]

originally, we were very careful to minimise overhead of BM_CHECK in NDEBUG in case it was used in the critical path; we don't want to be timing our CHECKs.

i agree that perhaps this one can be bumped up to something that also checks in NDEBUG, but we don't currently have that outside of PrintErrorAndDie in sysinfo.cc.

i'd be comfortable with a BM_ASSERT or similar macro that is enabled in NDEBUG, but we have to be careful where it's introduced.

[LebedevRI]

I agree with @dmah42. Though perhaps BM_CHECK_RUNTIME, not BM_ASSERT.