Don't call exit() after __debugbreak() on release builds.

Don't call exit() after __debugbreak() on release builds.

This CL optimizes code for debugging as opposed to for someone who attaches a
debugger and goes beyond a CHECK without realizing it. Debugging a release
build is already hard, especially on 64-bit builds.

BUG=408820
R=wfh@chromium.org

Committed: https://crrev.com/9e9ce3c56b155d66c4ee324edd19db6fe35fb876
Cr-Commit-Position: refs/heads/master@{#306871}

[Will Harris, 2014-11-27 03:10:24]

On 2014/11/27 02:10:19, rvargas wrote:

CHECK should always terminate the process, is there a risk here that if a
debugger is configured to attach and continue, a CHECK gets ignored in release?

[rvargas (doing something else), 2014-12-01 19:00:58]

On 2014/11/27 03:10:24, Will Harris OOO until 1 Dec wrote:
> On 2014/11/27 02:10:19, rvargas wrote:
> 
> CHECK should always terminate the process, is there a risk here that if a
> debugger is configured to attach and continue, a CHECK gets ignored in
release?

If a debugger is configured to attach, then the process is presumably being
actively debugged, in which case, why do we want to override what the person
debugging the process is doing? The whole point of the change is to let
developers debug Chrome.

[Will Harris, 2014-12-01 20:51:35]

On 2014/12/01 19:00:58, rvargas wrote:
> On 2014/11/27 03:10:24, Will Harris OOO until 1 Dec wrote:
> > On 2014/11/27 02:10:19, rvargas wrote:
> > 
> > CHECK should always terminate the process, is there a risk here that if a
> > debugger is configured to attach and continue, a CHECK gets ignored in
> release?
> 
> If a debugger is configured to attach, then the process is presumably being
> actively debugged, in which case, why do we want to override what the person
> debugging the process is doing? The whole point of the change is to let
> developers debug Chrome.

I suppose I'm more thinking of debuggers that are configured as post mortem e.g.
dr watson, that might attach on a debugbreak but then be configured to resume
execution afterwards.  I don't know the prevalence of these debuggers.  I worry
that removing the exit might have security implications if we are relying on a
CHECK to always terminate process (e.g. ASSERT_WITH_SECURITY_IMPLICATION).

[rvargas (doing something else), 2014-12-01 22:18:03]

On 2014/12/01 20:51:35, Will Harris OOO until 1 Dec wrote:
> On 2014/12/01 19:00:58, rvargas wrote:
> > On 2014/11/27 03:10:24, Will Harris OOO until 1 Dec wrote:
> > > On 2014/11/27 02:10:19, rvargas wrote:
> > > 
> > > CHECK should always terminate the process, is there a risk here that if a
> > > debugger is configured to attach and continue, a CHECK gets ignored in
> > release?
> > 
> > If a debugger is configured to attach, then the process is presumably being
> > actively debugged, in which case, why do we want to override what the person
> > debugging the process is doing? The whole point of the change is to let
> > developers debug Chrome.
> 
> I suppose I'm more thinking of debuggers that are configured as post mortem
e.g.
> dr watson, that might attach on a debugbreak but then be configured to resume
> execution afterwards.  I don't know the prevalence of these debuggers.  I
worry
> that removing the exit might have security implications if we are relying on a
> CHECK to always terminate process (e.g. ASSERT_WITH_SECURITY_IMPLICATION).

I don't believe we had any issues with this behavior when it was in effect (for
a few years).

I don't know if it is possible to configure Dr Watson to continue execution of a
process, given that it is not a debugger but a post-mortem reporter (so the
application is crashing and it is a mistake to just ignore exceptions). I'll
look into that anyway.

On the other hand, if a user configures a real debugger to attach on exceptions,
ignore the exception and continue execution, that is not a user we should try to
cover with our policies. That qualifies as a developer in my book (anyone who
configures a real debugger to attach on exceptions), and our policy should be to
make their job easier as long as it doesn't put real users in danger.

[Will Harris, 2014-12-01 22:28:48]

