base::debug::BreakDebugger is bad, question mark?

[Torne (Richard Coles)]

While investigating the cause of a stack unwinding failure in crbug.com/851851, I've started to suspect that base::debug::BreakDebugger is bad and we shouldn't use it (at least not for all the purposes it's currently used), but would like some second opinions, especially from people who work on windows or mac (I'm only familiar with linux/android) or from people who frequently use an interactive debugger to debug chromium (I rarely do).

Some problems I have with it:

1) It's extremely complex: there's a ton of ifdefs that make it behave quite differently on different build configurations, as well as further runtime conditionals. This makes it hard to figure out what it's actually going to do in any given context.

2) It appears to be trying to serve two rather different purposes at once:

- enable people to add hardcoded breakpoints temporarily to their build, to make their interactive debugger stop at a given place. This kind of usage isn't checked into the tree, and isn't expected to exist in official builds.

- crash the process in a way that should generate a reasonable crash dump, but also give an attached debugger an opportunity to step in and inspect the state before actually doing so. This kind of usage is checked in and is expected to result in a hard crash in all builds.

3) A number of the implementation choices and comments appear to be outdated - e.g. it refers to breakpad not generating a dump on SIGTRAP, which doesn't appear to be true any more.

In official builds, CHECK() already uses IMMEDIATE_CRASH() instead of BreakDebugger (for several reasons, documented in comments in logging.h), and this appears to work fine for both generating reliably unwindable crash dumps on all our supported platforms, *and* for local interactive debugging, at least on Linux and Android (it uses a hardware breakpoint instruction on all our supported arches, so should reliably cause an attached debugger to break in even without the logic in BreakDebugger). To fix crbug.com/851851 I've made LOG(FATAL) *also* use IMMEDIATE_CRASH in official builds.

Would it break anyone's debugging workflow if we just replaced *all* usage of BreakDebugger in the codebase with something like IMMEDIATE_CRASH, regardless of build configuration (i.e. including on debug builds), and declared that BreakDebugger was only for the "add temporary hardcoded breakpoint during local debugging" use case?

[Primiano Tucci]

Uh I never realized that BreakDebugger() was being used in production code.

If find very hard to find arguments for having BreakDebugger() calls in production code. IMHO there should be a presubmit against that, at very least for non *test.cc files.

> this appears to work fine for both generating reliably unwindable crash dumps on all our supported platforms, *and* for local interactive debugging, at least on Linux and Android (it uses a hardware breakpoint instruction on all our supported arches, so should reliably cause an attached debugger to break in even without the logic in BreakDebugger)

The only problem I can see with local development/debugging cycles/  is that IMMEDIATE_CRASH() might not always allow to resume the execution in the same way of a breakpoint (I didn't try though). I am mostly looking at that __builtin_unreachable().

However, for the dev/dbg use case, DCHECK(false) / NOTREACHED() should do the right thing (right now they seem to call BreakDebugger()), so we seem to have good options.

Perhaps I'm missing something, but in the current state I don't see reasons for needing to call BreakDebugger(), imho we should have either CHECK(false)/LOG(FATAL)  for production cases or DCHECK(false)/NOTREACHED() for debugging cases.

(Personally, the number of times I had the need of attaching a debugger AND continuing past a NOTREACHED() or similar is close to 0, but can't speak for everybody here)

> Would it break anyone's debugging workflow if we just replaced *all* usage of BreakDebugger in the codebase with something like IMMEDIATE_CRASH, regardless of build configuration (i.e. including on debug builds), and declared that BreakDebugger was only for the "add temporary hardcoded breakpoint during local debugging" use case?

Yeah for the existing cases I think we need to be conservative and preserve the crash-in-production behavior. There seem to be only two cases though:

1) SilentRuntimeAssertHandler() in chrome_watcher_main.cc 

2) ReportThreadHang() in thread_watcher_report_hang.cc but this one is gated behind a #ifdef(NDEBUG).

