[futex] Allow debugger to break in the middle of a futexWait

[futex] Allow debugger to break in the middle of a futexWait

R=yangguo@chromium.org

[Yang, 2015-08-27 06:27:25]

https://codereview.chromium.org/1321543004/diff/1/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/1321543004/diff/1/src/debug/debug.cc#newcode2281
src/debug/debug.cc:2281: Handle<JSFunction> jsfun(JSFunction::cast(fun));
I think it might be cleaner to just pass in an optional parameter from
FutexEmulation::Wait through Execution::HandleInterrupts to here, stating that
the interrupt comes from futex wait.

https://codereview.chromium.org/1321543004/diff/1/test/cctest/test-debug.cc
File test/cctest/test-debug.cc (right):

https://codereview.chromium.org/1321543004/diff/1/test/cctest/test-debug.cc#n...
test/cctest/test-debug.cc:7740: "\"command\":\"continue\"}";
Please do not use the JSON protocol for tests anymore. Chrome does not use it.
Use the JS API instead.

[binji, 2015-08-31 21:25:18]

https://codereview.chromium.org/1321543004/diff/1/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/1321543004/diff/1/src/debug/debug.cc#newcode2281
src/debug/debug.cc:2281: Handle<JSFunction> jsfun(JSFunction::cast(fun));
On 2015/08/27 at 06:27:25, Yang wrote:
> I think it might be cleaner to just pass in an optional parameter from
FutexEmulation::Wait through Execution::HandleInterrupts to here, stating that
the interrupt comes from futex wait.

Done.

https://codereview.chromium.org/1321543004/diff/1/test/cctest/test-debug.cc
File test/cctest/test-debug.cc (right):

https://codereview.chromium.org/1321543004/diff/1/test/cctest/test-debug.cc#n...
test/cctest/test-debug.cc:7740: "\"command\":\"continue\"}";
On 2015/08/27 at 06:27:25, Yang wrote:
> Please do not use the JSON protocol for tests anymore. Chrome does not use it.
Use the JS API instead.

What is the JS API for continue? All I could find was
DebugCommandProcessor.prototype.continueRequest_ in debug.js, which seems to be
only dispatched from the JSON protocol as well.

[Michael Starzinger, 2015-08-31 21:52:20]

mstarzinger@chromium.org changed reviewers:
+ jarin@chromium.org, mstarzinger@chromium.org

[Michael Starzinger, 2015-08-31 21:52:21]

Some concerns. Also adding Jaro who might have an opinion about this.

https://codereview.chromium.org/1321543004/diff/20001/src/execution.h
File src/execution.h (right):

https://codereview.chromium.org/1321543004/diff/20001/src/execution.h#newcode212
src/execution.h:212: BreakInNonDebuggableFlag flag = DontBreakInNonDebuggable);
Hmpf! I realize that Yang suggested this approach, but this gives me the
shivers. Now execution.h contains both debugger and futex semantics. I am not
comfortable with this approach at all and would have actually preferred your
first patch set over this.

Yang, do you still think this second patch set is cleaner?

[Yang, 2015-08-31 22:16:28]

On 2015/08/31 21:52:21, Michael Starzinger wrote:
> Some concerns. Also adding Jaro who might have an opinion about this.
> 
> https://codereview.chromium.org/1321543004/diff/20001/src/execution.h
> File src/execution.h (right):
> 
>
https://codereview.chromium.org/1321543004/diff/20001/src/execution.h#newcode212
> src/execution.h:212: BreakInNonDebuggableFlag flag =
DontBreakInNonDebuggable);
> Hmpf! I realize that Yang suggested this approach, but this gives me the
> shivers. Now execution.h contains both debugger and futex semantics. I am not
> comfortable with this approach at all and would have actually preferred your
> first patch set over this.
> 
> Yang, do you still think this second patch set is cleaner?

Michael is right. The current solution is not ideal either. How about this:
instead of threading this through execution.cc, we could set a bool on the Debug
object via a scope similar to IgnoreBreaks (I think thats how its called). That
would contain the logic to the debugger and the futex code.

[binji, 2015-08-31 22:37:01]

> Michael is right. The current solution is not ideal either. How about this:
instead of threading this through execution.cc, we could set a bool on the Debug
object via a scope similar to IgnoreBreaks (I think thats how its called). That
would contain the logic to the debugger and the futex code.

Like this?

[Yang, 2015-09-01 01:55:49]

yup. thats what I had in mind.

https://codereview.chromium.org/1321543004/diff/40001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/1321543004/diff/40001/src/debug/debug.cc#newc...
src/debug/debug.cc:2282: break_in_nondebuggable_))
please add brackets around return

[Michael Starzinger, 2015-09-01 07:58:10]

On 2015/08/31 22:37:01, binji wrote:
> > Michael is right. The current solution is not ideal either. How about this:
> instead of threading this through execution.cc, we could set a bool on the
Debug
> object via a scope similar to IgnoreBreaks (I think thats how its called).
That
> would contain the logic to the debugger and the futex code.
> 
> Like this?

Works for me. Thanks!

[binji, 2015-09-16 17:17:54]

https://codereview.chromium.org/1321543004/diff/40001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/1321543004/diff/40001/src/debug/debug.cc#newc...
src/debug/debug.cc:2282: break_in_nondebuggable_))
On 2015/09/01 at 01:55:48, Yang OOO until mid October wrote:
> please add brackets around return

Done.

[binji, 2015-09-16 17:21:13]

https://codereview.chromium.org/1321543004/diff/80001/test/cctest/test-debug.cc
File test/cctest/test-debug.cc (right):

https://codereview.chromium.org/1321543004/diff/80001/test/cctest/test-debug....
test/cctest/test-debug.cc:7763: const char* command_continue =
It sounds like Yang will be OoO for a month. He didn't want me to use the JSON
protocol here, and use the JS API instead, but I can't see how to do that.

Do either of you know?