PSA: Run Chromium locally with DCHECKs turned on

[Avi Drissman]

DCHECKs are an important part of Chromium. One way that they help that we are all familiar with is that they signal a failure during our automated testing, where a red trybot or waterfall bot can provide data about the failure, and a bug can be filed.

However, tests do not have 100% coverage, and even if they did, they cannot have 100% coverage of every website out there. That's why it's important to, when you build and run Chromium locally, to build it with DCHECKs turned on.

As someone who builds the Chromium codebase and runs it locally, please build it with DCHECKs on (in gn args, either "is_debug = true" or at least "dcheck_always_on = true"). If you encounter a DCHECK in your local use, please file it as at least a P1 bug, if not a P0. (Use your discretion.) Only then disable that specific DCHECK in your local checkout so you can continue to do your work.

For those who receive those DCHECK failures, please take those bugs as high-priority. It's a high priority for your colleagues, as your bug (as manifested by a DCHECK failure) is blocking them, but it is also high-priority for yourself, as every DCHECK found locally and fixed is one less "impossible-to-reproduce" random crasher in the field.

I hope that, together, we can reinvigorate the use of DCHECKs to help ourselves and our colleagues to write the best code that we and they can.

Stay safe and be well,

Avi

[Peter Boström]

I personally hit 3 different DCHECKs today in very rapid succession while trying to enable and try out live captioning.

I propose we change the developer defaults for dcheck_always_on = true outside of official builds. This means we're likely to find these quickly and can revert/resolve them easier without relying on vigilant individuals.

Draft CL for this is crrev.com/c/2893204. If we can agree on goal / direction, I assume most of the actual work to get this landed will be flipping flags in buildbot land to avoid bot regressions. If this is something folks are enthusiastic about I'd be happy to look more into details and write up a proposal.

Thanks,

Peter

--
--
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/CACWgwAbEjat92cNVjFyDTb_3jNddMXSTux1%2Bcy7w5dBdh_NSvA%40mail.gmail.com.

[Dirk Pranke]

If we turn dchecks on outside of official builds, then we lose coverage of the non-dcheck case on the release builders, and this is important both (a) because it's what we ship and (b) because dcheck builds are significantly slower and the timing issues will be different.

The "developer default" is already dcheck_always_on=true, because we default to debug builds. And, we build with dcheck_always_on in the CQ.

So, while I'm a fan of people using dcheck-enabled builds more often and fixing dcheck failures, I'm not yet a fan of this change.

-- Dirk

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

[Erik Chen]

The main deterrent for me running with DCHECK more frequently is that I run into DCHECKs, file bugs, and the bugs get deprioritized [thereby meaning that every time I build/checkout I need to introduce temporary local workarounds]. 

I think we need a process change whereby the default response to a DCHECK crbug is to disable it, with owners to investigate/follow up. Otherwise a small number of deprioritized, unfixed DCHECK crbugs will prevent most developers from adopting DCHECKs.

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

[dan...@chromium.org]

On Wed, May 12, 2021 at 3:54 PM Erik Chen <erik...@chromium.org> wrote:

The main deterrent for me running with DCHECK more frequently is that I run into DCHECKs, file bugs, and the bugs get deprioritized [thereby meaning that every time I build/checkout I need to introduce temporary local workarounds]. 

I think we need a process change whereby the default response to a DCHECK crbug is to disable it, with owners to investigate/follow up. Otherwise a small number of deprioritized, unfixed DCHECK crbugs will prevent most developers from adopting DCHECKs.

+1

 

On Wed, May 12, 2021 at 12:50 PM 'Dirk Pranke' via Chromium-dev <chromi...@chromium.org> wrote:

If we turn dchecks on outside of official builds, then we lose coverage of the non-dcheck case on the release builders, and this is important both (a) because it's what we ship and (b) because dcheck builds are significantly slower and the timing issues will be different.

The "developer default" is already dcheck_always_on=true, because we default to debug builds. And, we build with dcheck_always_on in the CQ.

Most developers will immediate disable debug builds, at which point the default behaviour becomes without dchecks. Bots to could be configured to disable them explicitly to maintain coverage.

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

[Dirk Pranke]

On Wed, May 12, 2021 at 12:55 PM <dan...@chromium.org> wrote:

On Wed, May 12, 2021 at 3:54 PM Erik Chen <erik...@chromium.org> wrote:

The main deterrent for me running with DCHECK more frequently is that I run into DCHECKs, file bugs, and the bugs get deprioritized [thereby meaning that every time I build/checkout I need to introduce temporary local workarounds]. 

I think we need a process change whereby the default response to a DCHECK crbug is to disable it, with owners to investigate/follow up. Otherwise a small number of deprioritized, unfixed DCHECK crbugs will prevent most developers from adopting DCHECKs.

+1

 

On Wed, May 12, 2021 at 12:50 PM 'Dirk Pranke' via Chromium-dev <chromi...@chromium.org> wrote:

If we turn dchecks on outside of official builds, then we lose coverage of the non-dcheck case on the release builders, and this is important both (a) because it's what we ship and (b) because dcheck builds are significantly slower and the timing issues will be different.

The "developer default" is already dcheck_always_on=true, because we default to debug builds. And, we build with dcheck_always_on in the CQ.

Most developers will immediate disable debug builds, at which point the default behaviour becomes without dchecks. Bots to could be configured to disable them explicitly to maintain coverage.

If that's true (I have no idea), maybe the right question is then: why don't we fix debug builds so that people don't need to disable them?

-- Dirk 

[Ken Rockot]

On Wed, May 12, 2021 at 12:53 PM Erik Chen <erik...@chromium.org> wrote:

The main deterrent for me running with DCHECK more frequently is that I run into DCHECKs, file bugs, and the bugs get deprioritized [thereby meaning that every time I build/checkout I need to introduce temporary local workarounds]. 

I think we need a process change whereby the default response to a DCHECK crbug is to disable it, with owners to investigate/follow up. Otherwise a small number of deprioritized, unfixed DCHECK crbugs will prevent most developers from adopting DCHECKs.

I worry that this would be a counterproductive default policy. In many cases the DCHECK is correct, and the direct owner is not necessarily responsible for all code paths which might lead to its violation.

 

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

[Wez]

#pileon

Folks,

Regardless of what build settings you use locally, if you are on Windows you can opt-in to the "Albatross" build, and force-enable DCHECKs in that to get an Official experience, with the joyful exuberance of DCHECK goodness.

Perhaps we should provide that for Linux and Mac as well. :)

Wez

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

[Avi Drissman]

On Wed, May 12, 2021 at 3:59 PM Dirk Pranke <dpr...@google.com> wrote:

If that's true (I have no idea), maybe the right question is then: why don't we fix debug builds so that people don't need to disable them?

The problem is that it's a downward spiral. If people run into DCHECKs during normal ToT use, they turn off DCHECKs of their own to get their job done. And then inadvertently add their own.

Yes, we should be fixing debug builds so that DCHECKs don't need to be disabled. And in the vast majority of cases, those fixes happen. However, there is a culture shift needed here, so that unaddressed DCHECKs aren't left hanging, like they are today in rare cases.

Avi

[Dirk Pranke]

That's fine, and I'm in favor of trying to make *that* culture shift, rather than trying to shift to something to work around the problem.

-- Dirk

 

Avi

[Peter Boström]

Personally I disabled debug for performance, so I'll be another reason for why is_debug=false. I do dcheck_always_on=true then though, but that's because I've found out about this flag. I think there's a lot of folks who flipped between debug/release without considering DCHECK coverage as a part of that.