Am I missing something else?

I suspect 1 might have some special treatment in the crash/ processor to extract the the message strings, by looking at that MSVC_DISABLE_OPTIMIZE(), so I'd check this one with 

+Bruce Dawson.

For 2, imho the part in the debug case could be switched to a NOTREACHED()

--
--
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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAEV-rjfWo9%3D24FeUyOLapNcvyQYXf4yyfKU7VJ93A%2B8u7mcFBQ%40mail.gmail.com.

[Torne (Richard Coles)]

Well the issue is that various of CHECK/NOTREACHED/LOG(FATAL)/etc ultimately crash by calling BreakDebugger. Those are the main cases I'd want to change.

IMMEDIATE_CRASH is explicitly intended *not* to allow you to resume execution even if you have a debugger attached, but that doesn't seem like you actually should be doing that - if you want one you can continue past, then actually putting in a temporary call to BreakDebugger is probably the right thing.

[Bruce Dawson]

I don't see any value to the disabling of optimizations in SilentRuntimeAssertHandler. If the goal is to preserve the strings then it is necessary to copy the strings (not their pointers) to a local array and then base::debug::Alias to stop the optimizer from removing the array.

I added that code but I just copied it from another file (to avoid binary bloat) and I'm not sure of the intent in the original file.

[Wez]

Bear in mind that we plan to have NOTREACHED() become [[noreturn]] (by becoming a synonym for IMMEDIATE_CRASH() in Release builds, for example), so that surrounding code doesn't need e.g. redundant returns, etc.

If we want to allow debuggers to continue past a particular breakpoint then we do need a distinct mechanism that is guaranteed not to be [[noreturn]], to ensure that there is actually code to continue into.  Retaining a BreakDebugger()-ish call for that purpose seems reasonable, but we should add it to the PRESUBMIT checks so it can't be accidentally used in production.

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

[Greg Thompson]

On Thu, Jul 26, 2018 at 3:43 PM Wez <w...@chromium.org> wrote:

Bear in mind that we plan to have NOTREACHED() become [[noreturn]] (by becoming a synonym for IMMEDIATE_CRASH() in Release builds, for example), so that surrounding code doesn't need e.g. redundant returns, etc.

If we want to allow debuggers to continue past a particular breakpoint then we do need a distinct mechanism that is guaranteed not to be [[noreturn]], to ensure that there is actually code to continue into.  Retaining a BreakDebugger()-ish call for that purpose seems reasonable, but we should add it to the PRESUBMIT checks so it can't be accidentally used in production.

Hi Wez. I ordinarily develop with a dcheck_always_on = true build. I find it very useful to be able to continue past unrelated DCHECKs during my own testing and debugging. Are you saying that this will no longer be possible -- meaning that I'd have to abandon a debugging session, edit code to remove the DCHECK, rebuild, relaunch, then try to get back to whatever I was doing in the first place? This seems like it'll be a loss from the standpoint of developer productivity. Or am I misunderstanding?

Thanks,

Greg

 

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

[Wez]

No, I'm specifically talking about NOTREACHED(), not DCHECKs, CHECKs or other conditional crashers.

[Greg Thompson]

On Thu, Jul 26, 2018 at 3:54 PM Wez <w...@chromium.org> wrote:

No, I'm specifically talking about NOTREACHED(), not DCHECKs, CHECKs or other conditional crashers.

It's the same issue, though. We use NOTREACHED() in code branches that we expect not to hit, since it's a convenient shorthand for DCHECK(false).

[Wez]

DCHECK(false) is Debug-only, so it is inherently the case that it cannot be [[noreturn]] - there must be code following the DCHECK that would be run in a non-DCHECK build.

