tree 2c6d49388d943d89eab84f631b7487973ff6fb4a
parent 82a89508915e7607e10675ee5a4dacd8fc72c649
author Tejas Anil Nagmote <tejasnagmote520@gmail.com> 1789986299 +0530
committer GitHub <noreply@github.com> 1789986299 +0100
gpgsig -----BEGIN PGP SIGNATURE-----
 
 wsFcBAABCAAQBQJqsQX7CRC1aQ7uu5UhlAAAJAgQAA8jK4tOMH44xTeYSrKhKY3/
 HCL3r5/9UxgoLXSQsz+1gpmLSmNbhUDbmUTxtmlZ+tgw3fS5Ojx6aP3gF+kGGDf2
 VJY3Pszt+voZ5/L01QRmqz6zFfxRDT5b1G5CtQeayyFtL3aP7ql1zT2dzIZisd5E
 XGe/MoCQa+7DweJcnZ0dNVx8x/WYq9qqj7qc7cs4rydV/6ck8KLytXsLVcRNdpPB
 yJ7Z+blZwmv3POpVBoxq2YtQNynyx8ZfMej6sInwi7dqBV0JZRpYNlrOecxfbaC7
 I/BZkFcS+zsQPiee6nEHykOCfXZWRTCqgeBEHQ/+fivn+l99oYxxEJO79/k3HDXL
 VAO9qupBTHPVLiy7byRQ8OrMCDqkOBnlLvyT0eEvx7WCYExvotRmHQ/AvhKojAA2
 wqk2aCBk3UUb1aFnL7pVST9yEEzIIBvWJhLN+oFhii/BhA4fXbYcNJ5aMhbfCckW
 dnV4Beuv38AlWz0UNGtBgxQmK2oLeRMGFZTXUtw4F7/MNYIcM5qbDQFtzybcowas
 /n+YaYzB8EKkbbWPPnJGG27iklECFpsXnzUa08du3XvuMpfOLd3zfcStmVgh0W6/
 gTAlcXvfyJEzDXHQPocoEic1/c5d/fWpwbmvttu1gUJSjl4++skex6rI9ZBpuk2u
 cLV+z/mm1GWPhOBJCm9C
 =1SxG
 -----END PGP SIGNATURE-----
 

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

* Move the PauseTiming()/ResumeTiming() assertions to the caller (#2235)

Calling PauseTiming() outside the benchmark loop stops a timer that was never
started, so StopTimer() adds `ChronoClockNow() - 0` to the run's real time --
an absolute clock reading, not a duration. The same happens to the CPU time.
That is the ~86 s offset in #2235, and why every later benchmark in the
process reports a bigger number.

The precondition is already checked. The problem is where: BM_CHECK is
compiled into the library, so it only fires if the library was built with
assertions. Distributions ship a release build, and then the check is gone no
matter how the benchmark itself was compiled.

Move it. PauseTiming() and ResumeTiming() become inline wrappers in state.h
that assert and call PauseTimingImpl()/ResumeTimingImpl(), which hold the
existing bodies. The condition is the same and the runtime behaviour is the
same; the assertion now follows the NDEBUG of whoever writes the benchmark,
and disappears once they define it.

diagnostics_test caught what BM_CHECK threw through the library's abort
handler. A plain assert() does not use that handler, so the test becomes
diagnostics_gtest with ASSERT_DEATH_IF_SUPPORTED, like min_time_parse_gtest
and profiler_manager_gtest. It covers pause and resume before and after the
loop, and checks that a run which is not diagnosed still reports a real and
CPU time below a second.

* Rewrite death tests

* Force-inline PauseTiming()/ResumeTiming() for MSVC shared builds

State is BENCHMARK_EXPORT, so a program linking the shared library sees
it as __declspec(dllimport). MSVC does not expand these two inline
wrappers at the call site; it calls the copies exported from
benchmark.dll instead, and those were compiled with the library's
NDEBUG. In Release/shared the assertion is therefore gone, which is why
diagnostics_gtest failed to die on exactly the two windows *.Release.shared
jobs while static and Debug builds passed. (A Debug DLL keeps its
asserts, so the same call into the DLL still fires there.)

An inline dllimport member without the assert is expanded normally; it
is the body with assert() that MSVC declines to inline. Marking the
wrappers BENCHMARK_ALWAYS_INLINE (__forceinline on MSVC), as begin() and
end() already are, puts the check back in the caller's translation unit.

Checked on MSVC 19.40 and 19.latest x64: at /O2 and /O2 /Ob1 the caller
now contains the _wassert call and a tail call to PauseTimingImpl(); with
NDEBUG only the tail call remains. No C4714 at /W4 /WX.

* Move the PauseTiming()/ResumeTiming() tests into benchmark_gtest

As asked in review, the tests now live in the existing benchmark_gtest.cc
instead of a new diagnostics_gtest.cc. Only the suite names changed.

* Drop issue reference from timing test comment

* Reword the PauseTiming()/ResumeTiming() docs note

Use the wording suggested in review: state the precondition for each
call directly rather than describing the failure mode, and note that
invalid calls are diagnosed when user code is built without NDEBUG.

---------

Co-authored-by: Roman Lebedev <lebedev.ri@gmail.com>
Co-authored-by: dominic <510002+dmah42@users.noreply.github.com>