If we rely on Release bots building without DCHECKs to match official release, then that could definitely be part of that rollout plan. I'm not proposing we submit this as is, but would like for it to be the default on both debug/release.

On Wed, May 12, 2021 at 1:06 PM Avi Drissman <a...@google.com> wrote:

[Avi Drissman]

I'm not sure I agree.

Because we're all committers, and we have the authority to add code to the codebase, we have the responsibility to handle issues in the code we add.

If you add a DCHECK to the code, and it's firing enough that it's blocking your colleagues from doing their work, then it's up to you to figure out what's going on and either fix it or figure out who's the right person to fix it.

The proposal of disabling DCHECKs is a proposal to address the problem of a broken DCHECK languishing without an owner. Perhaps that's the answer, perhaps not, but the real issue is a cultural one if we let broken DCHECKs languish.

[Avi Drissman]

On Wed, May 12, 2021 at 4:09 PM Dirk Pranke <dpr...@google.com> wrote:

That's fine, and I'm in favor of trying to make *that* culture shift, rather than trying to shift to something to work around the problem.

100% agreed.

I haven't fully thought through the implications of the disabling proposal, so I don't have a position on it yet. However, I see the first step as getting everyone on the DCHECK train, and then seeing where that leaves us in terms of actually fixing the issues that are uncovered.

Avi

[Avi Drissman]

On Wed, May 12, 2021 at 3:59 PM Ken Rockot <roc...@google.com> wrote:

I worry that this would be a counterproductive default policy. In many cases the DCHECK is correct, and the direct owner is not necessarily responsible for all code paths which might lead to its violation.

An interesting question would be how did this evolve. Did the direct owner add the DCHECK to an existing system? If so, then they don't fully understand it, and are directly responsible. Did a second developer add new code that caused the existing DCHECK in a different part of the codebase to fire? How did they do that without noticing the DCHECK firing? Did they have DCHECKs turned off?

To be clear, I am not advocating for or against this policy of turning off DCHECKs. I just want to point out how the argument against this policy is indicative of the situation we're in, where a downward spiral of people turning off DCHECKs on local builds has yielded a situation where we have to consider tough policies.

Avi

[Philip Rogers]

Would it be feasible to enable bisect_builds to bisect DCHECK-enabled builds? It's not always clear if the DCHECK author is the culprit and this would help us find an appropriate owner in a similar way as regular bugs.

--

--
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/CACWgwAa-0ej_W6mNacFC0L4wzj2MHRjJKnYHdKbdC27aoPxSEA%40mail.gmail.com.

[Dirk Pranke]

I don't believe we currently archive any DCHECK-enabled builds, so bisect_builds wouldn't work right now, but we can certainly start to do so if that's useful.

-- Dirk

[Ken Russell]

In the balance between "testing what we ship" and "strengthening our automated testing", I think that setting dcheck_always_on=true on Release builders of the Chromium waterfall would nearly always simply catch more bugs, as opposed to testing an unshipped configuration. This assertion is based on having done this on the chromium.gpu and chromium.gpu.fyi waterfalls at times in the past. The recently introduced branch waterfalls and commit queues would help ensure that critical code isn't introduced under a DCHECK-only configuration.

If this is still unpalatable to the project leadership, then maybe we could set up "Release DCHECK" (shorthand "rdc" as opposed to "rel") builder/tester bots for the primary configurations of the various OSs on the waterfall. The CQ is the main source of testing load, so a few more waterfall bots should only add a relatively low constant amount of overhead.

-Ken

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

[Justin Cohen]

Chrome for iOS has an internal-only dev channel official builder that builds with `dcheck_is_configurable = true`. That combined with a `DcheckIsFatal` experiment gives us plenty of insight into DCHECK's on crash server. This has been pretty valuable. For the occasional build with a DCHECK that breaks the app, our testers can disable DCHECKs via iOS settings.

[Dirk Pranke]

I don't actually have a strong opinion on what exactly should be tested where.

I do think that if we're occasionally shipping dcheck-enabled browsers to at least the canary/dev channels, then it probably makes sense to be testing at least some of the configs in CI.

I am also happy to spin up configs for $WHATEVER people want assuming we can get the Powers-That-Be to pay for them :).

-- Dirk

[Avi Drissman]

I'm happy to hear people talking about shipping DCHECK-enabled builds, but I want to reinforce that this has to start right here at home with us.

I need to build and run ToT Chromium to do my job. The last time I did that, I hit four different DCHECKs. Several I filed as bugs and they were fixed in a day, but one broken DCHECK was months old, well known, had many stars, and it was only after I insistently commented on the bug that yes this is still happening did it finally get fixed. This was not the first time I had this kind of experience, and I'm not the only one for whom this keeps happening.

We have broken DCHECKs throughout Chromium code which are not found through testing and that are long-lived enough that they are not revertible without undoing chains of many CLs. This is very very bad.

If we all run DCHECK-enabled builds, then these broken DCHECKs can be caught quickly, which is good for everyone. This is why Peter was suggesting changing the defaults, because defaults matter.

I'm happy to have everyone talking about adjusting the testing configs, and shipping builds, but I want to make sure this is clearly about getting everyone on board with running DCHECK builds locally.

Avi

[Sergey Ulanov]

+1 to what Peter wrote.

DCHECKs enabled by default in release builds is a good idea IMO. Release build with DCHECKs enabled is often the right choice for developer builds, but currently it requires setting two separate GN flags, which is not ideal.

--

--
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/CAGFX3sFaoT98mHKP1z_8JQ2WkDO5Dq%2BaFr1-wMH1tgbcgLuo5g%40mail.gmail.com.

[Aleks Totic]

> DCHECKs enabled by default in release builds is a good idea IMO. Release build with DCHECKs enabled is often the right choice for developer builds, but currently it requires setting two separate GN flags, which is not ideal.

There is a bit of tension between what DCHECKs are used for, and how
they behave in release builds.

Severe errors that'd cause a crash are CHECKs, not DCHECKs.

DCHECKs in code mean "something is not behaving as expected".
What is the priority that should be assigned to the bug "DCHECK fired
on this URL?"
It depends on many things:
- how hard is the fix?
- what are the consequences of unexpected behavior?
- how often does the DCHECK fire?

For example, I had a DCHECK in Tables code that checks that sum of
distributed widths is equal to total width. The check was there to
catch errors in my distribution algorithm. What it often caught
instead is slight rounding errors in floating point math, some cell
width would be off by 0.1px. I fixed all of these becaus clusterfuzz
loved triggering the floating point errror, and I really wanted to
keep the DCHECK as a safety for my algorithm. Without clusterfuzz, I'd
have probably never encountered or fixed the floating point errors,
and that would have been ok.