NOTREACHED() is conceptually different in that NOTREACHED() indicates that the codepath should literally be unreachable (e.g. it's a virtual method that we have to implement, but which will never be called because of some other property of our implementation), and continuing to try to execute past it is meaningless. In that sense, NOTREACHED() is conceptually closer to IMMEDIATE_CRASH(), which part of why we're proposing to change it to be defined that way in Release.  Right now NOTREACHED() is fatal only in DCHECK builds, equivalent to DCHECK, though, you're right.

Please don't use DHCECK(false) in branches you don't expect to hit; DCHECK-but-also-handle is explicitly called out as an anti-pattern in the Chromium style guide. :)

[Primiano Tucci]

Ah Torne, I think I am understanding your proposal fully only now. Lemme try to recap are, you proposing that:

in release / official builds: no change from today.

   CHECK -> IMMEDIATE_CRASH()

   DCHECK/NOTREACHED: does nothing (% future plans wez mentioned)

in debug:

   D/CHECK/NOTREACHED: WRAPPED_TRAP_SEQUENCE (that is, IMMEDIATE_CRASH without the __builtin_unreachable)

   instead of calling BreakDebugger?

I think the main question for anybody here that knows debuggers enough: are those trap sequences resumable ?

The arm / arm64 IIRC are. Not sure what happens on Linux/Mac/Win on x86_64 with that int3+ud2. Can anybody try that?

The main difference from today's BreakDebugger() call on x86_64 seems to be the extra "ud2".

If ud2 is resumable as well, that should keep everybody happy and we could get rid of BreakDebugger altogether.

> I don't see any value to the disabling of optimizations in SilentRuntimeAssertHandler.

The question was not about removing the optimizations around SilentRuntimeAssertHandler, was about replacing the inner BreakDebugger() call with IMMEDIATE_CRASH() / CHECK(false)

[Greg Thompson]

Historically, NOTREACHED() was implemented as DCHECK(false), so it, too, was debug-only. If I'm looking at the right thing, it still is today on non-Chrome OS platforms. I think it has commonly been used with the assumption that it's a debug-only crash. Now, I'm not trying to say that that's the way it *should* be, but I thought that's the way it was. Has this changed? A quick codesearch tells me that NOTREACHED(); is (still) commonly used as "oops, I didn't think we would ever get here", rather than "it is not possible to get here."

Are we somehow talking about two different things? :-)

[Will Harris]

There's some prior discussion on this (for Windows, at least) on this CL:

https://codereview.chromium.org/2676483002/

At least, we want to be able to distinguish CHECK() from an access violation in the crash exit codes. This is one of the primary metrics we use for monitoring baseline stability of child processes. If we lost this distinction on release/production builds, it would make triaging operational stability issues, as it's a very useful primary signal from UMA.

Regarding the separate discussion of whether it should be possible to continue/debug after a CHECK() vs a base::debug::BreakDebugger vs an IMMEDIATE_CRASH() - I think the only thing that matters here is whether the function has been marked [[noreturn]] because, if so, it's likely there is no way to jump back to caller (as it might have been made with a JMP rather than CALL).  I think otherwise, on Windows at least, it's trivial to step over the instructions or handle the exception in the debugger from any of these types of assertions - the difference being just how long the windbg command is to step over it.

Will

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

[Mark Mentovai]

ud2 doesn’t advance the program counter, so you can’t continue beyond it.

You can manually increment %rip (or %eip) by 2 (ud2 is a 2-byte opcode) and resume, but I don’t think that’s what anyone has in mind.

int3 (and int 3) do advance the program counter and thus are resumable. When they trap, you’ll find in a debugger that the instruction pointer is already one instruction beyond the interrupt.

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

[Torne (Richard Coles)]

OK, so there's a lot of specific points here I'll need to read and re-read in detail but it sounds like in general there are some folks who would be sad if DCHECK and friends weren't something you could step over in a local build.

I'll look at a way to continue adapting the various interesting callsites here in a way similar to how CHECK() currently works (and how LOG(ERROR) works with my recent change) such that in official builds it is an immediate crash that has the properties we want for crash dumps, but still works more or less how BreakDebugger has worked in the past for non-official, I think - and look at ways to make sure this doesn't regress in future.

