Stengthening our language around CHECKs, DCHECKs and NOTREACHEDs

[Peter Boström]

Hi folks!

We've been working on detecting and reporting DCHECK and NOTREACHED failures in the wild as well as strengthening NOTREACHEDs to detect bugs in production. To support this work we need stronger language in the style guide.

None of these macros are appropriate for things that occasionally fail within our control, and it's preferable to CHECK over DCHECK where there are no performance concerns. CHECK failures are no longer expensive if the conditional is inexpensive to evaluate (base::ImmediateCrash() is two instructions).

Proposed changes include:

"All invariant failures should be seen as P1 bugs, regardless of their crash rate."

// Good:
//
// Testing pointer equality is very cheap, write this as a CHECK. Like many
// CHECKs, a security bug happens afterward if the CHECK would fail, in this
// case immediately afterward.
auto it = container.find(key);
CHECK(it != container.end());
*it = Foo();

We also mention alternatives to these macros as base::ImmediateCrash() for intentionally crashing (but not invariant failure) and base::debug::DumpWithoutCrashing(), LOG(DFATAL) or DLOG(FATAL) for production-non-fatal alternatives.

The style-guide change is up for review crrev.com/c/4287020.

Please let me know what you think. :)
Peter

[dan...@chromium.org]

Thank you for this important work toward making our product's behaviour match what the code says, and making bugs both faster and easier to locate, LGTM

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGFX3sHnHQSK2EFGsk6Z6kaLu41WEgZBAp%2BpsGNPgOuOYvvOdQ%40mail.gmail.com.

[Peter Kasting]

I left sentiments on the CL but I wanted to note for posterity here that directionally this change has my strong support and IMO moves us in the right direction. I look forward to landing this and then beginning to argue with authors about how they should be CHECKing more...

PK

[Elly Fong-Jones]

I strongly approve this message.

-- elly

On Thu, Mar 2, 2023 at 2:01 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

I left sentiments on the CL but I wanted to note for posterity here that directionally this change has my strong support and IMO moves us in the right direction. I look forward to landing this and then beginning to argue with authors about how they should be CHECKing more...

PK

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFDQjb6LqjYSA%2BYd9UQAv6C4WEG7PWs0GYO%2ByBp9g8jcRw%40mail.gmail.com.

[Peter Boström]

Thanks everyone. Changes to https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++.md are live and we have https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/checks.md as well.

[Peter Kasting]

Yay! I think this should be announced to chromium-dev as it is a pretty significant policy update.

PK

[dan...@chromium.org]

+1

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFAeUg%3DTapfEWbW4dG%3DTw94zKjy7M0_3iPhzKNU74f_oLw%40mail.gmail.com.

[Xiaohan Wang (王消寒)]

Thanks for the change, I like it a lot as well, especially NOTREACHED_NORETURN()!

I am asking people to use CHECKs in code reviews for new code.

One question: how about some implicit DCHECKs, e.g. DCHECK_CALLED_ON_VALID_THREAD. Should we migrate them gradually?

Xiaohan

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaQ%3D6zfoqRxn%2Bq%3DWC0fuF_9sttNgMnwNLO%3DUK%3DvXgdukTw%40mail.gmail.com.

[Peter Boström]

I haven't gotten to the announcement yet as my family's been out sick. Can you file a bug for the DCHECK_CALLED_ON stuff? I believe thread / sequence checking isn't cheap and this may not be one we can blanket approve.

Maybe it'd make sense to introduce a CHECK_CALLED_ON_VALID_THREAD for places where you *know* you're not in performance sensitive code. Blanket changing DCHECK_CALLED_ON_VALID_THREAD may hurt performance more than we'd like.

[Michael Spang]

The new guidance seems to advocate for a blanket change. If it's not
cheap enough for sequence checks, changing the default choice seems
suspect (there may be a lot of sequence checks, but there will also
be a lot of new CHECKs added over time as a result of this change).

> To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGFX3sHvrG6BuMbC%2B7arNx7SF2f7XHamoE2ZYLg-XoJNJJWOyw%40mail.gmail.com.

[Peter Boström]

The blanket change has an exception for things that are expensive. Sequence checking is very frequent and, unless I'm wrong, expensive on some platforms requiring taking multiple locks(?).

[Michael Spang]

I don't think it will be intuitive to developers that sequence checking
is expensive.

The upside of DCHECK as a default is that you could state any
invariant without worrying about how expensive it would be in
production, because it was free in production.




Michael

[Sunny Sachanandani]

Sequence/thread checking is exposed to developers through the DCHECK_CALLED_ON_* macros so assuming those macros only use DCHECK and not CHECK, there shouldn't be an issue with developers accidentally introducing these in production builds, right?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAC5F_1mWedeFznkMOBQ-JoqjraLP%3Dcri4cqKZAqPELfZbe0ibg%40mail.gmail.com.

[Peter Boström]

That would only be a concern imo if we exposed a CHECK_ version of them that didn't look expensive but was. Expensive looking cheap is an API design flaw imo.