DCHECKs in release builds crash the renderer, which is the most severe outcome.
(At least that's what it does locally when I build Chromium with DCHECKs on).
This makes everyday browsing with DCHECKs extra annoying, every hour
there is an "Aw Snap"

Running Canary is challenging as is, I've been running it for months
because of TablesNG:
"Crash on startup; Memory leaks when Chromoting using up all the
memory on your Mac;
Slack not working".

Aleks

[Wez]

On Thu, 13 May 2021 at 02:15, 'Avi Drissman' via Chromium-dev <chromi...@chromium.org> wrote:

I'm happy to hear people talking about shipping DCHECK-enabled builds, but I want to reinforce that this has to start right here at home with us.

We're not _talking about_ shipping DCHECK-enabled builds; we _already_ ship DCHECK-enabled builds (aka Albatross), albeit only to a fraction of the Windows Canary population.

Broadening that to other platforms (particular Linux, as used by a lot of our devs) would have several benefits:

0. Failures are actually uploaded as crashes, making them visible to our tooling.

1. Developers who cross-compile (which includes me) would still be able to benefit.

2. We could use the same per-session randomization as the Albatross build, which ensures that if you _do_ hit a very-DCHECKy build, restarting the browser probably gets you back to something workable.

I need to build and run ToT Chromium to do my job. The last time I did that, I hit four different DCHECKs. Several I filed as bugs and they were fixed in a day, but one broken DCHECK was months old, well known, had many stars, and it was only after I insistently commented on the bug that yes this is still happening did it finally get fixed. This was not the first time I had this kind of experience, and I'm not the only one for whom this keeps happening.

There does seem to be a fundamental problem with DCHECKs being de-prioritized because people see them as not-user-affecting, rather than as a gift that helps them debug a problem.

My experience has been that I do work with a Chromium Debug Component-build (so I have DCHECKs), but I use Google Chrome canary/dev-channel as my actual browser.  My guess would be that that is true for a lot of other Chrome devs, which is why I think DCHECK-enabled Google Chrome canary builds would be an easier way to get more folks running with DCHECKs enabled.

We have broken DCHECKs throughout Chromium code which are not found through testing and that are long-lived enough that they are not revertible without undoing chains of many CLs. This is very very bad.

Very much +1!

On Thu, 13 May 2021 at 06:51, 'Aleks Totic' via Chromium-dev <chromi...@chromium.org> wrote:

> DCHECKs enabled by default in release builds is a good idea IMO. Release build with DCHECKs enabled is often the right choice for developer builds, but currently it requires setting two separate GN flags, which is not ideal.

There is a bit of tension between what DCHECKs are used for, and how
they behave in release builds.

Severe errors that'd cause a crash are CHECKs, not DCHECKs.

DCHECKs in code mean "something is not behaving as expected".

That's an unfortunate misconception, but yes, we have a load of code that is written as though DCHECKs are test-only sanity-checks for things that won't affect production adversely if they're not met.  That is not the intended usage of DCHECKs, though.

DCHECKs express inviolable pre-conditions of the code that follows, that should follow from the structure of the surrounding code.

e.g. we use DCHECKs for thread-checks, non-null checks on arguments, etc.

Situations in which DCHECK is not appropriate include:

1. Those in which the consequences of failing would be user-facing badness, or potential for exploits. e.g. don't DCHECK that an index received over IPC is in-range.

2. Those in which the check _can_ fail, in practice, in real-world use, where we know they won't in our test environments. e.g. don't DCHECK system call return-values.

 

What is the priority that should be assigned to the bug "DCHECK fired
on this URL?"
It depends on many things:
- how hard is the fix?
- what are the consequences of unexpected behavior?
- how often does the DCHECK fire?

The priority for any DCHECK bug is P1 (possibly P0, if it falls into the "potential for exploits" category); it may be the case that the effects are determined to be bening and low-priority to fix, but in that case the DCHECK should be commented-out with a reference to the relevant bug, at least.

For example, I had a DCHECK in Tables code that checks that sum of
distributed widths is equal to total width. The check was there to
catch errors in my distribution algorithm. What it often caught
instead is slight rounding errors in floating point math, some cell
width would be off by 0.1px. I fixed all of these becaus clusterfuzz
loved triggering the floating point errror, and I really wanted to
keep the DCHECK as a safety for my algorithm. Without clusterfuzz, I'd
have probably never encountered or fixed the floating point errors,
and that would have been ok.

That's a good example of a DCHECK that expresses a condition that is not valid in real usage.  You'd address it by allowing some tolerance in the check, i.e. effectively encode your intuition on whether the DCHECK "failures" are real failures into the check itself.

DCHECKs in release builds crash the renderer, which is the most severe outcome.
(At least that's what it does locally when I build Chromium with DCHECKs on).

That's great!  Please make sure to file bugs for those - they won't get fixed otherwise. :) 

Wez

[Satoru Takabayashi]

On Thu, May 13, 2021 at 5:28 AM 'Avi Drissman' via Chromium-dev <chromi...@chromium.org> wrote:

On Wed, May 12, 2021 at 3:59 PM Ken Rockot <roc...@google.com> wrote:

I worry that this would be a counterproductive default policy. In many cases the DCHECK is correct, and the direct owner is not necessarily responsible for all code paths which might lead to its violation.

I think this happens commonly with DCHECKs in low level code (ex. thread_restrictions.cc).

 

An interesting question would be how did this evolve. Did the direct owner add the DCHECK to an existing system? If so, then they don't fully understand it, and are directly responsible. Did a second developer add new code that caused the existing DCHECK in a different part of the codebase to fire? How did they do that without noticing the DCHECK firing? Did they have DCHECKs turned off?

I think it's the latter in many cases.

To be clear, I am not advocating for or against this policy of turning off DCHECKs. I just want to point out how the argument against this policy is indicative of the situation we're in, where a downward spiral of people turning off DCHECKs on local builds has yielded a situation where we have to consider tough policies.

Avi

--

--
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/CACWgwAYB%3DQYEawhobSq0%3Dm8BMN2zwCspO2CXJDG1HWVxpoXAkA%40mail.gmail.com.

[Chris Lam]

My 2c on this.

I like DCHECKs. I write DCHECKs. I never run with DCHECKs.

It's just too unstable, and my number one priority day to day is to compile and run Chrome and develop the code I'm working on. I still use them because they're a great way to explicitly state your assumptions/preconditions, which helps other developers contextualize your code.

But regarding turning on DCHECKs locally, if we want more reporting of DCHECK violations, why don't we make day to day life easier for local DCHECK-on builds? Do we really need to crash the browser when one of these conditions is violated? Can't we have a gn flag to log DCHECK failures as a Big Red Error with a stack trace, and suggest the developer file a bug? In my opinion, the less friction there is, the more adoption there'll be.

Also, if there was a way to easily find out if a bug was already filed, that would reduce the bystander effect that I get when I run into a DCHECK failure.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CACjFnbyYpgmDr-J%2B0GUjD49%2B_%3DqTyO7ZpSmwH3HmhLZus22rvw%40mail.gmail.com.

[Wez]

We already have a flag which causes DCHECKS to be checked, but to then be reported at ERROR level - you can set |dcheck_is_configurable=true| to build with that.  It implies |dcheck_always_on|, so you can replace one with the other.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CA%2Bg1BH0HO6zoLFv1Jcep1GSE_kDZGVtVAROpFc9O%2BqE%3DyLoD1A%40mail.gmail.com.

[dan...@chromium.org]

Logging that undefined behaviour is happening doesn't seem like a great situation to be in. Honestly it sounds like maybe we should be writing CHECKs instead of DCHECKs, if we're in a situation where the behaviour of Chromium is so undefined that developers are not able to run it without encountering that constantly. A product where we can expect undefined behaviour for users is a terrible place to be IMO.

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

[Sylvain Defresne]

Most of the people building the iOS version of Chromium build and run with DCHECK enabled. On that platform, encountering DCHECK failure is unusual (it happens from time to time). This makes detecting regression easy. If a DCHECK starts failing, it is usually caused by a recently landed CL. CLs that introduce DCHECK failure are usually quickly reverted (especially if the DCHECK fails frequently).

I think being able to run Chromium with DCHECK enabled locally is good to catch bugs and is achievable (at least one platform has been able to put this under control). It may be a pain if only a few developers run in that configuration as it puts the workload on those few developers. If most of the developers have DCHECK enabled, then I'm confident that the current situation can get under control eventually. Once this is done, then new regression will be caught earlier and will have less time to impact everyone.

-- Sylvain

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

[Raymond Toy]

I generally run with DCHECKS on, mostly to catch mistakes in the code I'm writing.  Sometimes I run into failures in other code so I turn it off because I'm trying to get something done right now.  Most of the dcheck other failures I've run in to get fixed by someone in a day or two so I haven't been diligent about filing bugs about the failures.

The only other time I run with DCHECKS off is if I'm trying to benchmark something.  (And then I sometimes forget to turn it back on when I'm done.)

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