[Marshall Greenblatt]

On Thu, Jul 26, 2018 at 1:13 PM, Torne (Richard Coles) <to...@chromium.org> wrote:

OK, so there's a lot of specific points here I'll need to read and re-read in detail but it sounds like in general there are some folks who would be sad if DCHECK and friends weren't something you could step over in a local build.

I'll look at a way to continue adapting the various interesting callsites here in a way similar to how CHECK() currently works (and how LOG(ERROR) works with my recent change)

Do you mean LOG(FATAL) here?

 

such that in official builds it is an immediate crash that has the properties we want for crash dumps, but still works more or less how BreakDebugger has worked in the past for non-official, I think - and look at ways to make sure this doesn't regress in future.

CEF uses "is_official_build=true is_debug=false dcheck_always_on=true" for "Debug" builds that we distribute to embedders for local development purposes. It would be preferable if [D]CHECK() and LOG(FATAL) in these builds allowed continuation past the assertion when a debugger is attached.

The "is_official_build=true is_debug=false dcheck_always_on=false" ("Release") builds, on the other hand, should crash in the way that's best for generating crash dumps.

 

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

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

[Torne (Richard Coles)]

I mean LOG(FATAL), sorry. :)

You should not expect to be able to use is_official_build=true for development purposes - do you have a specific reason why you need to do that? It has already been the case for over a year that CHECK() in official builds is implemented as IMMEDIATE_CRASH and cannot be continued past in a debugger, so if nobody has noticed/complained about that already, then I suspect you won't be affected by the changes I want to make either :)

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

[Will Harris]

[apologies for resend]

>It has already been the case for over a year that CHECK() in official builds is implemented as IMMEDIATE_CRASH and cannot be continued past in a debugger,

This doesn't appear to be the case for me.

Visiting chrome://checkcrash, which calls directly into CHECK(false) [source] on Chrome Stable 68.0.3440.75 (Official Build) (64-bit) (cohort: 68_75_win) gives me crash which is hitting INT 3

0:000> u

chrome_child!content::HandleChromeDebugURL+0x484 [C:\b\c\b\win64_clang\src\content\renderer\render_frame_impl.cc @ 934]:

00007ffc`aadb578f cc              int     3

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

[Torne (Richard Coles)]

It is the case. The next instruction after that "int 3" is "ud2", which generates an illegal instruction abort, and then the next thing is __builtin_unreachable() to tell the compiler that proceeding further is impossible.

So, if you step over the int3, you'll take another exception from the ud2, and if you step over that then Anything Can Happen beacuse the generated code is not guaranteed to have *anything* after that - in many cases the next bytes in the binary are not even code, as these IMMEDIATE_CRASH sequences are often moved to the end of a function by the compiler and the following bytes may be the function's constant pool.

On Thu, 26 Jul 2018 at 13:51 Will Harris <w...@google.com> wrote:

>It has already been the case for over a year that CHECK() in official builds is implemented as IMMEDIATE_CRASH and cannot be continued past in a debugger,

This doesn't appear to be the case for me.

Visiting chrome://checkcrash, which calls directly into CHECK(false) [source] on Chrome Stable 68.0.3440.75 (Official Build) (64-bit) (cohort: 68_75_win) gives me crash which is hitting INT 3

0:000> u

chrome_child!content::HandleChromeDebugURL+0x484 [C:\b\c\b\win64_clang\src\content\renderer\render_frame_impl.cc @ 934]:

00007ffc`aadb578f cc              int     3

Will

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

[Torne (Richard Coles)]

If you read the comments on IMMEDIATE_CRASH you will see the various goals it's trying to meet: it is trying to make sure an attached debugger stops here, but it's explicitly also trying to make sure that you can't accidentally tell that debugger to step over the crash and continue running, which is why the various implementations generally produce first SIGTRAP and *then* SIGILL.

[w...@chromium.org]

my apologies - you are, of course, correct here :)

