PSA: DCHECK by default for developers

[Peter Boström]

Hi chromium-dev@!

We’ll be turning on DCHECKs by default for developer release builds starting next week. We’re doing so to get better coverage of existing DCHECKs in paths not covered by automated testing. This will be done by changing the defaults for dcheck_always_on for non-official builds.

As the DCHECKs-by-default change is only intended for developers and because non-DCHECK paths are important to cover, we’ll change the existing buildbots that do not run with DCHECKs enabled today to use `dcheck_always_on = false`.

If you’re running infrastructure that currently builds without DCHECKs, you may want to consider updating your build configs to retain coverage of both DCHECK and non-DCHECK paths.

If you’re shipping binaries to users it’s still recommended that you build with `is_official_build = true`, otherwise consider using `dcheck_always_on = false` to retain existing behavior.

As we’re increasing exposure to DCHECKs to developers, a failing DCHECK is now a high-priority bug (P1/P0). It is also more likely that your changes get reverted if they introduce a failing DCHECK.

To try this early, build with `dcheck_always_on = false` and `is_debug = false`. If you hit any failing DCHECKs then file bugs. If they are bad enough that you think this should block rollout to fellow colleagues, indicate why and make this bug block crbug.com/1225701.

Thank you for your help!

Peter on behalf of DCHECKs.

[Dirk Pranke]

On Fri, Jul 16, 2021 at 2:36 PM Peter Boström <pb...@chromium.org> wrote:

Hi chromium-dev@!

We’ll be turning on DCHECKs by default for developer release builds starting next week. We’re doing so to get better coverage of existing DCHECKs in paths not covered by automated testing. This will be done by changing the defaults for dcheck_always_on for non-official builds.

As the DCHECKs-by-default change is only intended for developers and because non-DCHECK paths are important to cover, we’ll change the existing buildbots that do not run with DCHECKs enabled today to use `dcheck_always_on = false`.

If you’re running infrastructure that currently builds without DCHECKs, you may want to consider updating your build configs to retain coverage of both DCHECK and non-DCHECK paths.

I will be handling this for all of our existing builds on the chromium waterfalls, so please don't duplicate any of the work I'll be doing. Feel free to ask me if you're not sure.

-- Dirk

 

If you’re shipping binaries to users it’s still recommended that you build with `is_official_build = true`, otherwise consider using `dcheck_always_on = false` to retain existing behavior.

As we’re increasing exposure to DCHECKs to developers, a failing DCHECK is now a high-priority bug (P1/P0). It is also more likely that your changes get reverted if they introduce a failing DCHECK.

To try this early, build with `dcheck_always_on = false` and `is_debug = false`. If you hit any failing DCHECKs then file bugs. If they are bad enough that you think this should block rollout to fellow colleagues, indicate why and make this bug block crbug.com/1225701.

Thank you for your help!

Peter on behalf of DCHECKs.

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

[Peter Boström]

Oops, these instructions are wrong for anyone copy-pasting. To try this early set dcheck_always_on=true and is_debug=false.

Thanks again,

Peter

[Peter Boström]

Hi folks,

crrev.com/c/2893204 landed 24 minutes ago (I've confirmed locally that DCHECK_IS_ON() when is_debug=false on Mac). Sheriffs have been informed as well, just in case.

We're still working on flipping ChromeOS bot defaults so is_chromeos and is_fuchsia are currently excluded from this change. I'll reach out to Fuchsia folks separately as well.

Please note that DCHECKs should definitely be treated as P1/P0 issues now, if you didn't treat them as such already.

Thanks,

Peter

[Alexei Svitkine]

I'm not sure if it's a consequence of these recent changes, or unrelated, but one thing I noticed recently is that we don't have a non-DCHECK-compiled-in configuration on the default try bots.

This means that it's too easy to introduce a subtle bug like:

DCHECK(DoStuff());

Where in non-DCHECK builds, this gets compiled out and DoStuff() never runs. Even with unit tests verifying that DoStuff() was called, the try bots will all pass since we don't seem to have one that compiles out DCHECKs, like production builds do.

This seems like a bad situation to be in. Is there a plan to improve this?

--

--
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/CAGFX3sEL0ug%3DHFj7rMWpHw2%3DpdVZWBtyebxcTG9c%2BAL8E7zGMA%40mail.gmail.com.

[Dirk Pranke]

This change did not change how any of the bots were configured, so your observation is unrelated.

That said, I believe it is correct -- there are no bots in the default/CQ set that aren't either release+dcheck or debug -- and AFAIK this has not turned out to be a significant issue (it's been this way for a long time).

There are probably some optional bots that don't have dcheck enabled (I'd have to check to find out), and if there aren't, it might be a good idea to add some I agree.