[Alexei Svitkine]

What if we treat failing DCHECKs the same as we do failing unit tests?

i.e. the policy is to disable them with a bug and assign them to the appropriate owner.

This would have several benefits:

  - We can quickly get to a steady-state where failing DCHECKs are rare, so more people can run with DCHECKs and catch issues sooner. Right now, these failing DCHECKs may be masking other issues from being introduced (resulting in more DCHECKs failing over time that are unnoticed).

  - We get a consistent policy between handling unit tests and DCHECKs.

  - The downside of those DCHECKs being disabled isn't such a big problem - after all, they're already disabled in production releases. It's not like we're losing coverage much - it's more that we'd be gaining coverage for the remaining DCHECKs (by virtue of more users being able to run DCHECK builds).

Of course, that's not to say the test disabling policy is perfect, there's certainly tests that stay disabled for a long time. But that side of the problem can be fixed separately (via discussions of what work teams should prioritize). And anyway, having a consistent policy simplifies things (can be reasoned about in the same way).

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAE3TgXFPi34Fh9p0DUp_BrN-aO%2B-zs_jW9L%3DgYHiTxF0dWcG_w%40mail.gmail.com.

[Avi Drissman]

On Tue, May 18, 2021 at 1:31 AM Chris Lam <cala...@google.com> wrote:

My 2c on this.

I like DCHECKs. I write DCHECKs. I never run with DCHECKs.

It's just too unstable, and my number one priority day to day is to compile and run Chrome and develop the code I'm working on.

This is, unfortunately, exactly my point. Our state, full of broken DCHECKs, is so bad because no one runs with them on, and it’s a deadly spiral. Because few people run with them turned on, the broken DCHECKs that are committed are never caught, and then people have even less of an incentive to run with DCHECKs on.

 

I still use them because they're a great way to explicitly state your assumptions/preconditions, which helps other developers contextualize your code.

If people don’t run with DCHECKs on, then that’s no better than documenting your assumptions/preconditions in a comment.

 

But regarding turning on DCHECKs locally, if we want more reporting of DCHECK violations, why don't we make day to day life easier for local DCHECK-on builds? Do we really need to crash the browser when one of these conditions is violated? Can't we have a gn flag to log DCHECK failures as a Big Red Error with a stack trace, and suggest the developer file a bug? In my opinion, the less friction there is, the more adoption there'll be.

Also, if there was a way to easily find out if a bug was already filed, that would reduce the bystander effect that I get when I run into a DCHECK failure.

I’ve never found DCHECK failures to be difficult to search for. And even if you fail to find the bug that’s already there, you can ping someone in the area of the code and ask if they know of the bug.

[dan...@chromium.org]

On Tue, May 18, 2021 at 11:05 AM Alexei Svitkine <asvi...@chromium.org> wrote:

What if we treat failing DCHECKs the same as we do failing unit tests?

i.e. the policy is to disable them with a bug and assign them to the appropriate owner.

This would have several benefits:

  - We can quickly get to a steady-state where failing DCHECKs are rare, so more people can run with DCHECKs and catch issues sooner. Right now, these failing DCHECKs may be masking other issues from being introduced (resulting in more DCHECKs failing over time that are unnoticed).

  - We get a consistent policy between handling unit tests and DCHECKs.

  - The downside of those DCHECKs being disabled isn't such a big problem - after all, they're already disabled in production releases.

They are disabled in production, but that is under the assumption they are impossible to occur there. I don't think that leads to saying there's no downside to disabling them. A failing DCHECK means our code is doing something unexpected, and it could lead to any sort of surprising result. That we can't know if it's happening on users 'machines does not devalue the urgency of knowing that it's happening and fixing it IMO.

 

It's not like we're losing coverage much - it's more that we'd be gaining coverage for the remaining DCHECKs (by virtue of more users being able to run DCHECK builds).

Of course, that's not to say the test disabling policy is perfect, there's certainly tests that stay disabled for a long time. But that side of the problem can be fixed separately (via discussions of what work teams should prioritize). And anyway, having a consistent policy simplifies things (can be reasoned about in the same way).

They are similar, however check failures appear to be even more urgent than a test failure to me. Test failures are often due to our test environment/infrastructure or the way the test is written. DCHECKs firing implies that chrome itself can exhibit this behaviour and unpredictable results will follow for a user.

[Avi Drissman]

On Tue, May 18, 2021 at 11:05 AM Alexei Svitkine <asvi...@chromium.org> wrote:

What if we treat failing DCHECKs the same as we do failing unit tests?

i.e. the policy is to disable them with a bug and assign them to the appropriate owner.

This was Erik Chen’s proposal upstream. It has upsides and downsides.

If we do that, I would like to see that reserved for the most severe DCHECK failures, where it’s complicated to get a fix in. Most of the DCHECKs I’ve filed when running with DCHECKs enabled have been fixed within a day. People are pretty responsive if you file bugs (and then give them a gentle ping :) ).

[Avi Drissman]

On Tue, May 18, 2021 at 9:47 AM Sylvain Defresne <sdef...@chromium.org> wrote:

Most of the people building the iOS version of Chromium build and run with DCHECK enabled.

Has the team working on the iOS platform been diligent from day 1 in running with DCHECK enabled and kept it working well, or did the team slip and have to return to that?

I’m asking because this seems like a tragedy of the commons situation. The problem is that Chromium is full of broken DCHECKs, and the incentive is for every developer to take the route of running without DCHECKs.

[Alexei Svitkine]

On Tue, May 18, 2021 at 11:31 AM <dan...@chromium.org> wrote:

On Tue, May 18, 2021 at 11:05 AM Alexei Svitkine <asvi...@chromium.org> wrote:

What if we treat failing DCHECKs the same as we do failing unit tests?

i.e. the policy is to disable them with a bug and assign them to the appropriate owner.

This would have several benefits:

  - We can quickly get to a steady-state where failing DCHECKs are rare, so more people can run with DCHECKs and catch issues sooner. Right now, these failing DCHECKs may be masking other issues from being introduced (resulting in more DCHECKs failing over time that are unnoticed).

  - We get a consistent policy between handling unit tests and DCHECKs.

  - The downside of those DCHECKs being disabled isn't such a big problem - after all, they're already disabled in production releases.

They are disabled in production, but that is under the assumption they are impossible to occur there. I don't think that leads to saying there's no downside to disabling them. A failing DCHECK means our code is doing something unexpected, and it could lead to any sort of surprising result. That we can't know if it's happening on users 'machines does not devalue the urgency of knowing that it's happening and fixing it IMO.

If you agree that DCHECKs provide lots of value (of detecting bad assumptions/problems), do you disagree with my argument that if we do the above:

1. We would get more value from getting coverage for DCHECKs in general (by more people being able to run DCHECK builds since they're expected to be stable).

2. Compared to the value we lose from those specific DCHECKs being disabled.

In my mind, a small minority of DCHECKs will be disabled, so we lose coverage for them. But then all the other DCHECKs will get more coverage (since it will be a configuration most people can use - e.g. in local builds).

As to prioritization of investigation and fixes, that's up to the team that owns the code and their evaluation of the DCHECK. For example, I filed a bug about a DCHECK about Linux breakpad initialization (crbug.com/1197559). The discussion on that bug is that we're migrating from breakpad to crashpad, so this code path would be going away and therefore fixing that DCHECK is not a priority. Which makes sense, so that's a case where disabling the DCHECK is the correct thing.

[dan...@chromium.org]

On Tue, May 18, 2021 at 11:45 AM Alexei Svitkine <asvi...@chromium.org> wrote:

On Tue, May 18, 2021 at 11:31 AM <dan...@chromium.org> wrote:

On Tue, May 18, 2021 at 11:05 AM Alexei Svitkine <asvi...@chromium.org> wrote:

What if we treat failing DCHECKs the same as we do failing unit tests?

i.e. the policy is to disable them with a bug and assign them to the appropriate owner.

This would have several benefits:

  - We can quickly get to a steady-state where failing DCHECKs are rare, so more people can run with DCHECKs and catch issues sooner. Right now, these failing DCHECKs may be masking other issues from being introduced (resulting in more DCHECKs failing over time that are unnoticed).

  - We get a consistent policy between handling unit tests and DCHECKs.

  - The downside of those DCHECKs being disabled isn't such a big problem - after all, they're already disabled in production releases.

They are disabled in production, but that is under the assumption they are impossible to occur there. I don't think that leads to saying there's no downside to disabling them. A failing DCHECK means our code is doing something unexpected, and it could lead to any sort of surprising result. That we can't know if it's happening on users 'machines does not devalue the urgency of knowing that it's happening and fixing it IMO.

If you agree that DCHECKs provide lots of value (of detecting bad assumptions/problems), do you disagree with my argument that if we do the above:

1. We would get more value from getting coverage for DCHECKs in general (by more people being able to run DCHECK builds since they're expected to be stable).

2. Compared to the value we lose from those specific DCHECKs being disabled.

In my mind, a small minority of DCHECKs will be disabled, so we lose coverage for them. But then all the other DCHECKs will get more coverage (since it will be a configuration most people can use - e.g. in local builds).

As to prioritization of investigation and fixes, that's up to the team that owns the code and their evaluation of the DCHECK. For example, I filed a bug about a DCHECK about Linux breakpad initialization (crbug.com/1197559). The discussion on that bug is that we're migrating from breakpad to crashpad, so this code path would be going away and therefore fixing that DCHECK is not a priority. Which makes sense, so that's a case where disabling the DCHECK is the correct thing.

I agree, if we can see that disabling the DCHECK doesn't lead to other pathalogical behaviour, and/or we can add code to handle the case incorrectly-assumed to not happen.

[Colin Blundell]

On Tue, May 18, 2021 at 5:48 PM <dan...@chromium.org> wrote:

On Tue, May 18, 2021 at 11:45 AM Alexei Svitkine <asvi...@chromium.org> wrote:

On Tue, May 18, 2021 at 11:31 AM <dan...@chromium.org> wrote:

On Tue, May 18, 2021 at 11:05 AM Alexei Svitkine <asvi...@chromium.org> wrote:

What if we treat failing DCHECKs the same as we do failing unit tests?

i.e. the policy is to disable them with a bug and assign them to the appropriate owner.

This would have several benefits:

  - We can quickly get to a steady-state where failing DCHECKs are rare, so more people can run with DCHECKs and catch issues sooner. Right now, these failing DCHECKs may be masking other issues from being introduced (resulting in more DCHECKs failing over time that are unnoticed).

  - We get a consistent policy between handling unit tests and DCHECKs.

  - The downside of those DCHECKs being disabled isn't such a big problem - after all, they're already disabled in production releases.

They are disabled in production, but that is under the assumption they are impossible to occur there. I don't think that leads to saying there's no downside to disabling them. A failing DCHECK means our code is doing something unexpected, and it could lead to any sort of surprising result. That we can't know if it's happening on users 'machines does not devalue the urgency of knowing that it's happening and fixing it IMO.

If you agree that DCHECKs provide lots of value (of detecting bad assumptions/problems), do you disagree with my argument that if we do the above:

1. We would get more value from getting coverage for DCHECKs in general (by more people being able to run DCHECK builds since they're expected to be stable).

2. Compared to the value we lose from those specific DCHECKs being disabled.

In my mind, a small minority of DCHECKs will be disabled, so we lose coverage for them. But then all the other DCHECKs will get more coverage (since it will be a configuration most people can use - e.g. in local builds).

As to prioritization of investigation and fixes, that's up to the team that owns the code and their evaluation of the DCHECK. For example, I filed a bug about a DCHECK about Linux breakpad initialization (crbug.com/1197559). The discussion on that bug is that we're migrating from breakpad to crashpad, so this code path would be going away and therefore fixing that DCHECK is not a priority. Which makes sense, so that's a case where disabling the DCHECK is the correct thing.

I agree, if we can see that disabling the DCHECK doesn't lead to other pathalogical behaviour, and/or we can add code to handle the case incorrectly-assumed to not happen.

Even if we don't make any code changes when disabling a DCHECK that goes off, all we're doing by disabling the DCHECK is giving Chrome developers that run with DCHECK enabled the same experience that other Chrome developers or our users have in this particular flow. i.e., the "other pathological behavior" that you're describing is actually already Chrome users' experience :). Of course, that experience might be quite a bad one that should be fixed, but that's true irrespective of whether the DCHECK in question is enabled or not.

I think that it would be worth investing in us reaching a world where most/~all Chrome developers run locally with DCHECKs enabled. Alexei's policy (or some variant of it) sounds to me like a good approach for us to move there. Once we got to a state that's like Sylvain describes for iOS, we could treat failing DCHECKs as something like a P0, where it gets resolved in a ~day. In the meantime we would likely need to go through a period where we aggressively disable a bunch of DCHECKs with bugs that might take longer to resolve. I think that that would be worth it to reach a place that would be significantly better than the current state.

Best,

Colin

  

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaSjZ_wOFX%2Boi2n_%3DVW%3DxXrRV%2Bnge_WqhOSuWm0c_LJkaw%40mail.gmail.com.

[Rohit Rao]

On Tue, May 18, 2021 at 11:40 AM 'Avi Drissman' via Chromium-dev <chromi...@chromium.org> wrote:

On Tue, May 18, 2021 at 9:47 AM Sylvain Defresne <sdef...@chromium.org> wrote:

Most of the people building the iOS version of Chromium build and run with DCHECK enabled.

Has the team working on the iOS platform been diligent from day 1 in running with DCHECK enabled and kept it working well, or did the team slip and have to return to that?

I’m asking because this seems like a tragedy of the commons situation. The problem is that Chromium is full of broken DCHECKs, and the incentive is for every developer to take the route of running without DCHECKs.

iOS has always had DCHECKs enabled for developer builds, but we did not always address failures and sometimes folks would need to comment out DCHECKs when developing locally.  About two years ago we made a concerted effort to fix broken DCHECKs, and after that we enabled them in internal canary builds, which has helped to keep us in a non-broken state. On the waterfall, we have ios-simulator tests running with DCHECKs and ios-device running without DCHECKs.

I'm sure that the scale of the problem on desktop is far larger than what we had to deal with on iOS, but if we can get back to a known-good state, maintaining that should become a lot easier. From my experience, I think it's worth the effort.

- Rohit

[Peter Kasting]

On Tue, May 18, 2021 at 9:20 AM Rohit Rao <rohi...@chromium.org> wrote:

I'm sure that the scale of the problem on desktop is far larger than what we had to deal with on iOS, but if we can get back to a known-good state, maintaining that should become a lot easier. From my experience, I think it's worth the effort.

I don't even think the problem is necessarily far larger.  I run DCHECK-enabled builds sometimes and don't tend to have any problems.  I am skeptical of claims that things are so broken we can't just do this.

Frankly, I'd rather see us getting to the point where we could ship some percentage of Dev builds with DCHECK on.  I'd turn them on on my daily-drive experience then.

PK

[Chris Hamilton]

This mirrors my experience. My usual build configuration is release + dcheck_always_on, and I rarely encounter DCHECK failures. It could be that different parts of the codebase are unhealthier than others, and the likelihood of you encountering a DCHECK depends largely on the projects and code-paths you are exercising during your typical dev work.

I also like the approach of an aggressive burndown to get things in reasonable shape, followed by a switch of the default build configuration for developers.

PK

--
--
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/CAAHOzFC_GceQsPLGB%2Bgtjy_tstz-ZiQ6AhFSqKu_wKmH%3D-o-VQ%40mail.gmail.com.

[Sven Zheng]

There are concerns that enabling DCHECK can slow down development. Can we first set up some monkey tests(which just randomly click) on CI with DCHECK on? Hopefully the builder can catch some DCHECK failures. I don't know how often DCHECK breaks so not sure how useful this can be.

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

[Chris Hamilton]

As mentioned earlier we already have something slightly better than this for the Windows platform, where we ship dcheck-enabled builds to a population of users (Albatross builds); users explore substantially more of Chrome's feature space than any simple CI tests would do. We don't have anything like this for other platforms, however, so something on the CI could be a good starting point there.

Is there appetite / ability to do something like the Albatross builds for other platforms?

Cheers,

Chris

[Ben Kelly]

On Tue, May 18, 2021 at 8:35 PM Chris Hamilton <chr...@chromium.org> wrote:

As mentioned earlier we already have something slightly better than this for the Windows platform, where we ship dcheck-enabled builds to a population of users (Albatross builds); users explore substantially more of Chrome's feature space than any simple CI tests would do. We don't have anything like this for other platforms, however, so something on the CI could be a good starting point there.

FWIW, I purposely stopped running canary as my daily driver when albatross builds started shipping.  I found the performance to be noticeably slower and I felt it reduced my productivity.  I don't know if this was just due to DCHECK or something else (I recall they are 32-bit builds or something).

 

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

[Avi Drissman]

On Tue, May 18, 2021 at 8:26 PM Sven Zheng <sven...@chromium.org> wrote:

There are concerns that enabling DCHECK can slow down development.

I don't understand those concerns; can you clarify what you mean by "slow down development"?

DCHECKs are the invariants of the code you are writing, in that you are writing in machine-runnable format your expectations about how the code should behave. If you are not running a DCHECK-enabled build, how are you verifying that the code you are writing is actually following the expectations you've written into the code?

I would argue that it speeds up development to actually test the DCHECKs you're putting into the code yourself. If you put DCHECKs into the code but don't run with DCHECKs enabled, you're outsourcing to other people the basic validation of the code that you've written, which is slower.

[Dirk Pranke]

On Tue, May 18, 2021 at 7:21 PM Avi Drissman <a...@google.com> wrote:

On Tue, May 18, 2021 at 8:26 PM Sven Zheng <sven...@chromium.org> wrote:

There are concerns that enabling DCHECK can slow down development.

I don't understand those concerns; can you clarify what you mean by "slow down development"?

DCHECKs are the invariants of the code you are writing, in that you are writing in machine-runnable format your expectations about how the code should behave. If you are not running a DCHECK-enabled build, how are you verifying that the code you are writing is actually following the expectations you've written into the code?

I would argue that it speeds up development to actually test the DCHECKs you're putting into the code yourself. If you put DCHECKs into the code but don't run with DCHECKs enabled, you're outsourcing to other people the basic validation of the code that you've written, which is slower.

I imagine he means that because Chrome is slower, it takes you more time to do a lot of things.

This is probably obvious, but you're not just running your DCHECKs, you're running everyone else's. DCHECK-enabled builds are something like 30-50% slower than regular release builds, I think (I'd have to double check).

I would, of course, welcome any effort we put into speeding up DCHECK-enabled builds, though.

-- Dirk

[Avi Drissman]

The question of using DCHECK-enabled builds for everyday use is separable from using them for development. While I think that the former is probably a good idea, we absolutely need to have everyone use them when building Chromium to do feature development.

If you are building a feature, you absolutely should be running everyone's DCHECKs in your local builds. You are doing development, and you need to ensure that your new code does not trigger any of your own DCHECKs as well as not trigger anyone else's. That's why everyone else's DCHECKs are there! There is a lot of code in Chromium, and no one knows it all. DCHECKs are there to help everyone code things correctly.

The vast majority of feature development is not speed sensitive. No matter how much slower a DCHECK-enabled build is, you should be paying that price to make sure that the code you are committing to the code base is solid.

Avi

[Dirk Pranke]

I suppose it depends on what you mean by "using them for development". To me, using a browser for development means accessing Monorail, Gerrit, etc, not just testing code paths I'm modifying in the browser. But, it sounds like you have the latter more in mind.

-- Dirk

[Avi Drissman]

That was my intent for the original PSA. When I’m trying to do actual development, and I can’t even access a test page without hitting four different DCHECKs each crashing the browser in a different way, it’s clear that we have to start with getting a ten-minute browser for the purpose of being able to do any development at all before considering moving to the next stage of being able to do things like access Monorail and Gerrit in it.

Avi

[K. Moon]

I'm not sure if I'm misunderstanding something, but I would never use a freshly-built browser for regular Web browsing; I do all that using an official binary. (Besides performance and stability, I don't extend the same level of trust to a trunk build.) Is this a common workflow?

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

[Peter Kasting]

On Wed, May 19, 2021 at 11:05 AM Avi Drissman <a...@google.com> wrote:

I can’t even access a test page without hitting four different DCHECKs each crashing the browser in a different way,

I have literally never had an experience that bad.

I do think things are distinctly worse on Mac than other platforms, DCHECK-wise, based on some of the data about test flakiness in the last Brews & Bites.  Perhaps you're running into this, and I'm not since I'm on Windows.

PK

[Avi Drissman]

Because I rarely do builds on non-Mac platforms, I cannot personally speak to the platform specificity of this, but this is most certainly not a Mac-only issue. There are other people who I've spoken to who aren't fond of the spotlight and who have asked to not be named, but they definitely experience this issue, and they are not Mac folks.

Avi

[Ken Russell]

To voice support for Avi's and others' standpoints, I fully endorse the idea of making dcheck_always_on=true in Release builds the default configuration for Chromium developers, and doing a Fixit / Code Yellow / whatever is needed to get it to the point of stability, and then to keep it there. This is the configuration I use personally, and usually only disable dchecks when doing performance testing. I haven't often seen stability problems as bad as Avi has, but I'm likely not exercising my own developer builds as much.

-Ken

--

--
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/CACWgwAZ5v5%2BsD-dcVQVLg608EAaGcoc9zFnTZzRc7ai0FTDrQQ%40mail.gmail.com.

[Dirk Pranke]

If we make that the default configuration for Chromium developers (whatever that means, I'm assuming we change the defaults in GN, for example), why would we keep the Debug configuration?

I.e., why shouldn't we just make Debug default to that?

-- Dirk

[Ken Russell]

Debug is unusably slow. As I understand it, V8 injects a lot more checks in Debug builds - heap verification and such. We've gone back and forth on this years ago but to date there haven't been any configuration changes. Release + DCHECKs remains a good balance.

-Ken

[Dirk Pranke]

Put perhaps a different way:

What sets of coverage would we want for Debug vs Release+DCHECK vs Release vs Official? 

Right now we have no CQ/presubmit test coverage for Debug by default, "full" coverage for Release+DCHECK, and limited for Release and Official, and we have "full" CI/postsubmit coverage for Debug and Release, limited for Release+DCHECK, and very little for Official.

Should that change? We can't simply cover everything everywhere without cost.

-- Dirk

[Dirk Pranke]

There have been config changes on this in the past (I know, because I worked with the v8 team to make them happen), though I would not be at all surprised to hear that things have regressed since. 

But, I'll restate what I said earlier: I'm not really interested in us testing a configuration that no one finds useful. If v8 wants extended coverage of their asserts, they can do that on their bots, but perhaps we should change what our "default" for Debug is if no one actually uses the current one.

(I know Avi didn't want to tackle that, for understandable reasons, but I will if that means I don't have to support more configs in CI :).

-- Dirk

[Jakob Kummerow]

From a V8 perspective: +1 to running with DCHECKs.

Should V8 constitute an obstacle for this plan in any way, we'll fix it. We already have a distinction between DCHECK and SLOW_DCHECK, where the latter need an extra build time flag + runtime flag to turn on. And we have a very strong (and ClusterFuzz-supported) culture to treat every failing DCHECK in V8 as a high-priority bug (so I highly doubt you'll run into any; but if you do, please let us know!).

I actually run full Debug builds of Chromium (with renderers in GDB) every now and then, when chasing down crashes. Hitting unrelated DCHECKs as part of that has been an occasional but not super-frequent annoyance in the past. I agree with what's been said that full Debug builds are too slow for regular usage (but they're very useful for debugging!).

I don't think the purpose of Debug builds is to provide more coverage than Release+DCHECK. I think their purpose is to be debuggable in debuggers :-)

[dan...@chromium.org]

On Thu, May 20, 2021 at 10:05 AM Jakob Kummerow <jkum...@chromium.org> wrote:

From a V8 perspective: +1 to running with DCHECKs.

Should V8 constitute an obstacle for this plan in any way, we'll fix it. We already have a distinction between DCHECK and SLOW_DCHECK, where the latter need an extra build time flag + runtime flag to turn on. And we have a very strong (and ClusterFuzz-supported) culture to treat every failing DCHECK in V8 as a high-priority bug (so I highly doubt you'll run into any; but if you do, please let us know!).