0:000> u

chrome_child!content::HandleChromeDebugURL+0x484 [C:\b\c\b\win64_clang\src\content\renderer\render_frame_impl.cc @ 934]:

00007ffc`aadb578f cc              int     3

00007ffc`aadb5790 0f0b            ud2

00007ffc`aadb5792 6a32            push    32h

00007ffc`aadb5794 0f0b            ud2

As long as CHECK can be distinguished in the crash exit codes from a non-CHECK crash (which it currently can be, because it does the int 3 before the ud2) then me with my "stability hat" on, is happy.

With my "developer hat" on - I never add my own breakpoints using base::debug::BreakDebugger() and always use __debugbreak() but I appreciate this is Windows only... (although now I've checked, that's exactly what base::debug::BreakDebugger() does, so maybe I am just saving typing here!

I also don't mind whether I can step over CHECK, DCHECK, IMMEDIATE_CRASH or not, because I think if I'm hitting one of those, I probably want to fix whatever is causing it rather than step over it. But I hear several on the team do wish to be able to step over these in development builds... (I've never had a scenario when I want to, I'd be curious of examples).

Will

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

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAEV-rjd0gzsmAaFK00tVKZepc2YCxWKxqFQWJbDyKXKxRyhdcg%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.

[Torne (Richard Coles)]

Yeah, I am confident that IMMEDIATE_CRASH currently generates sane, parseable crash dumps for all our supported platforms which can be reliably distinguished from other types of crash - those are some of the properties it was designed to have that are documented in its comments :)

The only property it doesn't have that it seems like anyone cares about is being able to continue past it in a debugger, but I don't think we actually want that property in official builds, because having that is likely to cause the code to be larger and slower - the __builtin_unreachable() creates compiler optimisation opportunities.

So, I'll think about exactly what I want to do and propose a CL, but the general summary is "I'll try to leave things roughly how they are for non-official builds, but make sure that all intentional crashes in official builds use something like IMMEDIATE_CRASH instead of BreakDebugger". I may also try to clean up BreakDebugger itself to make it less confusing :)

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/CAEV-rjd0gzsmAaFK00tVKZepc2YCxWKxqFQWJbDyKXKxRyhdcg%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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAEV-rjetm40M%2Bh3eRga8eV1Fzen6v9oaKqGMsybf%3Dz4Pp3KKdA%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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/4a10db07-963a-452e-9bea-44d413ffde10%40chromium.org.

[Will Harris]

of course... V8_IMMEDIATE_CRASH does something different... but that's probably outside the scope of this thread :) 😈

Will

[Marshall Greenblatt]

On Thu, Jul 26, 2018 at 1:34 PM, Torne (Richard Coles) <to...@chromium.org> wrote:

I mean LOG(FATAL), sorry. :)

You should not expect to be able to use is_official_build=true for development purposes - do you have a specific reason why you need to do that?

CEF distributes Release and Debug binaries to embedders. We've found a few things from past experience:

• Setting "is_official_build=true" for Release builds and "is_official_build=false" for Debug builds would result in unexpected behavior differences. Most of these were either bugs or confusion (https://crbug.com/574126), but we prefer to avoid them in our distributions.
• Using "is_official_build=true is_debug=false dcheck_always_on=true" gives us most of the same debugging advantages as "is_official_build=true is_debug=true" but without the significant binary size and performance penalties of an actual is_debug=true build. 

It has already been the case for over a year that CHECK() in official builds is implemented as IMMEDIATE_CRASH and cannot be continued past in a debugger, so if nobody has noticed/complained about that already, then I suspect you won't be affected by the changes I want to make either :)

In that case maybe it's fine :)

In my experience there's a pretty substantial quality/quantity difference between CHECK() and DCHECK() usage in some parts of the Chromium code. For example, I regularly experience DCHECK()s in Blink code that are actually survivable (and most have unresolved bugs). However, I usually also have a local non-official build so having DCHECK()s in official builds IMMEDIATE_CRASH for me is not a big deal personally.

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

[Torne (Richard Coles)]

On Thu, 26 Jul 2018 at 14:13 Marshall Greenblatt <magree...@gmail.com> wrote:

On Thu, Jul 26, 2018 at 1:34 PM, Torne (Richard Coles) <to...@chromium.org> wrote:

I mean LOG(FATAL), sorry. :)