On 2014/12/01 22:18:03, rvargas wrote:
> On 2014/12/01 20:51:35, Will Harris OOO until 1 Dec wrote:
> > On 2014/12/01 19:00:58, rvargas wrote:
> > > On 2014/11/27 03:10:24, Will Harris OOO until 1 Dec wrote:
> > > > On 2014/11/27 02:10:19, rvargas wrote:
> > > > 
> > > > CHECK should always terminate the process, is there a risk here that if
a
> > > > debugger is configured to attach and continue, a CHECK gets ignored in
> > > release?
> > > 
> > > If a debugger is configured to attach, then the process is presumably
being
> > > actively debugged, in which case, why do we want to override what the
person
> > > debugging the process is doing? The whole point of the change is to let
> > > developers debug Chrome.
> > 
> > I suppose I'm more thinking of debuggers that are configured as post mortem
> e.g.
> > dr watson, that might attach on a debugbreak but then be configured to
resume
> > execution afterwards.  I don't know the prevalence of these debuggers.  I
> worry
> > that removing the exit might have security implications if we are relying on
a
> > CHECK to always terminate process (e.g. ASSERT_WITH_SECURITY_IMPLICATION).
> 
> I don't believe we had any issues with this behavior when it was in effect
(for
> a few years).
> 
> I don't know if it is possible to configure Dr Watson to continue execution of
a
> process, given that it is not a debugger but a post-mortem reporter (so the
> application is crashing and it is a mistake to just ignore exceptions). I'll
> look into that anyway.
> 
> On the other hand, if a user configures a real debugger to attach on
exceptions,
> ignore the exception and continue execution, that is not a user we should try
to
> cover with our policies. That qualifies as a developer in my book (anyone who
> configures a real debugger to attach on exceptions), and our policy should be
to
> make their job easier as long as it doesn't put real users in danger.

I'm wary of removing this exit.  I re-read the bug and can't understand why it's
not possible the developer to just look at the stack frames to get locals - I
don't think we should be letting even developers skip over a CHECK as the state
of execution might be in a bad way after a CHECK.

[rvargas (doing something else), 2014-12-01 22:54:32]

On 2014/12/01 22:28:48, Will Harris OOO until 1 Dec wrote:
> On 2014/12/01 22:18:03, rvargas wrote:
> > On 2014/12/01 20:51:35, Will Harris OOO until 1 Dec wrote:
> > > On 2014/12/01 19:00:58, rvargas wrote:
> > > > On 2014/11/27 03:10:24, Will Harris OOO until 1 Dec wrote:
> > > > > On 2014/11/27 02:10:19, rvargas wrote:
> > > > > 
> > > > > CHECK should always terminate the process, is there a risk here that
if
> a
> > > > > debugger is configured to attach and continue, a CHECK gets ignored in
> > > > release?
> > > > 
> > > > If a debugger is configured to attach, then the process is presumably
> being
> > > > actively debugged, in which case, why do we want to override what the
> person
> > > > debugging the process is doing? The whole point of the change is to let
> > > > developers debug Chrome.
> > > 
> > > I suppose I'm more thinking of debuggers that are configured as post
mortem
> > e.g.
> > > dr watson, that might attach on a debugbreak but then be configured to
> resume
> > > execution afterwards.  I don't know the prevalence of these debuggers.  I
> > worry
> > > that removing the exit might have security implications if we are relying
on
> a
> > > CHECK to always terminate process (e.g. ASSERT_WITH_SECURITY_IMPLICATION).
> > 
> > I don't believe we had any issues with this behavior when it was in effect
> (for
> > a few years).
> > 
> > I don't know if it is possible to configure Dr Watson to continue execution
of
> a
> > process, given that it is not a debugger but a post-mortem reporter (so the
> > application is crashing and it is a mistake to just ignore exceptions). I'll
> > look into that anyway.
> > 
> > On the other hand, if a user configures a real debugger to attach on
> exceptions,
> > ignore the exception and continue execution, that is not a user we should
try
> to
> > cover with our policies. That qualifies as a developer in my book (anyone
who
> > configures a real debugger to attach on exceptions), and our policy should
be
> to
> > make their job easier as long as it doesn't put real users in danger.
> 
> I'm wary of removing this exit.  I re-read the bug and can't understand why
it's
> not possible the developer to just look at the stack frames to get locals - I