I actually run full Debug builds of Chromium (with renderers in GDB) every now and then, when chasing down crashes. Hitting unrelated DCHECKs as part of that has been an occasional but not super-frequent annoyance in the past. I agree with what's been said that full Debug builds are too slow for regular usage (but they're very useful for debugging!).

I don't think the purpose of Debug builds is to provide more coverage than Release+DCHECK. I think their purpose is to be debuggable in debuggers :-)

+1. I use debug if I need to run in a debugger. Since I mostly printf debug, I run in release+component+dchecks. It builds fast and runs fast and includes the correctness checks.

I think debug == !NDEBUG is correct and changing that relationship would not be great. I would suggest dchecks_always_on=true be the default unless is_official_build=true (our true "release" build), and we disable it explicitly on bots that want to test without them.

[dan...@chromium.org]

On Thu, May 20, 2021 at 10:13 AM <dan...@chromium.org> wrote:

On Thu, May 20, 2021 at 10:05 AM Jakob Kummerow <jkum...@chromium.org> wrote:

From a V8 perspective: +1 to running with DCHECKs.

Should V8 constitute an obstacle for this plan in any way, we'll fix it. We already have a distinction between DCHECK and SLOW_DCHECK, where the latter need an extra build time flag + runtime flag to turn on. And we have a very strong (and ClusterFuzz-supported) culture to treat every failing DCHECK in V8 as a high-priority bug (so I highly doubt you'll run into any; but if you do, please let us know!).