You should not expect to be able to use is_official_build=true for development purposes - do you have a specific reason why you need to do that?

CEF distributes Release and Debug binaries to embedders. We've found a few things from past experience:

• Setting "is_official_build=true" for Release builds and "is_official_build=false" for Debug builds would result in unexpected behavior differences. Most of these were either bugs or confusion (https://crbug.com/574126), but we prefer to avoid them in our distributions.

We have three kinds of build: debug, release, and official. You should think of official builds as a different thing altogether, not as a separate flag - they're only implemented as two separate booleans for historical reasons at this point (you cannot have an official debug build, there's an assert banning it). Debug and release builds are both for development, and official builds are for distribution.

If you expect to use them differently then you're going to run into unexpected things, because that's not how the project uses them.

 

• Using "is_official_build=true is_debug=false dcheck_always_on=true" gives us most of the same debugging advantages as "is_official_build=true is_debug=true" but without the significant binary size and performance penalties of an actual is_debug=true build. 

I'm not sure what you mean by "debugging advantages" here. The only thing you are doing there that's different to an official build is enabling DCHECKs, which seems like it has no effect on how easy debugging is.

 

It has already been the case for over a year that CHECK() in official builds is implemented as IMMEDIATE_CRASH and cannot be continued past in a debugger, so if nobody has noticed/complained about that already, then I suspect you won't be affected by the changes I want to make either :)

In that case maybe it's fine :) 

In my experience there's a pretty substantial quality/quantity difference between CHECK() and DCHECK() usage in some parts of the Chromium code. For example, I regularly experience DCHECK()s in Blink code that are actually survivable (and most have unresolved bugs).

Right, and that's one of the reasons why we don't ship builds with DCHECKs enabled, because they trigger sometimes in cases where nothing is actually wrong due to bugs :)

 

owever, I usually also have a local non-official build so having DCHECK()s in official builds IMMEDIATE_CRASH for me is not a big deal personally.

Well, we don't enable DCHECKs in any official builds, so in some regards what happens there isn't relevant, but what I'm aiming for is having all the places in Chrome that crash intentionally crash in a consistent way (even though that way may be different in different build configurations, I don't want it to be different between different code paths in the same build configuration), so I'd rather not intentionally leave DCHECK out.

 

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

[Marshall Greenblatt]

On Thu, Jul 26, 2018 at 2:25 PM, Torne (Richard Coles) <to...@chromium.org> wrote:

On Thu, 26 Jul 2018 at 14:13 Marshall Greenblatt <magree...@gmail.com> wrote:

On Thu, Jul 26, 2018 at 1:34 PM, Torne (Richard Coles) <to...@chromium.org> wrote:

I mean LOG(FATAL), sorry. :)

You should not expect to be able to use is_official_build=true for development purposes - do you have a specific reason why you need to do that?

CEF distributes Release and Debug binaries to embedders. We've found a few things from past experience:

• Setting "is_official_build=true" for Release builds and "is_official_build=false" for Debug builds would result in unexpected behavior differences. Most of these were either bugs or confusion (https://crbug.com/574126), but we prefer to avoid them in our distributions.

We have three kinds of build: debug, release, and official. You should think of official builds as a different thing altogether, not as a separate flag - they're only implemented as two separate booleans for historical reasons at this point (you cannot have an official debug build, there's an assert banning it). Debug and release builds are both for development, and official builds are for distribution.

