Diagnose invalid PauseTiming()/ResumeTiming() calls (#2235)

[devtejasx]

PauseTiming() and ResumeTiming() guarded their preconditions with BM_CHECK, which compiles away under NDEBUG. In a release build, calling PauseTiming() outside the benchmark loop -- e.g. before it, to try to exclude setup work -- therefore reached ThreadTimer::StopTimer() with a timer that was never started, and accumulated ChronoClockNow() - 0, an absolute clock reading, into the run's real time. The same happens to the CPU time. Every benchmark in the process then reports a huge constant offset plus whatever the clocks had advanced by, which is why the results in #2235 grow monotonically with registration order and are unrelated to the work being measured.

Turn the preconditions into a reported benchmark error: an invalid call now leaves the timer untouched and skips the run with a message naming the misuse, in every build configuration. The user sees what went wrong instead of plausible-looking numbers, and a run that would have been garbage is no longer reported as a result.

Rewrite diagnostics_test to cover the new contract -- pause/resume before the loop, pause after it, and pausing or resuming twice -- in both debug and release builds; previously it could only run in debug, since the behaviour it checked existed only there.

---

Per AGENTS.md: AI-assisted — the patch was drafted with AI assistance
(Claude) and then reviewed, tested, and understood by me. I take full
responsibility for it.

[dmah42]

what impact does this have on the generated assembly for the core timing loop? did you consider why we keep these methods light weight or did you just point some AI crap at the project and suggest changes without a deeper understanding of the library?

[devtejasx]

Fair question. I've reworked the patch around it and measured.

The core timing loop is not touched. KeepRunning(), KeepRunningBatch(), KeepRunningInternal() and StateIterator all live in state.h, which this PR does not modify. Compiling the same benchmark translation unit against the library before and after, gcc 15 at -O2, objdump -d of the benchmark function is byte-for-byte identical.

PauseTiming()/ResumeTiming() themselves. The condition now keys off the timer rather than the loop flags: the timer runs exactly between the first and the last iteration and is stopped while timing is paused, so timer_->running() on its own answers "are we inside the loop with the timer going". What that adds to the fast path is

mov  0x98(%rcx),%rbx     ; timer_
cmpb $0x0,0x1(%rbx)      ; timer_->running_
je   <cold path>

one load and a predicted-not-taken branch, in a function that already reads two clocks (ChronoClockNow() and ReadCpuTimerOfChoice()). The message is handed to an out-of-line helper so the std::string temporary that SkipWithError() takes is built on the cold path rather than here.

AI assistance is disclosed in the description, per AGENTS.md. On the substance: BM_CHECK compiles out under NDEBUG, so in a release build PauseTiming() before the loop reaches ThreadTimer::StopTimer() with start_real_time_ == 0 and accumulates ChronoClockNow() - 0 — an absolute clock reading rather than a duration — into the run's real time, and likewise for CPU time. That is what produces the fixed ~86 s offset in #2235 and why every subsequent benchmark in the process reports a larger number regardless of registration order.

If the position is that misuse of these two functions is a debug-only concern, that is a reasonable call and I'll close this. The reason I thought it was worth raising is that the release-build symptom is plausible-looking wrong numbers rather than a crash.

[dmah42]

why is this check !_timer_->running() rather than the original started_ && !finished_ && !skipped()?

[dmah42]

same here. this has changed the check

[dmah42]

this comment makes no sense. no temporary is "built" as the strings are literals.

[dmah42]

having these be global increases the risk of order dependent testing if tests accidentally share the variables. is there any reason why they're global? presumably they can be local to the benchmark and asserted on after the for loop in each case.

[dmah42]

this is a big red flag right here.

[LebedevRI]

I don't like this.
Can we instead move State::PauseTiming() into a header, use normal assert(),
and then call the original State::PauseTiming() renamed as State::PauseTimingImpl() and kept in a non-header?

[devtejasx]

Done — rebuilt the way you suggested, @LebedevRI.

PauseTiming() and ResumeTiming() are now small inline wrappers in state.h that assert() and forward to PauseTimingImpl()/ResumeTimingImpl(), which keep the existing bodies in benchmark.cc.

@dmah42 — that also answers both of your questions on the check. The predicate is back to the original started_ && !finished_ && !skipped(); the timer-based form is gone, along with the comment about the temporary. What the change amounts to now is where the assertion is evaluated: it was compiled into the library, so whether it fires depends on how the library was built, and distributions ship a release build — the check is then absent no matter how the benchmark itself was compiled. As an inline wrapper it follows the NDEBUG of whoever writes the benchmark, and costs nothing once they define it.

One consequence worth flagging: diagnostics_test worked by installing the library's abort handler and catching what BM_CHECK threw. A plain assert() does not go through that handler, so the test cannot work as written. It is now diagnostics_gtest using ASSERT_DEATH_IF_SUPPORTED, the same pattern as min_time_parse_gtest and profiler_manager_gtest — which also removes the globals you flagged, since each case is a separate benchmark and the death test runs in its own process. There is one non-death case asserting that the documented pause/resume-inside-the-loop usage still runs.

Release and debug suites pass.

[LebedevRI]

what about pause of paused, resume of resumed, pause of skipped, resume of skipped, etc?

[LebedevRI]

And note that skipping can happen before/during/after the loop, and time may be either running or paused, so some more variations to test.
Oh, and we should actually test that the timings reported aren't absurd in the cases that aren't diagnosed.

[devtejasx]

Covered, @LebedevRI — and working through the list changed the patch, not just the test.

pause of paused / resume of resumed. started_ && !finished_ && !skipped() does not catch either, and both corrupt the accumulated time exactly the way #2235 does. The assertion now includes the timer state via a TimerIsRunning() accessor that is only ever named from inside the assertions, so it is never called once NDEBUG is defined. (ThreadTimer is not visible in the public header, hence the accessor rather than touching timer_ directly.)

pause of skipped / resume of skipped. These must not be diagnosed, and the test proves it: with an assertion there, BM_skipped_while_paused — which is just

for (auto _ : state) {
  benchmark::ScopedPauseTiming pause(state);
  state.SkipWithMessage("...");
}

— aborts on the ScopedPauseTiming destructor. SkipWith*() has already stopped the timer and the run is being abandoned, so both calls now return early when the state is skipped. I checked that by removing the guard and re-running:

Assertion failed: started_ && !finished_ && !TimerIsRunning() && "ResumeTiming() called ..."

The variations. diagnostics_gtest now covers pause and resume before the loop, pause and resume after it, pause of paused, resume of resumed, and skipping before the loop, during it with the timer running, during it with timing paused, and after it — eleven benchmarks, each Iterations(1), whole file runs in about a second.

Timings that are not diagnosed. The last test captures the report through a reporter and asserts the real and CPU time of the undiagnosed run are non-negative and under a second; the bug this PR is about produces values in the tens of billions of nanoseconds, so the bound is loose on purpose. One limitation worth stating: the tests are compiled with -UNDEBUG, so this cannot exercise what an NDEBUG build does with the same misuse — there, the calls are simply unchecked, as they are on main today.

[LebedevRI]

Am i talking to an LLM or did human write last comment?

[devtejasx]

I wrote that comment myself. I do use AI as a drafting tool (and disclosed that in the previous PR), but I review, test, and understand every change before submitting it. The previous reply was too long and probably read like an LLM-generated summary rather than a code review discussion—that's on me. I'll keep the responses focused on the specific technical questions from here.

[LebedevRI]

Does TimerIsRunning() return true when outside of the measuring loop?

[devtejasx]

Thanks, I went through those cases and updated both the implementation and the tests.

I found that calling PauseTiming or ResumeTiming more than once also gives incorrect timing results, so the tests now check for those cases too.

For skipped benchmarks, PauseTiming and ResumeTiming should not fail. If a benchmark is skipped while timing is paused, ScopedPauseTiming will later call ResumeTiming during cleanup. Since the benchmark has already been skipped and timing has already stopped, that call is simply ignored.

I also added tests for

Calling PauseTiming and ResumeTiming before the loop
Calling PauseTiming and ResumeTiming after the loop
Calling PauseTiming twice
Calling ResumeTiming twice
Skipping a benchmark before, during, and after the loop, including while timing is paused

I also added a test for the normal case to make sure valid benchmarks still report reasonable timing values.

This description focuses on the expected behavior instead of the internal implementation details.

[LebedevRI]

I'm confused, why are we relaxing the check, and now always adding control flow to every callsite that wasn't there before?

[devtejasx]

The change is meant to keep scopedpauseTiming wroking correctly .when skipwith is called,the benchmark has already stopped and been marked as skipped.if a benchmark::scopepausetiming object is still active,its destructor will automatically call resumetiming.making pausetiming and resumetiming do nothing after a skip allow that automatic cleanup to finish safely instead of triggering an assertion .This behavior only applies after a benchmark has been skipped .neomal benchmark behaviour and checks for incorrect API usage stay the same

[LebedevRI]

But you see the problem, right? The PR is intended to simply move the existing assertion so that it actually triggers not just when the whole library is built with assertions.
And now it also contains subtle behavioural changes.

[devtejasx]

Yes, I see the concern. The original goal was only to make the existing misuse checks apply based on the benchmark user's build configuration instead of the library build configuration.
The extra behavior changes came from trying to handle the edge cases uncovered while adding tests. I agree that they make the patch harder to review because they mix two different changes.these should be reply
I will split this back to the original intent: move the assertions into the caller-visible header path without changing the runtime behavior. If we want to change the skipped benchmark behavior or other edge cases, that can be discussed separately.

[devtejasx]

Reverted. The patch is back to only moving the assertion.

void PauseTiming() {
  assert(started_ && !finished_ && !skipped() &&
         "PauseTiming() called outside of the benchmark loop");
  PauseTimingImpl();
}

Same condition as the BM_CHECK it replaces. No skipped() early return, no TimerIsRunning(), no added control flow at any call site. Under NDEBUG the wrapper is empty and the call goes straight to PauseTimingImpl(), exactly as now.

To answer the two questions in case they matter later:

TimerIsRunning() was timer_->running(). It is false outside the loop and also false while timing is paused, which is how it caught pause-of-paused.

The skipped() early return was for ScopedPauseTiming. Its destructor calls ResumeTiming(), so a benchmark that calls SkipWithError() inside a ScopedPauseTiming scope trips the assert on the way out. That is pre-existing behaviour, not something this PR should change.

Both are behaviour changes, so they are out. I can file the double-pause case as its own issue if it is worth fixing.

Tests are back to the four out-of-loop cases, plus one check that a run which is not diagnosed reports a real and CPU time below a second. Release and debug both pass.