if-guards after DCHECKs not allowed by Chromium style

[Caleb Raitto]

The Chromium style has this rule:

[...] you should not handle DCHECK() failures, even

if failure would result in a crash. Attempting to handle a `DCHECK()`

failure is a statement that the `DCHECK()` can fail, which contradicts the

point of writing the `DCHECK()`. In particular, do not write code like the

following:

```c++

DCHECK(foo);

if (!foo) // Eliminate this code.

...

if (!bar) { // Replace this whole conditional with "DCHECK(bar);".

NOTREACHED();

return;

}

```

...the guide also mentions using CHECK when a security vulnerability is possible, but we rarely use CHECKs in Chromium code, especially in the critical browser process. But, uses of DCHECK that I've seen could result in undefined behavior if the DCHECK was false.

Why prohibit defense-in-depth when the DCHECK is false (I guess to avoid code bloat and untested paths)? It seems to me that for security and stability, this pattern should at least not be prohibited?

Also, about the no-delete-null-pointer-checks mentioned below -- does this only apply for memory access to 0x0? Many times, I'll see something like 0x4 or 0x8, etc. when debugging null pointer crashes, since some struct / class member offset is being added to the null address.

-Caleb

[Caleb Raitto]

Right, but browser process crashes are really disruptive for users -- they lose the entire session, not just one tab. Often, the broken assumption (that is, a real bug, not an external condition like a network error) doesn't prevent the browser process from continuing to run -- the current operation can fail gracefully.

I think if someone is investigating a browser process crash bug, they should be able to stop the stability issue (with an if guard), but maybe be able to use something like NOTREACHED() to get DCHECK crash data from canary to investigate further. With the current rules, as I understand them, this is not allowed, even for temporary debugging.

-Caleb

On Tue, Oct 25, 2022 at 5:49 PM Peter Boström <pb...@chromium.org> wrote:

Sorry I should've phrased that better. I think CHECK(foo); is strictly better than DCHECK(foo); if (!foo) handle_nonfoo(); when you think foo should always hold true.

We're avoiding CHECKs in production code for code-size reasons and "if (!foo) return; " is probably comparably expensive to CHECK(foo); so just use the latter when you can.

On Tue, Oct 25, 2022 at 2:45 PM Peter Boström <pb...@chromium.org> wrote:

Sidestepping the original question, I think if you care about !foo for security reasons please CHECK(foo)? You're not unlikely to generate more code size by handling !foo after the DCHECK than a CHECK would?

This sidesteps the maybe obvious question of "what if I want to CHECK it but I don't know what the stability implications of that are?"

--
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/8919c16d-0e52-4350-a97f-31053a18ce07n%40chromium.org.

[Charlie Reis]

 Often, the broken assumption (that is, a real bug, not an external condition like a network error) doesn't prevent the browser process from continuing to run -- the current operation can fail gracefully.

This isn't the sort of thing that a DCHECK should be used for, though.  A DCHECK should be used for an invariant that won't fail, so that others can reason about that when designing their code.  If we intend to handle the situation gracefully, that should just be a handled case and not a DCHECK.

Charlie

[Alan Cutter]

I would make exceptions in start up code to avoid bugs crash looping Chromebooks.

[Matt Denton]

I'd say using NOTREACHED() is a violation of this style guideline, as it's pretty much DCHECK(false) and so merely continuing to run is "handling the impossible condition."

As others have mentioned, there are plenty of cases where we would like to crash in testing, but handle the condition in production for stability purposes. I recently landed a CL that uses DLOG(FATAL), then calls DumpWithoutCrashing() and continues to run in non-dcheck builds. I want to catch any uses of a codepath that shouldn't be used going forward, but in the meantime use of this codepath shouldn't crash the process unnecessarily.

Perhaps the guideline should be updated?

This applies for nullptr + offset as well. The -fno-delete-null-pointer-checks flag is a legacy name and now means something broader: that clang will allow data to reside at nullptr. Given that, it would be very illogical if clang decided data could reside at 0x0 but not at 0x4. :) For more information see this thread or this bug.

 

-Caleb

--
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/8919c16d-0e52-4350-a97f-31053a18ce07n%40chromium.org.