Remember that the 64-bit ABI makes it so that many arguments are now being
passed through registers so the simplest operation (show the arguments by simple
inspection of the stack) doesn't work anymore. Sure, they _could_ be grabbed
from somewhere on the stack, but in practice it is as hard as getting _this_ on
a 32-bit build (and that means quite time consuming for a release build).

Also, the reason for adding the exit was not to protect users at all... it was
to help with people looking for rewards by attaching a debugger and not being
careful about what they were doing. I don't think we have evidence that users
could be in danger by not adding an exit. And optimizing for those "users" needs
instead of developers is not the right tradeoff.

> don't think we should be letting even developers skip over a CHECK as the
state
> of execution might be in a bad way after a CHECK.

I don't think there is any hope in trying to forbid developers to do something.
They own the machine.

[Will Harris, 2014-12-01 23:12:29]

wfh@chromium.org changed reviewers:
+ cpu@chromium.org, jschuh@chromium.org

[Will Harris, 2014-12-01 23:13:29]

On 2014/12/01 22:54:32, rvargas wrote:
> On 2014/12/01 22:28:48, Will Harris OOO until 1 Dec wrote:
> > On 2014/12/01 22:18:03, rvargas wrote:
> > > On 2014/12/01 20:51:35, Will Harris OOO until 1 Dec wrote:
> > > > On 2014/12/01 19:00:58, rvargas wrote:
> > > > > On 2014/11/27 03:10:24, Will Harris OOO until 1 Dec wrote:
> > > > > > On 2014/11/27 02:10:19, rvargas wrote:
> > > > > > 
> > > > > > CHECK should always terminate the process, is there a risk here that
> if
> > a
> > > > > > debugger is configured to attach and continue, a CHECK gets ignored
in
> > > > > release?
> > > > > 
> > > > > If a debugger is configured to attach, then the process is presumably
> > being
> > > > > actively debugged, in which case, why do we want to override what the
> > person
> > > > > debugging the process is doing? The whole point of the change is to
let
> > > > > developers debug Chrome.
> > > > 
> > > > I suppose I'm more thinking of debuggers that are configured as post
> mortem
> > > e.g.
> > > > dr watson, that might attach on a debugbreak but then be configured to
> > resume
> > > > execution afterwards.  I don't know the prevalence of these debuggers. 
I
> > > worry
> > > > that removing the exit might have security implications if we are
relying
> on
> > a
> > > > CHECK to always terminate process (e.g.
ASSERT_WITH_SECURITY_IMPLICATION).
> > > 
> > > I don't believe we had any issues with this behavior when it was in effect
> > (for
> > > a few years).
> > > 
> > > I don't know if it is possible to configure Dr Watson to continue
execution
> of
> > a
> > > process, given that it is not a debugger but a post-mortem reporter (so
the
> > > application is crashing and it is a mistake to just ignore exceptions).
I'll
> > > look into that anyway.
> > > 
> > > On the other hand, if a user configures a real debugger to attach on
> > exceptions,
> > > ignore the exception and continue execution, that is not a user we should
> try
> > to
> > > cover with our policies. That qualifies as a developer in my book (anyone
> who
> > > configures a real debugger to attach on exceptions), and our policy should
> be
> > to
> > > make their job easier as long as it doesn't put real users in danger.
> > 
> > I'm wary of removing this exit.  I re-read the bug and can't understand why
> it's
> > not possible the developer to just look at the stack frames to get locals -
I
> 
> Remember that the 64-bit ABI makes it so that many arguments are now being
> passed through registers so the simplest operation (show the arguments by
simple
> inspection of the stack) doesn't work anymore. Sure, they _could_ be grabbed
> from somewhere on the stack, but in practice it is as hard as getting _this_
on
> a 32-bit build (and that means quite time consuming for a release build).
> 
> Also, the reason for adding the exit was not to protect users at all... it was
> to help with people looking for rewards by attaching a debugger and not being
> careful about what they were doing. I don't think we have evidence that users
> could be in danger by not adding an exit. And optimizing for those "users"
needs
> instead of developers is not the right tradeoff.
> 
> > don't think we should be letting even developers skip over a CHECK as the
> state
> > of execution might be in a bad way after a CHECK.
> 
> I don't think there is any hope in trying to forbid developers to do
something.
> They own the machine.

