Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(180)

Issue 2676483002: Make CHECK int 3 on Windows, rather than crash (Closed)

Created:
3 years, 10 months ago by scottmg
Modified:
3 years, 10 months ago
CC:
chromium-reviews, vmpstr+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Make CHECK int 3 on Windows, rather than crash Not being able to distinguish intentional crashes (CHECK) from unintentional ones (other access violations) in terms of exit codes reduces the amount of signal we have in stability metrics. R=wfh@chromium.org BUG=664209, 687326 Review-Url: https://codereview.chromium.org/2676483002 Cr-Commit-Position: refs/heads/master@{#450809} Committed: https://chromium.googlesource.com/chromium/src/+/a17c8db528cadca9eef98ce03b0910700735722e

Patch Set 1 #

Total comments: 8

Patch Set 2 : . #

Total comments: 8

Patch Set 3 : fixes #

Total comments: 2

Patch Set 4 : no need to special case #if for clang #

Unified diffs Side-by-side diffs Delta from patch set Stats (+62 lines, -2 lines) Patch
M base/logging.h View 1 2 3 1 chunk +4 lines, -2 lines 0 comments Download
M base/logging_unittest.cc View 1 2 2 chunks +58 lines, -0 lines 0 comments Download

Messages

Total messages: 69 (23 generated)
scottmg
3 years, 10 months ago (2017-02-02 01:01:53 UTC) #4
scottmg
3 years, 10 months ago (2017-02-02 01:02:09 UTC) #6
Will Harris
what does LOG_STREAM(FATAL) do? is this eventually calling the same code? https://codereview.chromium.org/2676483002/diff/1/base/logging.h File base/logging.h (right): ...
3 years, 10 months ago (2017-02-02 01:21:06 UTC) #8
scottmg
On 2017/02/02 01:21:06, Will Harris wrote: > what does LOG_STREAM(FATAL) do? is this eventually calling ...
3 years, 10 months ago (2017-02-02 01:33:37 UTC) #9
scottmg
https://codereview.chromium.org/2676483002/diff/1/base/logging.h File base/logging.h (right): https://codereview.chromium.org/2676483002/diff/1/base/logging.h#newcode465 base/logging.h:465: #define IMMEDIATE_CRASH() __builtin_trap() On 2017/02/02 01:21:06, Will Harris wrote: ...
3 years, 10 months ago (2017-02-02 01:41:24 UTC) #12
Will Harris
lgtm but sounds like you need a base owner, I recommend thakis :) https://codereview.chromium.org/2676483002/diff/20001/base/logging.h File ...
3 years, 10 months ago (2017-02-02 03:12:03 UTC) #13
Will Harris
never gonna give you up https://codereview.chromium.org/2676483002/diff/20001/base/logging_unittest.cc File base/logging_unittest.cc (right): https://codereview.chromium.org/2676483002/diff/20001/base/logging_unittest.cc#newcode204 base/logging_unittest.cc:204: CHECK(!y); One final thought ...
3 years, 10 months ago (2017-02-02 03:18:14 UTC) #14
Primiano Tucci (use gerrit)
non-owner LGTM. maybe you can expand a bit the commit message though (copy/paste and/or point ...
3 years, 10 months ago (2017-02-03 03:15:05 UTC) #17
scottmg
Thanks. https://codereview.chromium.org/2676483002/diff/20001/base/logging.h File base/logging.h (right): https://codereview.chromium.org/2676483002/diff/20001/base/logging.h#newcode467 base/logging.h:467: #define IMMEDIATE_CRASH() __debugbreak() On 2017/02/02 03:12:03, Will Harris ...
3 years, 10 months ago (2017-02-06 19:50:47 UTC) #18
scottmg
Nico, could you review as base OWNERS? Thanks.
3 years, 10 months ago (2017-02-06 19:51:34 UTC) #20
Will Harris
the test is gated on OFFICIAL_BUILD so won't run on the trybots, so I presume ...
3 years, 10 months ago (2017-02-06 19:58:51 UTC) #21
Nico
int 3 seems wrong to me in that it's not noreturn, you can just step ...
3 years, 10 months ago (2017-02-06 19:59:12 UTC) #22
Primiano Tucci (use gerrit)
On 2017/02/06 19:59:12, Nico wrote: > int 3 seems wrong to me in that it's ...
3 years, 10 months ago (2017-02-06 20:05:49 UTC) #23
scottmg
On 2017/02/06 19:59:12, Nico wrote: > int 3 seems wrong to me in that it's ...
3 years, 10 months ago (2017-02-06 20:14:10 UTC) #24
Mark Mentovai
I actually think that abort() is the right way to do this on non-Windows, and ...
3 years, 10 months ago (2017-02-10 16:10:20 UTC) #25
Primiano Tucci (use gerrit)
On 2017/02/10 16:10:20, Mark Mentovai wrote: > I actually think that abort() is the right ...
3 years, 10 months ago (2017-02-10 16:58:09 UTC) #26
Will Harris
I wonder if thakis@ had any further thoughts on this CL?
3 years, 10 months ago (2017-02-10 17:16:10 UTC) #27
Nico
I didn't know that I can stop across 0 pointer derefs -- and I can't ...
3 years, 10 months ago (2017-02-10 22:45:30 UTC) #28
scottmg
On 2017/02/10 22:45:30, Nico wrote: > I didn't know that I can stop across 0 ...
3 years, 10 months ago (2017-02-10 22:59:33 UTC) #29
Nico
On Fri, Feb 10, 2017 at 5:59 PM, <scottmg@chromium.org> wrote: > On 2017/02/10 22:45:30, Nico ...
3 years, 10 months ago (2017-02-10 23:02:06 UTC) #30
Will Harris
On 2017/02/10 23:02:06, Nico wrote: > On Fri, Feb 10, 2017 at 5:59 PM, <mailto:scottmg@chromium.org> ...
3 years, 10 months ago (2017-02-10 23:03:56 UTC) #31
scottmg
Wait, I just realized we used to do BreakDebugger() which is implemented as __debugbreak(). So ...
3 years, 10 months ago (2017-02-10 23:30:10 UTC) #32
Mark Mentovai
DebugBreak() is in kernel32, and __debugbreak() gives you your own int3?
3 years, 10 months ago (2017-02-11 00:15:23 UTC) #35
scottmg
On 2017/02/11 00:15:23, Mark Mentovai wrote: > DebugBreak() is in kernel32, and __debugbreak() gives you ...
3 years, 10 months ago (2017-02-11 00:17:16 UTC) #37
Mark Mentovai
Oh, but you said BreakDebugger()… Yeah…so that’s still a call (which is a few bytes), ...
3 years, 10 months ago (2017-02-11 00:18:50 UTC) #38
scottmg
On 2017/02/11 00:17:16, scottmg wrote: > On 2017/02/11 00:15:23, Mark Mentovai wrote: > > DebugBreak() ...
3 years, 10 months ago (2017-02-11 00:19:38 UTC) #39
scottmg
On 2017/02/11 00:18:50, Mark Mentovai wrote: > Oh, but you said BreakDebugger()… > > Yeah…so ...
3 years, 10 months ago (2017-02-11 00:24:49 UTC) #40
Will Harris
I definitely think we should stick with __debugbreak() not kernel32!DebugBreak() I'm not sure exactly what ...
3 years, 10 months ago (2017-02-11 04:26:59 UTC) #41
scottmg
https://codereview.chromium.org/2676483002/diff/40001/base/logging.h File base/logging.h (left): https://codereview.chromium.org/2676483002/diff/40001/base/logging.h#oldcode464 base/logging.h:464: #if defined(COMPILER_GCC) || defined(__clang__) On 2017/02/10 22:45:30, Nico wrote: ...
3 years, 10 months ago (2017-02-11 05:58:30 UTC) #44
Nico
how bad would it be for codesize if we did `int 3, mov 0 [0]`?
3 years, 10 months ago (2017-02-12 18:55:44 UTC) #47
Mark Mentovai
int3 ud2 would be better.
3 years, 10 months ago (2017-02-12 20:00:18 UTC) #49
Nico
Better than current patch set or better than what I said? (It's what my suggestion ...
3 years, 10 months ago (2017-02-12 20:22:34 UTC) #51
Mark Mentovai
Nico wrote: > Better than current patch set or better than what I said? (It's ...
3 years, 10 months ago (2017-02-12 21:11:00 UTC) #52
Will Harris
Hi I wonder if there was any progress on reviewing this CL? We are still ...
3 years, 10 months ago (2017-02-13 18:38:46 UTC) #53
Will Harris
Is this currently blocked on the question in #47? are you suggesting: #define IMMEDIATE_CRASH() {__debugbreak(); ...
3 years, 10 months ago (2017-02-14 00:21:59 UTC) #54
Nico
On 2017/02/14 00:21:59, Will Harris wrote: > Is this currently blocked on the question in ...
3 years, 10 months ago (2017-02-14 18:54:22 UTC) #55
Nico
FWIW if this is time-sensitive for some reason (wfh seems a bit agitated :-) ), ...
3 years, 10 months ago (2017-02-14 20:04:08 UTC) #56
scottmg
On 2017/02/14 18:54:22, Nico wrote: > On 2017/02/14 00:21:59, Will Harris wrote: > > Is ...
3 years, 10 months ago (2017-02-14 23:12:35 UTC) #57
scottmg
On 2017/02/14 23:12:35, scottmg wrote: > On 2017/02/14 18:54:22, Nico wrote: > > On 2017/02/14 ...
3 years, 10 months ago (2017-02-15 17:29:55 UTC) #58
Primiano Tucci (use gerrit)
On 2017/02/14 23:12:35, scottmg > The secondary question is how important consistency is across platforms. ...
3 years, 10 months ago (2017-02-15 17:31:33 UTC) #59
Nico
On 2017/02/14 23:12:35, scottmg wrote: > On 2017/02/14 18:54:22, Nico wrote: > > On 2017/02/14 ...
3 years, 10 months ago (2017-02-15 17:42:04 UTC) #60
Nico
(but as said, landing as-is for now sounds fine to me, as does reverting my ...
3 years, 10 months ago (2017-02-15 17:42:46 UTC) #61
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2676483002/60001
3 years, 10 months ago (2017-02-15 17:58:48 UTC) #64
scottmg
On 2017/02/15 17:42:04, Nico wrote: > On 2017/02/14 23:12:35, scottmg wrote: > > On 2017/02/14 ...
3 years, 10 months ago (2017-02-15 18:01:46 UTC) #65
Primiano Tucci (use gerrit)
On 2017/02/15 18:01:46, scottmg wrote: > > It depends a bit on how things map ...
3 years, 10 months ago (2017-02-15 18:22:57 UTC) #66
commit-bot: I haz the power
3 years, 10 months ago (2017-02-15 21:36:36 UTC) #69
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/a17c8db528cadca9eef98ce03b09...

Powered by Google App Engine
This is Rietveld 408576698