I actually run full Debug builds of Chromium (with renderers in GDB) every now and then, when chasing down crashes. Hitting unrelated DCHECKs as part of that has been an occasional but not super-frequent annoyance in the past. I agree with what's been said that full Debug builds are too slow for regular usage (but they're very useful for debugging!).

I don't think the purpose of Debug builds is to provide more coverage than Release+DCHECK. I think their purpose is to be debuggable in debuggers :-)

+1. I use debug if I need to run in a debugger. Since I mostly printf debug, I run in release+component+dchecks. It builds fast and runs fast and includes the correctness checks.

I think debug == !NDEBUG is correct and changing that relationship would not be great. I would suggest dchecks_always_on=true be the default unless is_official_build=true (our true "release" build), and we disable it explicitly on bots that want to test without them.

I think we have ~3 levels of builds:

debug - debug build (as specified to the compiler), !NDEBUG, symbols etc for debuggers.

developer - release build, component usually, some symbols for stacks, dchecks on (hopefully)

fake-official-for-bots - developer build but without dchecks

official - release build, no symbols, slower build for better optimization, static linking

[Peter Boström]

Fwiw these 4 levels make sense to me. Both checking debug and release by default on Dev builds is super desirable to me.

[Dirk Pranke]

It is possible (I think on every platform) to have a "release" build that is debuggable in a debugger (just set symbol_level=2), though IIUC it may be that stepping through some code paths may be different than what you'd get in a debug build with different optimization levels.

Is the idea behind NDEBUG that you'd have an optimized code path in NDEBUG that you can't easily follow under a debugger? Is there value in (say) executing those code paths on the bots by running tests under a "debug build"? Note that we don't normally build with fully symbols on the bots (since it's a waste of disk space and time), and part of my goal in this thread is to understand what, if any, reasons we *do* have for running a regular debug build (as opposed to a build w/ DCHECKs enabled) on the bots.

Note also that the official builds actually do build with full symbols, since we want those for the builds we ship :).

-- Dirk

[Peter Kasting]

On Thu, May 20, 2021 at 12:36 PM Dirk Pranke <dpr...@google.com> wrote:

Is the idea behind NDEBUG that you'd have an optimized code path in NDEBUG that you can't easily follow under a debugger?

Yes, it often results in interesting (and hard to follow) callstacks.

I would want "debug" bots to be !NDEBUG to get good stacks, which just symbol_level=2 alone does not guarantee you.

PK

[Torne (Richard Coles)]

I just use is_debug=true basically all the time and do not find it unusably slow, but am running on Android where we just use -Oz even when is_debug=true. For the folks whose primary issue with debug builds is performance, that's potentially another option to release with 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/CAAHOzFDA8%3DNb-HMY2_N8O2-ggeU_OZFYXWsGyDJ1BwoH2c4u_w%40mail.gmail.com.

[Ken Russell]

At least as of ~7 months ago, lldb was unusably slow against Chromium's debug builds on Mac:

https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/2312f725-99a7-444c-bd00-0b8c891be449%40chromium.org?utm_medium=email&utm_source=footer

It would be fantastic to fix this but I'm not sure whether there are algorithmic bugs in lldb, or whether Chromium's just grown too big.

Because Debug builds are non-debuggable on my setup, and because their performance at runtime is much slower than Release, essentially all of my work is done in Release+DCHECK builds.

-Ken

[Zentaro Kavanagh]

I strongly agree with having them on by default and organizationally mandating an SLO to resolve DCHECK bugs.

