Skip to content

Diagnose a non-growing RangeMultiplier in release builds (#1504) - #2280

Closed
devtejasx wants to merge 1 commit into
google:mainfrom
devtejasx:runtime-check-range-multiplier
Closed

devtejasx wants to merge 1 commit into
google:mainfrom
devtejasx:runtime-check-range-multiplier

Conversation

@devtejasx

Copy link
Copy Markdown
Contributor

RangeMultiplier(1) hangs the binary. The guards that reject it -- the multiplier > 1 check in RangeMultiplier() and mult >= 2 in AddRange() -- are BM_CHECKs, which compile away under NDEBUG, so in a release build the multiplier is accepted and AddPowers() then multiplies by one forever looking for a value that reaches hi. Debug builds abort with a clear message; release builds spin with no output at all.

Add BM_CHECK_RUNTIME (the name suggested in #1754), which is the existing BM_CHECK without the NDEBUG opt-out, and define BM_CHECK to it in debug builds so there is only one implementation. Use it for the two range multiplier preconditions.

These run once per BENCHMARK() registration, never inside the measured loop, so the concern about paying for checks in the critical path does not apply here. Nothing changes for debug builds, and release builds now abort with

RangeMultiplier: Check `multiplier > 1' failed.

instead of hanging.

Covered by two new cases in benchmark_gtest that swap in an abort handler and assert both preconditions fire; the RangeMultiplier() one exercises the library as built, i.e. with NDEBUG.

RangeMultiplier(1) hangs the binary. The guards that reject it -- the
`multiplier > 1` check in RangeMultiplier() and `mult >= 2` in AddRange() --
are BM_CHECKs, which compile away under NDEBUG, so in a release build the
multiplier is accepted and AddPowers() then multiplies by one forever looking
for a value that reaches `hi`. Debug builds abort with a clear message;
release builds spin with no output at all.

Add BM_CHECK_RUNTIME (the name suggested in google#1754), which is the existing
BM_CHECK without the NDEBUG opt-out, and define BM_CHECK to it in debug
builds so there is only one implementation. Use it for the two range
multiplier preconditions.

These run once per BENCHMARK() registration, never inside the measured loop,
so the concern about paying for checks in the critical path does not apply
here. Nothing changes for debug builds, and release builds now abort with

  RangeMultiplier: Check `multiplier > 1' failed.

instead of hanging.

Covered by two new cases in benchmark_gtest that swap in an abort handler and
assert both preconditions fire; the RangeMultiplier() one exercises the
library as built, i.e. with NDEBUG.
@LebedevRI

Copy link
Copy Markdown
Collaborator

This spam is starting to get rude.

@dmah42

dmah42 commented Aug 3, 2026

Copy link
Copy Markdown
Member

we're not going to accept changes like this without discussion and a demonstration of an understanding of the existing reasons for how the codebase is today. also failing to acknowledge AGENTS.md shows a contempt for the project that is unhelpful. Given the sheer number of PRs you're opening, @devtejasx , i'm starting to get to the point of finding a way to ban you.

Consider your approach carefully.

@dmah42 dmah42 closed this Aug 3, 2026
@devtejasx

Copy link
Copy Markdown
Contributor Author

bro I am solving multiple issue and then pull them together . have you not seem time been last PR and these time PR .Its more than week.If you want, I can solve 1 issue in 2 day- if these approach is fine let me know

@dmah42

dmah42 commented Aug 3, 2026

Copy link
Copy Markdown
Member

my issue is not the time. my issue is the lack of discussion or understanding about why things might be the way things are before throwing a PR at us.

just because we can write code doesn't mean we should write code. AI has enabled code changes to not be a bottleneck, and that's great, but writing code was never the most important thing. Understanding and discussing continues to be.

@devtejasx

Copy link
Copy Markdown
Contributor Author

ok bro, got it from next time we will discuss and then i will open PR . Is these fine for you

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.

3 participants