--
To unsubscribe from this group and stop receiving emails from it, send an email to security-dev...@chromium.org.

[Russ Hamilton]

> I think if you care about !foo for security reasons please CHECK(foo)?

That's not what the style guide says or even implies. I've had code reviewers that wouldn't allow CHECKs in my CLs, even if a DCHECK would leave the code calling an uninitialized function pointer. If I can't point to a plausible exploit reviewers push back. 

If the intent of the style guide is not to "almost never use CHECK", then perhaps it should be reworded to express that more clearly.

On Tue, Oct 25, 2022, 5:49 PM Peter Boström <pb...@chromium.org> wrote:

Sorry I should've phrased that better. I think CHECK(foo); is strictly better than DCHECK(foo); if (!foo) handle_nonfoo(); when you think foo should always hold true.

We're avoiding CHECKs in production code for code-size reasons and "if (!foo) return; " is probably comparably expensive to CHECK(foo); so just use the latter when you can.

On Tue, Oct 25, 2022 at 2:45 PM Peter Boström <pb...@chromium.org> wrote:

Sidestepping the original question, I think if you care about !foo for security reasons please CHECK(foo)? You're not unlikely to generate more code size by handling !foo after the DCHECK than a CHECK would?

This sidesteps the maybe obvious question of "what if I want to CHECK it but I don't know what the stability implications of that are?"

On Tue, Oct 25, 2022 at 2:37 PM Caleb Raitto <cara...@chromium.org> wrote:

--

[Peter Boström]

Sidestepping the original question, I think if you care about !foo for security reasons please CHECK(foo)? You're not unlikely to generate more code size by handling !foo after the DCHECK than a CHECK would?

This sidesteps the maybe obvious question of "what if I want to CHECK it but I don't know what the stability implications of that are?"

On Tue, Oct 25, 2022 at 2:37 PM Caleb Raitto <cara...@chromium.org> wrote:

--

[Peter Boström]

Sorry I should've phrased that better. I think CHECK(foo); is strictly better than DCHECK(foo); if (!foo) handle_nonfoo(); when you think foo should always hold true.

We're avoiding CHECKs in production code for code-size reasons and "if (!foo) return; " is probably comparably expensive to CHECK(foo); so just use the latter when you can.

On Tue, Oct 25, 2022 at 2:45 PM Peter Boström <pb...@chromium.org> wrote:

[K. Moon]

The relevant style guide section we're talking about:

https://chromium.googlesource.com/chromium/src/+/refs/heads/main/styleguide/c++/c++.md#check_dcheck_and-notreached

The relevant text on DCHECK:

A consequence of this is that you should not handle DCHECK() failures, even if failure would result in a crash.

