NOTREACHED fatal migration status + NOTREACHED_IN_MIGRATION rename

[Peter Boström]

Hi folks,

tl;dr: Existing NOTREACHED() rolling out as fatal in 1% of stable. Will be renamed NOTREACHED_IN_MIGRATION() unless anyone complains and NOTREACHED() will be the [[noreturn]] always-fatal version (like CHECKs) unless a base::NotFatalUntil parameter is provided.

Way back I sent out a RFC on making NOTREACHED() fatal under the kNotReachedIsFatal experiment. This experiment is now (for Chrome) rolled out to 50% Canary, Dev and Beta and rolling out to 1% stable. We're not currently seeing any stability spikes. NOTREACHEDs that were hit earlier in the rollout process have been changed to DUMP_WILL_BE_NOTREACHED_NORETURN() which really just excludes them from the experiment (de-risks rollout).

I'd like to replace the current invocation of NOTREACHED() with "NOTREACHED_IN_MIGRATION()" or other bikeshed color if you prefer it and make NOTREACHED() immediately fatal and [[noreturn]] directly. That lets us start the cleanup work to make NOTREACHED() fatal and [[noreturn]] while the NOTREACHED_IN_MIGRATION() invocations finish rolling out as part of this experiment.

Any thoughts/concerns with this? Otherwise I'll probably make the changes this week. The previous plan was to make all NOTREACHED() fatal, transition them slowly over to NOTREACHED_NORETURN() and then merge the name back. I think it's better to make "NOTREACHED()" the desired end state as soon as possible so new code can use NOTREACHED() without learning about _NORETURN() and all our other variations.

Also a reminder that we've added the base::NotFatalUntil::M128 (or so) parameter to de-risk rollout stability when you're not certain it's not currently holding true. In this case post rename NOTREACHED() will be [[noreturn]], but NOTREACHED(base::NotFatalUntil::M128); won't be, regardless of the current milestone fatality. As the base::NotFatalUntil is cleaned up you'd also change the surrounding code to get rid of dead-code warnings. This commonly means turning an if(foo) { NOTREACHED(base::NotFatalUntil); return nullptr; } into CHECK(!foo); but at least means removing return nullptr; in this case.

Thanks!

Peter

[danakj]

Hi Peter,

Thanks for driving this huge effort and your consistent efforts to make sure it's derisked and not hurting stability.

Your plan sounds good to me on getting NOTREACHED() to be the always-fatal version, now that we're starting to turn it on in stable.

Cheers,

Dana

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAGFX3sHBku3Rww0JpiKK23yP41aa5QiwDwUm622UOgnKTz4jvw%40mail.gmail.com.

[Peter Boström]

As of crrev.com/c/5548908 landing about an hour ago, NOTREACHED() is now always-fatal and [[noreturn]]. NOTREACHED() is now CHECK-fatal and [[noreturn]], NOTREACHED(base::NotFatalUntil::M128) is non-fatal before M128, fatal on/after M128 and never [[noreturn]].

Unless reverted (not unlikely) within like a day or two I'll make follow-up cleanups including removal of all NOTREACHED_NORETURN()s and consolidate documentation.

Thanks,

Peter

[Peter Boström]

.. and reverted. I'll keep converting more repositories that're discovered.

For now I'd recommend using either NOTREACHED(base::NotFatalUntil::) or NOTREACHED_NORETURN() until this sticks and the latter is removed.

[Peter Boström]

This has now been relanded for a while. It looks like it'll stick so you can probably use NOTREACHED() with or without NotFatalUntil going forward.