Locals have worked in 64-bit release since we added /d2Zi to our builds.  Also,
if 64-bit locals were not working in the debugger, then being able to step back
a frame wouldn't help you for any of the other frames.  BreakDebugger is called
from CHECK() so the exit *is* there to protect users, by ensuring the process is
correctly terminated on a CHECK() - or a ASSERT_WITH_SECURITY_IMPLICATION.  The
"scary-looking access violation" in the comment from r72107 is a potentially
dangerous bit of code being executed after a CHECK().  I still don't see any
benefit of removing the exit, and just added risks.  I'm happy to go with
consensus though, adding +jschuh and +cpu for their views.

[rvargas (doing something else), 2014-12-02 01:06:41]

> Locals have worked in 64-bit release since we added /d2Zi to our builds. 
Also,
> if 64-bit locals were not working in the debugger, then being able to step
back

Maybe locals was not the right term, but I don't see any way around arguments
passed through the stack. Considering that a release build can decide to remove
any number of local variables from the stack and just use registers (which now
are more plentiful), the effect should be the same: execute back to that
function to see what's going on.

> a frame wouldn't help you for any of the other frames.  BreakDebugger is
called
> from CHECK() so the exit *is* there to protect users, by ensuring the process
is

exit() was not added to protect users. It was added to avoid invalid bug reports
from not completely savvy reporters.

> correctly terminated on a CHECK() - or a ASSERT_WITH_SECURITY_IMPLICATION. 
The

And it is, unless a debugger is attached, at which points all bets are off.

> "scary-looking access violation" in the comment from r72107 is a potentially
> dangerous bit of code being executed after a CHECK().  I still don't see any

Again, after pressing F5 on a debugger. That's an error between the chair and
the keyboard.

> benefit of removing the exit, and just added risks.  I'm happy to go with
> consensus though, adding +jschuh and +cpu for their views.

[Will Harris, 2014-12-02 01:58:39]

On 2014/12/02 01:06:41, rvargas wrote:
> > Locals have worked in 64-bit release since we added /d2Zi to our builds. 
> Also,
> > if 64-bit locals were not working in the debugger, then being able to step
> back
> 
> Maybe locals was not the right term, but I don't see any way around arguments
> passed through the stack. Considering that a release build can decide to
remove
> any number of local variables from the stack and just use registers (which now
> are more plentiful), the effect should be the same: execute back to that
> function to see what's going on.

/d2Zi+ should give both locals and arguments on the stack, I'm curious if you're
seeing cases when you are unable to see these in x64 Release builds, because
that could be a bug somewhere.  Can you point me at a dump where you're not
seeing them?

> 
> > a frame wouldn't help you for any of the other frames.  BreakDebugger is
> called
> > from CHECK() so the exit *is* there to protect users, by ensuring the
process
> is
> 
> exit() was not added to protect users. It was added to avoid invalid bug
reports
> from not completely savvy reporters.
> 
> > correctly terminated on a CHECK() - or a ASSERT_WITH_SECURITY_IMPLICATION. 
> The
> 
> And it is, unless a debugger is attached, at which points all bets are off.
> 
> > "scary-looking access violation" in the comment from r72107 is a potentially
> > dangerous bit of code being executed after a CHECK().  I still don't see any
> 
> Again, after pressing F5 on a debugger. That's an error between the chair and
> the keyboard.
> 

We should make sure there are no cases under a default Chrome install where a
DebugBreak() resumes execution.  We should add a test to this effect, verifying
that breakpad or WER correctly catches and terminates the process.

[rvargas (doing something else), 2014-12-03 21:19:17]

Patchset #2 (id:20001) has been deleted

[rvargas (doing something else), 2014-12-03 21:19:26]

Patchset #2 (id:40001) has been deleted

[cpu_(ooo_6.6-7.5), 2014-12-03 23:20:26]

Breakpad does not act as a debugger, it only uses the the unhandled exception
filter, which cannot skip execptions, at most it can olny do what __except()
can.

I don't think we should assert security guarantees while a process is debugged.
I think we need to make debugging a reliable experience.

lgtm on my side.

[rvargas (doing something else), 2014-12-03 23:25:12]

Patchset #2 (id:60001) has been deleted