The point of this rule is that if you're DCHECK'ing and handling the "impossible" condition, you're being redundant/contradictory. If it's not impossible, get rid of the DCHECK. (This is what I'd recommend most of the time.) If it is impossible, get rid of the handling.

The relevant text on CHECK:

Use CHECK() if the consequence of a failed assertion would be a security vulnerability, where crashing the browser is preferable. Because this takes down the whole browser, sometimes there are better options than CHECK().

If it's impossible, but bad things would happen if you're wrong, then that needs to be a CHECK, full stop. The guide goes on to discuss alternatives to crashing the browser process, but the primary rule is that we need to bail out when security would be compromised.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAG-DU2TfjStu%3DhwmN%2BQsrkSH_etupi10qR3NgUE_%3DBUGq%3Ds9g%40mail.gmail.com.

[Russ Hamilton]

The style guide doesn't say bad things. It says security vulnerabilities. So if my reviewer says that there's no way that an attacker could control what would happen after the failed DCHECK then it can't be a security vulnerability.

If you want to change it to bad things or undefined behavior or something like that then that's completely fine by me and I would support it. CHECKing on undefined behavior is what I would like to do but that's not what I can do with the style guide as it is.

[Peter Boström]

I feel like the style guide does not account for the fact that you may want to know whether you're handling the impossible or not (i.e. could this turn into a CHECK without crashing the world). Silent failures because you're handling !foo when you shouldn't have to is not great for code health either.

It would be nice if you could violate the style guide with a temporary exception for N milestones and after that you need to decide whether to handle it or turn it into a CHECK.

[dan...@chromium.org]

On Tue, Oct 25, 2022 at 6:31 PM 'Russ Hamilton' via cxx <c...@chromium.org> wrote:

The style guide doesn't say bad things. It says security vulnerabilities. So if my reviewer says that there's no way that an attacker could control what would happen after the failed DCHECK then it can't be a security vulnerability.

Renderer-controlled memory bugs in the browser process would be a Pri-0 Critical-Severity security bug, though might be downgraded to Pri-1 High-Severity if it's difficult for the Renderer to accomplish it.

https://chromium.googlesource.com/chromium/src/+/HEAD/docs/security/severity-guidelines.md#TOC-Critical-severity

Undefined behaviour is abused to create memory bugs, so undefined behaviour is a security vulnerability. The style guide is not asking chrome engineers to be offensive exploit writers to prove a given memory bug can be used.

 

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

[K. Moon]

Big +1 to danakj's point about, "The style guide is not asking chrome engineers to be offensive exploit writers." I think it'd take a lot of hubris to claim that something can't be exploited. (If you really can come up with an iron-clad proof... then yes, that doesn't need a CHECK. In most cases, it's probably not worth the effort, though.)

Stepping back a bit, I think a big part of the motivation for the style rules is that you always want to be careful with assertions that are conditional on build configuration, like DCHECK. Other than the performance hit, CHECK is less problematic than DCHECK.

Hyrum's Law also probably applies: If you can rely on the behavior in production, you might as well make that the behavior in testing, too, because some part of the system is going to start relying on it. Test coverage isn't perfect, and conditional behavior is the enemy of simplicity.

On Wed, Oct 26, 2022 at 10:06 AM Alex Cooper <alco...@chromium.org> wrote:

There have definitely been times where I've kind of wanted to have a way to say "This really *shouldn't* be null, and if a dev is changing it I'd like them to know that it shouldn't be null; but nothing stops them from passing null so I should handle it to avoid dereferencing a nullptr or similar"; which is kind of the situation where I'd be in the state you describe as an assertion holding true in testing but not in real-world use.

The guidance also says "don't use CHECK/DCHECK in cases where it should always be true except in exceptional circumstances" (paraphrasing). So there's cases like that where I maybe *do* actually want to crash if it's a developer build (or trigger a breakpoint or similar); but could probably gracefully handle on a production build. But maybe that's a prime usecase for "DumpWithoutCrashing", which I don't really see in broad use and perhaps we should encourage for some of those situations.

On Wed, Oct 26, 2022 at 9:31 AM Peter Boström <pb...@chromium.org> wrote:

Imo NOTREACHED() should've been effectively CHECK_NOTREACHED() and always be fatal. I don't think it's different from any other DCHECKs though (continues running even after a condition is false).

We don't want to be in a state where an assertion holds true in testing and not in real-world use. For most code paths I hope you'd find out if a CHECK is crashy enough to be a stability problem long before it reaches stable, but I do see value in something intermediate.

Also note that any DumpWithoutCrashing that hits a lot in stable will flood the crash server so if it would've been a stability problem (on a population level) you should still fix that before it reaches stable.

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

--
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/CAGOLbz3asNB%3DWERY9yb_g-yGgZ0SfQ65DYLiM-PKmfb-Q30J9g%40mail.gmail.com.

[Peter Kasting]

I'm the primary author of this style rule. I've been hoping this thread would converge, but perhaps it helps to weigh in with a few things.

In an ideal world, we would not have both DCHECK and CHECK -- we would have a single macro, which asserts to the reader that it is the author's opinion that some condition is an invariant precondition at that point (barring cosmic rays flipping bits between statements). The condition in question can be relied upon utterly, and the compiler can optimize away any code which would depend on it being violated. The compiler need not check that the condition holds, because we _know_ the condition holds. More important, the reader knows that the author wrote the code under the belief that that condition holds. And, assuming the author is correct, the reader can then simplify all callers and callees, transitively, by assuming that condition holds.

In this world, the assertion in question is critical documentation to the reader as to the author's state of mind when writing the code, and it greatly helps in later refactoring and cleanup, because it tells us what sorts of refactors should maintain validity of the code in question.

We would never let authors write code of the form:

if (i < 0) return;

if (i < 0) ...

...because the second conditional is nonsensical. At best, it's clutter. At worst, later refactoring could obscure the fact that it is nonsensical, resulting in people misunderstanding the preconditions of code they're reading, and beginning themselves to write complicated defense in their own code for conditions which can never occur.  And in our ideal world, authors would treat this the same way:

INFALLIBLY_ASSERT(i >= 0);

if (i < 0) ...

Because the assertion is trustworthy, this has all the downsides mentioned above, and no upsides.

As soon as the assertion is not trustworthy, the benefits above fall apart. It is not safe for the compiler to optimize based on the assertion, because that could introduce cryptic bugs. It's not safe to refactor based on the assertion, because that could spread bugs across the codebase. The only remaining value of the assertion is if the code _does_ check whether it holds, and reports it back to us in some way, so that we can gather stats and make changes based on that.

When we wrote DCHECK, we intended it to have the first ("trustworthy") meaning, not the second. It meant that the author was certain the condition held and that we could safely optimize away checking that condition, or any consequent effects. The style guidance we have arose from this conception. We did still compile in such checks in developer mode, primarily so that while developers were in the midst of writing them they'd get notified about anything obviously silly they did, but we wanted them to be used as documentation of pre- and post-conditions, not as some sort of early-warning for surprising cases. If you weren't sure whether a DCHECK could fire, you needed to audit the code further until you were certain it could or certain it couldn't. Then either handle it without DCHECKing, or DCHECK without handling it. No middle ground.

CHECK was actually intended to be the defense-in-depth case. Much like trying to "handle" allocation failures due to OOM, "handling" the violation of believed-infallible preconditions just punts the problem to your callers and callees. It is better to crash immediately in such cases. CHECK was the concession we made to the reality that developers are not infallible. We reserved it for the case of security bugs primarily to avoid muddying the waters on whether or not DCHECKs were "trustworthy enough" (we didn't want people to believe CHECKs were "more trustworthy").

It has become clear over time that our existing DCHECKs are not as infallible as we'd like them to be, to the point where increasingly authors are suspicious of whether they can truly rely on them. We believe that we need wider testing, fixing, and enforcement of them, so that we can be sure they're holding in production. pbos@ has been leading this effort. Over time, you will likely see changes to both what assertion-like macros exist, as well as our style guidance on when to use them, so we can have more consistent testing of our assertions without adding too much binary size or runtime cost (as we believe will happen if we blindly convert ALL DCHECKs to CHECKs today).

In the meantime, I suggest you avoid "DCHECK + handler code". Be clear about the API design of your function: what are its preconditions and postconditions? Audit the callers and callees, and enforce the conditions you expect. Document them, both in comments and with DCHECKs, along with why it's important that your code have them. And handle (without DCHECKing) cases outside those.

PK

[Alex Cooper]

There have definitely been times where I've kind of wanted to have a way to say "This really *shouldn't* be null, and if a dev is changing it I'd like them to know that it shouldn't be null; but nothing stops them from passing null so I should handle it to avoid dereferencing a nullptr or similar"; which is kind of the situation where I'd be in the state you describe as an assertion holding true in testing but not in real-world use.

The guidance also says "don't use CHECK/DCHECK in cases where it should always be true except in exceptional circumstances" (paraphrasing). So there's cases like that where I maybe *do* actually want to crash if it's a developer build (or trigger a breakpoint or similar); but could probably gracefully handle on a production build. But maybe that's a prime usecase for "DumpWithoutCrashing", which I don't really see in broad use and perhaps we should encourage for some of those situations.

On Wed, Oct 26, 2022 at 9:31 AM Peter Boström <pb...@chromium.org> wrote:

Imo NOTREACHED() should've been effectively CHECK_NOTREACHED() and always be fatal. I don't think it's different from any other DCHECKs though (continues running even after a condition is false).

We don't want to be in a state where an assertion holds true in testing and not in real-world use. For most code paths I hope you'd find out if a CHECK is crashy enough to be a stability problem long before it reaches stable, but I do see value in something intermediate.

Also note that any DumpWithoutCrashing that hits a lot in stable will flood the crash server so if it would've been a stability problem (on a population level) you should still fix that before it reaches stable.

On Wed, Oct 26, 2022 at 1:44 AM Matt Denton <mpde...@google.com> wrote:

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

[Peter Boström]

Imo NOTREACHED() should've been effectively CHECK_NOTREACHED() and always be fatal. I don't think it's different from any other DCHECKs though (continues running even after a condition is false).

We don't want to be in a state where an assertion holds true in testing and not in real-world use. For most code paths I hope you'd find out if a CHECK is crashy enough to be a stability problem long before it reaches stable, but I do see value in something intermediate.

Also note that any DumpWithoutCrashing that hits a lot in stable will flood the crash server so if it would've been a stability problem (on a population level) you should still fix that before it reaches stable.

On Wed, Oct 26, 2022 at 1:44 AM Matt Denton <mpde...@google.com> wrote:

[Elly Fong-Jones]

Large +1 to pkasting's perspective about this. DCHECKs should be used to document preconditions that are always true and are required to always be true for correct operation of the function body. If your function body works fine when some input is null (or whatever other odd value) then "this input isn't null" is not a precondition and you don't want to DCHECK that.

The philosophical question of whether you should handle odd function inputs by:

a. Crashing immediately and hard, or

b. Returning some out-of-range return value (null, -1, nullopt, whatever), or

c. Returning some in-range return value (a new object of the required type)

is a question about how you should define the allowed inputs of your function, and not necessarily about DCHECKs. As examples of all three types:

/* function defined on non-null p */

int strlen(const char *p) {

  CHECK(p);

  /* blithely use p */

}

/* function defined on any p, null or non-null */

int strlen(const char *p) {

  if (!p) return -1;

  /* blithely use p */

}

/* function defined on any p, null or non-null */

int strlen(const char *p) {

  if (!p) return 0;

  /* blithely use p */

}

In the latter two cases no CHECK is necessary or desirable because the function *does* have a defined correct behavior when passed null, so no precondition is being violated.

My personal perspective on the philosophical question is that (a) is always better because (as pkasting mentioned) the other two approaches tend to move bugs around the codebase, further from where they are actually introduced, and in particular tends to lead to crash stacks that don't show where the original problem was - a function handles an odd value by turning it into another odd value, that happens several times, and *eventually* the odd value is detected later on and causes a crash, but tracking it back to the original problem site is very difficult. Unless you are certain that your entire codebase will handle such odd values safely at all points, you are better off crashing right away so that the crash stack at least reveals the problem site.

-- elly

--

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/CAAHOzFBboZ_QoTJ5HkHED9NUEL-2CWy7STiRFzPcHQiErR%2BQ7A%40mail.gmail.com.

[Peter Kasting]

On Mon, Oct 31, 2022 at 11:09 AM Russ Hamilton <beham...@google.com> wrote:

Would it make sense to enable DCHECKs on all platforms for Canary at least? I think that would catch a lot of the issues that lead me not to trust DCHECKs as much. As it is now, DCHECK is really just a comment that you can't easily write a test case for.

As I mentioned, pbos@ is working on greater testing and enforcement of DCHECKs in the field.

Note that it is possible to write tests for DCHECKs using macros like EXPECT_DCHECK_DEATH.

PK

[K. Moon]

I think it's reasonable to lean towards CHECKs when you're unsure, but I see the purpose of DCHECKs to be a little bit stronger than a comment saying, "You shouldn't pass a null pointer into this function." It's mainly there to tell you when you're holding the API wrong, but you can't count on it to enforce anything.

On Mon, Oct 31, 2022 at 9:57 AM Alexei Svitkine <asvi...@chromium.org> wrote:

Is DCHECK actually working to ensure those preconditions stay true?

For example, if that's the case when initially implemented, but we don't have sufficient test coverage on all platforms to ensure that some later change doesn't regress that, then there is a reasonable question of "what do we want to happen in those cases - i.e. the handled code path that we can reason about or something that's not easy to reason about"? IMHO, that seems to be the fundamental problem. Maybe we should lean more towards CHECKs instead if we can't get said confidence in DCHECKs.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CADpj5oxVLvQ2rPqyoPixjCgoyh3Jn86gxW%2BJ9g3pCo1%3DeEKSjg%40mail.gmail.com.

--
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/CAKFU2SADSS1bTT9MJ3uettAoHXrbWgKjvwXLbrHMsiZssaDoDQ%40mail.gmail.com.

[Alexei Svitkine]

Is DCHECK actually working to ensure those preconditions stay true?

For example, if that's the case when initially implemented, but we don't have sufficient test coverage on all platforms to ensure that some later change doesn't regress that, then there is a reasonable question of "what do we want to happen in those cases - i.e. the handled code path that we can reason about or something that's not easy to reason about"? IMHO, that seems to be the fundamental problem. Maybe we should lean more towards CHECKs instead if we can't get said confidence in DCHECKs.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CADpj5oxVLvQ2rPqyoPixjCgoyh3Jn86gxW%2BJ9g3pCo1%3DeEKSjg%40mail.gmail.com.

[Russ Hamilton]

Would it make sense to enable DCHECKs on all platforms for Canary at least? I think that would catch a lot of the issues that lead me not to trust DCHECKs as much. As it is now, DCHECK is really just a comment that you can't easily write a test case for.

[K. Moon]

While it's possible to write death tests, I've been discouraged from writing them; certainly not to the extent that every DCHECK could have a death test.

[Elly Fong-Jones]

I think you should be pretty sparing with death tests. They require forking off a process (which dies), which is a much slower and more heavyweight option than unit tests usually are and can be kind of annoying to work with in a debugger. In general I would only bother if the precondition itself is complicated to evaluate... in which case the precondition should be a separate function which returns a bool and you should test that function separately from the death part.

-- elly

[Gabriel Charette]

+1 to pkasting's description.

I think we should be testing all non-trivial DCHECKs (i.e. don't just pass 0 to a function which performs DCHECK(x > 0) as part of its basic contract).

I request this for all core APIs where DCHECKs are part of the API (i.e. the API will tell you if you're holding it wrong, that's a feature, and features should be tested). I'm a big fan of those DCHECKs-with-a-description-of-what-else-to-do, but the invariants must be bomb-proof (and thus DCHECK_DEATH tested).

--

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/8296a421-8f01-4450-a2d9-6f92b16b8c71n%40chromium.org.

[K. Moon]

We wouldn't need death tests if we had exceptions. :P

[Peter Boström]

I'm going to write up something more comprehensive about the state of our invariants and how we can end up in a better world where we can trust both our CHECKs and DCHECKs (and NOTREACHEDs, and...).

But for the purposes of this thread:

* I think continuing past a DCHECK(false); or DCHECK(x > 0); with x <= 0 should be treated as UB for two reasons: I think we should let the compiler optimize based on that as it can generate more efficient code (using __builtin_assume(cond)), and human readers should also be able to modify the code based on this invariant being true. Even if we have defined behavior from a compiler, humans should not assume that the DCHECK can ever be incorrect. This is in line with the current documentation.

* Do not use "how bad if !cond" to determine whether a CHECK/DCHECK is appropriate. A failing DCHECK is UB, so you cannot reason about how bad it would be (and because humans can rely on this invariant they can make the fallout worse than your initial commit). Other criteria that're more reasonable are:

- Can we afford to CHECK it in performance-sensitive stable builds? The current DCHECK build has significant overhead vs. regular official. We believe things like sequence checking, which is really important, is currently really expensive. But if you have DCHECK(x > 0), you can probably afford a CHECK(x > 0); unless you're in a critical loop or already do expensive work in this function (other work dominates evaluating cond).

- How likely is this DCHECK to fail without us noticing? We've added DCHECK builders and cover DCHECKs for a percentage of Canary to help discovery in real-world scenarios, but this is work in progress and we don't cover as much as we'd like to (channels, platforms, populations). We also need more aggressive monitoring (you should know if your (D)CHECK ever fails, not just if it reaches a certain CPM), but we probably need to get our house in order before that.

I don't have strong feelings about death-test coverage for these things, I feel strongly about increasing our monitoring in the wild because real-world scenarios are more interesting. If I should care more about death tests here please holler. :)

Hope that makes some sense,

Peter

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-5-fYTF7Rj_1rS30XNecuUEy2QN%2BcOWnYT9AAJfZm0pJg%40mail.gmail.com.