Thanks for the reminder, I forgot that is_official_build=true + is_debug=false is disallowed. We want the Debug and Release builds that we distribute to be functionally identical with the exception that DCHECK()s are enabled in the Debug build. Otherwise it's really hard to reason about why something works in a Release build that doesn't work in a Debug build, or visa-versa.

 

If you expect to use them differently then you're going to run into unexpected things, because that's not how the project uses them. 

 

• Using "is_official_build=true is_debug=false dcheck_always_on=true" gives us most of the same debugging advantages as "is_official_build=true is_debug=true" but without the significant binary size and performance penalties of an actual is_debug=true build. 

I'm not sure what you mean by "debugging advantages" here. The only thing you are doing there that's different to an official build is enabling DCHECKs, which seems like it has no effect on how easy debugging is.

By "debugging advantages" I mean: Hitting a DCHECK() (which hopefully acts as inline documentation of code expectations) should generally be more useful for debugging purposes than having the application behave or crash mysteriously later on. This is useful when you consider that (a) the majority of CEF embedders will probably file a bug report instead of trying to debug the Chromium code themselves, and (b) we don't have access to crash reports/statistics for most applications using CEF.

 

 

It has already been the case for over a year that CHECK() in official builds is implemented as IMMEDIATE_CRASH and cannot be continued past in a debugger, so if nobody has noticed/complained about that already, then I suspect you won't be affected by the changes I want to make either :)

In that case maybe it's fine :) 

In my experience there's a pretty substantial quality/quantity difference between CHECK() and DCHECK() usage in some parts of the Chromium code. For example, I regularly experience DCHECK()s in Blink code that are actually survivable (and most have unresolved bugs).

Right, and that's one of the reasons why we don't ship builds with DCHECKs enabled, because they trigger sometimes in cases where nothing is actually wrong due to bugs :)

 

owever, I usually also have a local non-official build so having DCHECK()s in official builds IMMEDIATE_CRASH for me is not a big deal personally.

Well, we don't enable DCHECKs in any official builds, so in some regards what happens there isn't relevant, but what I'm aiming for is having all the places in Chrome that crash intentionally crash in a consistent way (even though that way may be different in different build configurations, I don't want it to be different between different code paths in the same build configuration), so I'd rather not intentionally leave DCHECK out.

This sounds reasonable to me. Thanks for explaining.

 

 

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

[Greg Thompson]

Perhaps my concern about being able to continue after hitting an unrelated NOTREACHED isn't such a big deal. I think it's more common for me to hit an unrelated DCHECK, and it's not something that happens multiple times a day.

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

[Wez]

Lots of emails have passed since I commented (yay, timezones!) so some possibly random-looking points:

1. We have fewer bogus DCHECKs than we used to, because we have a permanent experiment generating DCHECK-enabled (aka "Albatross") builds, which have |dcheck_is_configurable| set. These binaries build-in DCHECKs and then switch the LogSeverity between LOG_ERROR and LOG_FATAL at run-time.

2. Having CHECK(false)-ish macros be [[noreturn]] (or use __builtin_unreachable()) in one configuration but not in others becomes a source of pain when we enable unreachable code warnings, since now you have code after the CHECK(false)-ish thing that is unreachable in Official, but reachable in Debug.  We don't have unreachable code warnings currently enabled, for precisely this reason, but Clang _will_ already warn if a FALLTHROUGH is unreachable.

3. NOTREACHED() is currently equivalent to DCHECK(false) (except when it's not, on ChromeOS...) but we plan to make it roughly equivalent to IMMEDIATE_CRASH() instead, so that it can be [[noreturn]], and remove the need for a load of redundant returns, FALLTHROUGHs, etc.  We can't do that unless NOTREACHED() is [[noreturn]] in _all_ configurations (i.e. Release and Official, as well as Debug).

#2 is probably most relevant to your immediate goal, Torne, in that I'm suggesting that we do our best to avoid using [[noreturn]] in some build types and not others, but as per #3, please bear in mind that NOTREACHED() will become conceptually closer to CHECK than to DCHECK in future :)