Inside ops-land we're just starting a conversation about changing builder names to better reflect what they actually do (and be more understandable) and it might make sense to set up some optional release-no-dcheck bots as part of that as well.

-- Dirk

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKFU2SAeX3Bnb6ua9PK_5%2B%3DuxGWfnvoA0X_HSMZ%2Bvi4E%3D67SMA%40mail.gmail.com.

[Dirk Pranke]

I may have left out one bit that confused things slightly: we have plenty of post-submit/CI coverage of Release and Official builder configs that don't have DCHECKs enabled, so we just don't catch it pre-submit by default.

-- Dirk

[Garrett Beaty]

There is at least one optional trybot for running without dchecks: https://ci.chromium.org/p/chromium/builders/try/linux-dcheck-off-rel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAEoffTAvwjPG5%3DHUYqtufBFDim8O2JnSmio4DoNt7jQM9wH6gA%40mail.gmail.com.

[Dirk Pranke]

Good find :). Now that that has jogged my memory, I'm sure we set that up intentionally for this very reason.

-- Dirk

[Garrett Beaty]

Yeah, that was another one of my starter bugs.

[Alexei Svitkine]

Ah cool. I'm wondering if it's something we could include in the default set of trybots?

Right now, it's something that we expect reviewers to catch - which isn't ideal as it can be missed or added post-review (e.g. after a reviewer +1s but asks to add a DCHECK).

[Garrett Beaty]

It only makes sense to add it to the CQ by default if dchecks with side effects are landing often enough to make it worth running all of the additional testing for every CL. Nothing has indicated that's the case.

[Olga Sharonova]

It looks like we need one bot per platform with DCHECKs off in CQ?

Example:

I've got a CL which broke compilation with DCHECKs off on Win and Mac. It passed CQ and is now causing tree closures repeatedly when hitting waterfall bots with DCHECKs off.

It's pretty easy to forget to compile with DCHECKs off locally in addition to a normal workflow. 

I'd expect CQ to guard from such situations.

Olga

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAOZqsm1OfDyBY%3DMi6HrMm5tHUzzywfqMxTLmEuSdj-LMZaQ9Aw%40mail.gmail.com.

[Gabriel Charette]

+1 and while we're at it, let's make that bot use is_official_build=true (no src-internal, just the optimisation), because I've also hit an issue for a second time last week where I had an IS_OFFICIAL_BUILD only issue in my code and it passed CQ only to break the interval waterfall.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHri2_SnvQivSkqjhM7wm4F2qoCg632NE%2BHhXfJ8i70u%3D-X-QQ%40mail.gmail.com.

[Roland Bock]

+2 (or a +1 for each suggestion)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7L%2BeGN0f%2Bxq4p_Ae0dwhan_0dO5tQm%3DGNG1amaB3%3DjoQdw%40mail.gmail.com.

[K. Moon]

Adding bots to the CQ is more expensive than post-submit, since they run multiple times on every change, so I think it's reasonable to wonder about the cost-benefit ratio of adding more bots vs. having to close the tree and revert a change from time to time. (I claim no knowledge of what the quantitative trade-offs would be, but this probably isn't a slam dunk.)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHq1K7Q7Kb923-AFtnC0iYLrS6cH7Fre-6V4kZXg%2B3Q8c3DHyA%40mail.gmail.com.

[Dirk Pranke]

kmoon@ is correct: CQ bots are very expensive and as we said before, they need to catch a significant number of failures to be justified, and as a result we do expect some percentage of bad CLs to get through and for things to be broken on trunk as a result.

Unfortunately, we don't yet have a good way of tracking how often we do get different kinds of failures escaping the CQ, though. We're working on fixing this, but progress has been slow. 

In the meantime, at best we'd probably have to try and get some sort of anecdotal sense of how often these failures escape, and my sense is that neither class of failures happens often enough to justify adding new bots (particularly not 6-7 new bots, the requested "one per platform" number). The rough bar we talk about is seeing multiple failures per week get through, and I haven't heard that yet for these categories. As noted, this is anecdata at best, so I could easily be wrong and would love to hear about it if I am. And, I am particularly sensitive to the idea for these configs since it seems intrinsically like a good idea to test what we actually ship ...

That said, I'd find the idea that we're commonly hitting official-specific build issues to be surprising (non-dcheck failures would not be surprising). @Gabriel Charette are you saying that you hit failures in official twice in the span of a week? That's probably worth digging into, if so.

-- Dirk 

[Avi Drissman]

We do have official trybots, they just don't trigger on every CL. Whenever I touch IS_OFFICIAL_BUILD code, I always make sure to manually trigger them.

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

[Alexei Svitkine]

Would it make sense to have those bots be added automatically if the diff includes lines with IS_OFFICIAL_BUILD in them?

