[interpreter, debugger] fix remaining cctest failures.

[interpreter, debugger] fix remaining cctest failures.

R=rmcilroy@chromium.org, vogelheim@chromium.org
BUG=v8:4690
LOG=N

Committed: https://crrev.com/d0d73999a1139996c7707bca5501fcaafde38672
Cr-Commit-Position: refs/heads/master@{#34781}

[Yang, 2016-03-15 09:32:12]

Please take a look.

https://codereview.chromium.org/1802903002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (left):

https://codereview.chromium.org/1802903002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:971:
builder()->SetStatementPosition(stmt);
In FCG, we set a statement position, but no debug break slot. So it would be a
non-breakable statement position.

Originally, I worried that we may need to port that to ignition as well. But
now, if I'm not mistaken, we don't actually have to. A statement position that
is not associated with a break location has no value for the debugger. For stack
traces it does not matter either, since locations of stack traces are at calls
and throw sites, but not any of these statement positions that do not break.

So I think we could just omit them completely.

[vogelheim, 2016-03-15 09:51:51]

lgtm

https://codereview.chromium.org/1802903002/diff/1/test/cctest/cctest.status
File test/cctest/cctest.status (left):

https://codereview.chromium.org/1802903002/diff/1/test/cctest/cctest.status#o...
test/cctest/cctest.status:512: # TODO(yangguo,4690): Test failures in debugger
tests.
This makes me happy!

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

https://codereview.chromium.org/1802903002/diff/1/test/cctest/test-debug.cc#n...
test/cctest/test-debug.cc:3463: "  return x ? 1 : 2;"
Hmm. Not sure, but this might count as cheating... ;-)

This new test case tests exactly what the old one was *meant* to test, so it's
arguably an improvement. But the old test case (accidentally) triggered a
difference in FCG and Ignition breakpoints (via an unrelated optimization in
Ignition), which is valuable in its own right.

Since we've come to the conclusion that this difference is intended (or at least
best we can do for now), I think ideally I'd want a test case that "documents"
this. E.g., the old test case, but with something like:
  CHECK_IMPLIES(FLAG_ignition, 2 == breakpoint_hit_count);
  CHECK_IMPLIES(!FLAG_ignition, 3 == breakpoint_hit_count);


I'm not entirely sure either way, so.. your choice! :)

[Yang, 2016-03-15 10:00:12]

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

https://codereview.chromium.org/1802903002/diff/1/test/cctest/test-debug.cc#n...
test/cctest/test-debug.cc:3463: "  return x ? 1 : 2;"
On 2016/03/15 09:51:51, vogelheim wrote:
> Hmm. Not sure, but this might count as cheating... ;-)
> 
> This new test case tests exactly what the old one was *meant* to test, so it's
> arguably an improvement. But the old test case (accidentally) triggered a
> difference in FCG and Ignition breakpoints (via an unrelated optimization in
> Ignition), which is valuable in its own right.
> 
> Since we've come to the conclusion that this difference is intended (or at
least
> best we can do for now), I think ideally I'd want a test case that "documents"
> this. E.g., the old test case, but with something like:
>   CHECK_IMPLIES(FLAG_ignition, 2 == breakpoint_hit_count);
>   CHECK_IMPLIES(!FLAG_ignition, 3 == breakpoint_hit_count);
> 
> 
> I'm not entirely sure either way, so.. your choice! :)

The old version tested both a conditional, and the assignment-and-return combo.
Now It only tests the conditional, and the newly introduced
src/ignition/elided-instruction* tests test the assignment-and-return combo, so
I think we are not losing any test coverage here :)

[rmcilroy, 2016-03-15 10:30:51]

LGTM, thanks!

https://codereview.chromium.org/1802903002/diff/1/test/cctest/cctest.status
File test/cctest/cctest.status (left):

https://codereview.chromium.org/1802903002/diff/1/test/cctest/cctest.status#o...
test/cctest/cctest.status:512: # TODO(yangguo,4690): Test failures in debugger
tests.
On 2016/03/15 09:51:51, vogelheim wrote:
> This makes me happy!

Yay \o/

[Yang, 2016-03-15 10:33:30]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2016-03-15 10:33:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1802903002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1802903002/1

[commit-bot: I haz the power, 2016-03-15 11:00:38]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-03-15 11:01:17]

Description was changed from

==========
[interpreter, debugger] fix remaining cctest failures.

R=rmcilroy@chromium.org, vogelheim@chromium.org
BUG=v8:4690
LOG=N
==========

to

==========
[interpreter, debugger] fix remaining cctest failures.

R=rmcilroy@chromium.org, vogelheim@chromium.org
BUG=v8:4690
LOG=N

Committed: https://crrev.com/d0d73999a1139996c7707bca5501fcaafde38672
Cr-Commit-Position: refs/heads/master@{#34781}
==========

[commit-bot: I haz the power, 2016-03-15 11:01:18]

Patchset 1 (id:??) landed as
https://crrev.com/d0d73999a1139996c7707bca5501fcaafde38672
Cr-Commit-Position: refs/heads/master@{#34781}