I like DCHECKs and wish people would use them more, but the value is limited if people don't enable them. DCHECKs are much better for code health than having people writing essential dead code to handle things logically we can assert aren't possible, and then having people write tests for that dead code, and having future developer come and duplicate that code "just in case".

If a DCHECK is firing - the code is wrong and we should care about that. The problem isn't the DCHECK it's the broken code.

On Tue, May 18, 2021 at 8:33 AM <dan...@chromium.org> wrote:

On Tue, May 18, 2021 at 11:05 AM Alexei Svitkine <asvi...@chromium.org> wrote:

What if we treat failing DCHECKs the same as we do failing unit tests?

i.e. the policy is to disable them with a bug and assign them to the appropriate owner.

This would have several benefits:

  - We can quickly get to a steady-state where failing DCHECKs are rare, so more people can run with DCHECKs and catch issues sooner. Right now, these failing DCHECKs may be masking other issues from being introduced (resulting in more DCHECKs failing over time that are unnoticed).

  - We get a consistent policy between handling unit tests and DCHECKs.

  - The downside of those DCHECKs being disabled isn't such a big problem - after all, they're already disabled in production releases.

They are disabled in production, but that is under the assumption they are impossible to occur there. I don't think that leads to saying there's no downside to disabling them. A failing DCHECK means our code is doing something unexpected, and it could lead to any sort of surprising result. That we can't know if it's happening on users 'machines does not devalue the urgency of knowing that it's happening and fixing it IMO.

 

It's not like we're losing coverage much - it's more that we'd be gaining coverage for the remaining DCHECKs (by virtue of more users being able to run DCHECK builds).

Of course, that's not to say the test disabling policy is perfect, there's certainly tests that stay disabled for a long time. But that side of the problem can be fixed separately (via discussions of what work teams should prioritize). And anyway, having a consistent policy simplifies things (can be reasoned about in the same way).

They are similar, however check failures appear to be even more urgent than a test failure to me. Test failures are often due to our test environment/infrastructure or the way the test is written. DCHECKs firing implies that chrome itself can exhibit this behaviour and unpredictable results will follow for a user.

 

 

On Tue, May 18, 2021 at 10:38 AM 'Raymond Toy' via Chromium-dev <chromi...@chromium.org> wrote:

I generally run with DCHECKS on, mostly to catch mistakes in the code I'm writing.  Sometimes I run into failures in other code so I turn it off because I'm trying to get something done right now.  Most of the dcheck other failures I've run in to get fixed by someone in a day or two so I haven't been diligent about filing bugs about the failures.

The only other time I run with DCHECKS off is if I'm trying to benchmark something.  (And then I sometimes forget to turn it back on when I'm done.)

On Tue, May 18, 2021 at 6:48 AM Sylvain Defresne <sdef...@chromium.org> wrote:

Most of the people building the iOS version of Chromium build and run with DCHECK enabled. On that platform, encountering DCHECK failure is unusual (it happens from time to time). This makes detecting regression easy. If a DCHECK starts failing, it is usually caused by a recently landed CL. CLs that introduce DCHECK failure are usually quickly reverted (especially if the DCHECK fails frequently).

I think being able to run Chromium with DCHECK enabled locally is good to catch bugs and is achievable (at least one platform has been able to put this under control). It may be a pain if only a few developers run in that configuration as it puts the workload on those few developers. If most of the developers have DCHECK enabled, then I'm confident that the current situation can get under control eventually. Once this is done, then new regression will be caught earlier and will have less time to impact everyone.

-- Sylvain

On Tue, May 18, 2021 at 3:30 PM <dan...@chromium.org> wrote:

Logging that undefined behaviour is happening doesn't seem like a great situation to be in. Honestly it sounds like maybe we should be writing CHECKs instead of DCHECKs, if we're in a situation where the behaviour of Chromium is so undefined that developers are not able to run it without encountering that constantly. A product where we can expect undefined behaviour for users is a terrible place to be IMO.

On Tue, May 18, 2021 at 3:32 AM Wez <w...@chromium.org> wrote:

We already have a flag which causes DCHECKS to be checked, but to then be reported at ERROR level - you can set |dcheck_is_configurable=true| to build with that.  It implies |dcheck_always_on|, so you can replace one with the other.

On Tue, 18 May 2021 at 06:32, 'Chris Lam' via Chromium-dev <chromi...@chromium.org> wrote:

My 2c on this.

I like DCHECKs. I write DCHECKs. I never run with DCHECKs.

It's just too unstable, and my number one priority day to day is to compile and run Chrome and develop the code I'm working on. I still use them because they're a great way to explicitly state your assumptions/preconditions, which helps other developers contextualize your code.

But regarding turning on DCHECKs locally, if we want more reporting of DCHECK violations, why don't we make day to day life easier for local DCHECK-on builds? Do we really need to crash the browser when one of these conditions is violated? Can't we have a gn flag to log DCHECK failures as a Big Red Error with a stack trace, and suggest the developer file a bug? In my opinion, the less friction there is, the more adoption there'll be.

Also, if there was a way to easily find out if a bug was already filed, that would reduce the bystander effect that I get when I run into a DCHECK failure.

On Thu, May 13, 2021 at 9:28 PM Satoru Takabayashi <sat...@chromium.org> wrote:

On Thu, May 13, 2021 at 5:28 AM 'Avi Drissman' via Chromium-dev <chromi...@chromium.org> wrote:

On Wed, May 12, 2021 at 3:59 PM Ken Rockot <roc...@google.com> wrote:

I worry that this would be a counterproductive default policy. In many cases the DCHECK is correct, and the direct owner is not necessarily responsible for all code paths which might lead to its violation.

I think this happens commonly with DCHECKs in low level code (ex. thread_restrictions.cc).

 

An interesting question would be how did this evolve. Did the direct owner add the DCHECK to an existing system? If so, then they don't fully understand it, and are directly responsible. Did a second developer add new code that caused the existing DCHECK in a different part of the codebase to fire? How did they do that without noticing the DCHECK firing? Did they have DCHECKs turned off?

I think it's the latter in many cases.

To be clear, I am not advocating for or against this policy of turning off DCHECKs. I just want to point out how the argument against this policy is indicative of the situation we're in, where a downward spiral of people turning off DCHECKs on local builds has yielded a situation where we have to consider tough policies.

Avi

--
--
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/CACWgwAYB%3DQYEawhobSq0%3Dm8BMN2zwCspO2CXJDG1HWVxpoXAkA%40mail.gmail.com.

--
--
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/CACjFnbyYpgmDr-J%2B0GUjD49%2B_%3DqTyO7ZpSmwH3HmhLZus22rvw%40mail.gmail.com.

--
--
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/CA%2Bg1BH0HO6zoLFv1Jcep1GSE_kDZGVtVAROpFc9O%2BqE%3DyLoD1A%40mail.gmail.com.

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

--
--
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/CAHtyhaSzAKs9fQQd%3DVcGtfOKXCHkRBkDRVHjtvW-vaRv92gzVQ%40mail.gmail.com.

--
--
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/CAJauWCHP91osS0D8x8Oow%3DdG%3D25zd0qaeKLEmyRG6%3D7Ajeithg%40mail.gmail.com.

--
--
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/CAE3TgXFPi34Fh9p0DUp_BrN-aO%2B-zs_jW9L%3DgYHiTxF0dWcG_w%40mail.gmail.com.

--
--
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/CAHtyhaT%3DXF754nzKJByi%3DVF9z%2BnqTTyAObwrPHG2OWf7hvx29A%40mail.gmail.com.