[Dirk Pranke]

That's an interesting idea, but I'm not sure if we have any easy way of implementing it. If someone wanted to look into that, I'd be open to it.

-- Dirk

[Gabriel Charette]

It can be more subtle than that. In my case it was that CHECK eats its stream under IS_OFFICIAL_BUILD so my death test expectation broke.

This happened to an important CL I submitted just before going on 3 weeks holidays. It got reverted by a sheriff because one of its death tests failed on official bots...  That CL was supposed to eliminate a class of failures on TSAN bots, instead of that: many more tests were disabled because the fix was reverted (and to this day I have no way to tell how many were disabled under my radar...).

Re. each week? No, but the impact goes beyond "minor annoyance" to ultimately evolve into a culture like "avoid death tests". Seems it's cheaper to add that bot than have a way to measure whether we need it or not... 

[dan...@chromium.org]

On Thu, Jan 27, 2022 at 1:23 PM Gabriel Charette <g...@chromium.org> wrote:

It can be more subtle than that. In my case it was that CHECK eats its stream under IS_OFFICIAL_BUILD so my death test expectation broke.

This happened to an important CL I submitted just before going on 3 weeks holidays. It got reverted by a sheriff because one of its death tests failed on official bots...  That CL was supposed to eliminate a class of failures on TSAN bots, instead of that: many more tests were disabled because the fix was reverted (and to this day I have no way to tell how many were disabled under my radar...).

Re. each week? No, but the impact goes beyond "minor annoyance" to ultimately evolve into a culture like "avoid death tests". Seems it's cheaper to add that bot than have a way to measure whether we need it or not... 

FWIW, this case is why we have EXPECT_CHECK_DEATH macros which should be used when looking for a CHECK() death.

This isn't super discoverable and there's no guardrails for doing it wrong really, unfortunately.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7LJfkW09BeOPFhv3zewjs7KA5dMsZGC-3vJmXEKGrixB%3DQ%40mail.gmail.com.

[Dirk Pranke]

On Thu, Jan 27, 2022 at 10:22 AM Gabriel Charette <g...@chromium.org> wrote:

It can be more subtle than that. In my case it was that CHECK eats its stream under IS_OFFICIAL_BUILD so my death test expectation broke.

This happened to an important CL I submitted just before going on 3 weeks holidays. It got reverted by a sheriff because one of its death tests failed on official bots...  That CL was supposed to eliminate a class of failures on TSAN bots, instead of that: many more tests were disabled because the fix was reverted (and to this day I have no way to tell how many were disabled under my radar...).

Re. each week? No, but the impact goes beyond "minor annoyance" to ultimately evolve into a culture like "avoid death tests". Seems it's cheaper to add that bot than have a way to measure whether we need it or not... 

For one bot, on one platform, it *might* be cheaper. For 7 bots, it probably isn't. For the hundreds of build configurations we have that aren't on the CQ, it certainly isn't, and you can make an argument like this to varying degrees for most if not all of them.

[Gabriel Charette]

On Thu, Jan 27, 2022 at 1:28 PM <dan...@chromium.org> wrote:

On Thu, Jan 27, 2022 at 1:23 PM Gabriel Charette <g...@chromium.org> wrote:

It can be more subtle than that. In my case it was that CHECK eats its stream under IS_OFFICIAL_BUILD so my death test expectation broke.

This happened to an important CL I submitted just before going on 3 weeks holidays. It got reverted by a sheriff because one of its death tests failed on official bots...  That CL was supposed to eliminate a class of failures on TSAN bots, instead of that: many more tests were disabled because the fix was reverted (and to this day I have no way to tell how many were disabled under my radar...).

Re. each week? No, but the impact goes beyond "minor annoyance" to ultimately evolve into a culture like "avoid death tests". Seems it's cheaper to add that bot than have a way to measure whether we need it or not... 

FWIW, this case is why we have EXPECT_CHECK_DEATH macros which should be used when looking for a CHECK() death.

I know, I added it :). But it didn't fit my use case and I forgot about IS_OFFICIAL_BUILD. Minor mistake, but cost eng time.

[Erik Staab]

We've started looking into CQ IS_OFFICIAL_BUILD coverage this quarter but nothing has been decided yet.

Many but not all of the examples I've seen so far (both for DCHECKs and IS_OFFICIAL_BUILD) seem to be caught at compile time. The cost for a compile-only builder is much lower (both in terms of machines and waiting on CQ results) so if that can catch a good number of regressions we might be able to add something without too much effort. Multiple platforms might be trickier.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7LJZWWCk0WRWAh%3D3FpZnA5S-%3Djbrjga3ON222DvLc0GbVw%40mail.gmail.com.