[rvargas (doing something else), 2014-12-03 23:25:22]

Patchset #2 (id:80001) has been deleted

[rvargas (doing something else), 2014-12-04 00:09:07]

I added unit tests for the layer where this code lives (base).

I verified that Dr Watson cannot be configured to ignore exceptions (it
understands -g as an option but ignores it).

There is an option (not default) for WER to specify that a dialog should be
shown to debug an app... but that requires a real post mortem debugger to be
installed and configured.

There are breakpad unit tests that verify that an app is actually terminated
after an exception, although to be fair a particular debug exception is not
tested. Still, the actual breakpad code is not doing anything to attempt to
continue execution (which is not trivial) for any exception. I have no idea
where that tests are actually run (not part of Chrome).

[Will Harris, 2014-12-04 00:18:45]

lgtm. thanks for adding the tests.  I would still be interested to hear of cases
when you can't see locals/arguments for all stack frames in Release x64.

[rvargas (doing something else), 2014-12-04 01:40:55]

On 2014/12/04 00:18:45, Will Harris wrote:
> lgtm. thanks for adding the tests.  I would still be interested to hear of
cases
> when you can't see locals/arguments for all stack frames in Release x64.

Thanks!.

Playing with recent builds gives much better results (independent of the
architecture) so yes, I'd say the new compiler flags are working.

[rvargas (doing something else), 2014-12-04 19:44:27]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-12-04 19:45:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/759283005/120001

[commit-bot: I haz the power, 2014-12-04 19:47:33]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2014-12-04 19:48:18]

Patchset 3 (id:??) landed as
https://crrev.com/9e9ce3c56b155d66c4ee324edd19db6fe35fb876
Cr-Commit-Position: refs/heads/master@{#306871}

[Nico, 2015-08-17 17:15:25]

On 2014/12/03 23:20:26, cpu (OOO until 8-15) wrote:
> Breakpad does not act as a debugger, it only uses the the unhandled exception
> filter, which cannot skip execptions, at most it can olny do what __except()
> can.
> 
> I don't think we should assert security guarantees while a process is
debugged.
> I think we need to make debugging a reliable experience.

Hm, I think this change makes debugging _less_ reliable. It makes sense if
compilers can assume that CHECK(false) is noreturn, and because of that they can
opt to delete code after a CHECK(false) (and warn about it not having an effect
etc). With this change, what you're debugging no longer matches what the
compiler sees.

(Also see https://codereview.chromium.org/1224553002 where I did this change in
reverse for that reason.)

I don't think this change is a good idea.

(I saw this review because of
http://code.google.com/p/chromium/issues/detail?id=520982 but I don't think it's
a good idea independent of that bug.)

[rvargas (doing something else), 2015-08-17 18:41:01]

On 2015/08/17 17:15:25, Nico wrote:
> On 2014/12/03 23:20:26, cpu (OOO until 8-15) wrote:
> > Breakpad does not act as a debugger, it only uses the the unhandled
exception
> > filter, which cannot skip execptions, at most it can olny do what __except()
> > can.
> > 
> > I don't think we should assert security guarantees while a process is
> debugged.
> > I think we need to make debugging a reliable experience.
> 
> Hm, I think this change makes debugging _less_ reliable. It makes sense if
> compilers can assume that CHECK(false) is noreturn, and because of that they
can
> opt to delete code after a CHECK(false) (and warn about it not having an
effect
> etc). With this change, what you're debugging no longer matches what the
> compiler sees.

In any case, what one debugs on a Release build is what we ship to users so I
don't follow an argument saying that one will be debugging something different
(something that the compiler doesn't see).

Also, no matter what, we'll have the need to run the released version of Chrome
under a debugger. Are you suggesting that we should just say "sorry, you cannot
debug that?". Assuming you are not saying that, why should we be making the
process of debugging a fault more difficult that what already is?

> 
> (Also see https://codereview.chromium.org/1224553002 where I did this change
in
> reverse for that reason.)
> 
> I don't think this change is a good idea.
> 
> (I saw this review because of
> http://code.google.com/p/chromium/issues/detail?id=520982 but I don't think
it's
> a good idea independent of that bug.)

I'm looking forward too see what went wrong with that bug.