[ignition, debugger] correctly set position for return with elided bytecode.

[ignition, debugger] correctly set position for return with elided bytecode.

We may not emit bytecode for the evaluation of the to-be-returned
expression. In that case we cannot set two return positions for a return
statement (one before and one after the expression evaluation). This
sets the interpreter apart from full-codegen.

Make sure that we always have the second of the two return positions.

Note that we end up with separate test cases for ignition and FCG.

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

Committed: https://crrev.com/3c1dc424d3f2f651ad5f250817eb10c7078da2bc
Cr-Commit-Position: refs/heads/master@{#34771}

[Yang, 2016-03-14 09:32:23]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-14 09:32:32]

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

[commit-bot: I haz the power, 2016-03-14 10:00:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-14 10:00:43]

Dry run: Try jobs failed on following builders:
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)

[vogelheim, 2016-03-14 10:41:15]

I'm not quite sure... Please have a look at the comments.

https://codereview.chromium.org/1801473003/diff/1/src/interpreter/source-posi...
File src/interpreter/source-position-table.cc (right):

https://codereview.chromium.org/1801473003/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.cc:142: if (bytes_.size() > 0 &&
previous_.bytecode_offset == entry.bytecode_offset) {
I'm not sure this is correct... This change always keeps the "current" entry in
candidate_ - much like earlier versions of this code did - but it compares
against previous_ - not candidate_ - to determine whether to write an entry.

So if we write entries A, B, C, I expect something like this to happen:

[p_ == previous_, c_ == candidate_]

- p_: {0,..}, c_: {-1,...}
- AddEntry(A)
  - p_: {0,..}, c_: A
- AddEntry(B)
  - p_: A, c_: B
- AddEntry(C)
  - ...

If B + C have the same offset and OVERWRITE_DUPLICATE, wouldn't it still write
B, since in the last step's AddEntry it will compare C against A, rather then B?

https://codereview.chromium.org/1801473003/diff/1/src/interpreter/source-posi...
File src/interpreter/source-position-table.h (right):

https://codereview.chromium.org/1801473003/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.h:48: enum OnDuplicateCodeOffset {
style nitpick: c++ style wants an enum declaration before the constructors.

Ref: https://google.github.io/styleguide/cppguide.html#Declaration_Order

https://codereview.chromium.org/1801473003/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.h:64: static const int
kUninitializedCandidateOffset = -1;
Also style nitpick: consts also go before methods.

https://codereview.chromium.org/1801473003/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.h:71: PositionTableEntry candidate_;
Maybe add mini-comments on what these do, e.g.:

... candidate_;  // Next entry to be written, unless offset !=
kUninitializedCandidateOffset.
... previous_;  // Previously written entry, used to compute deltas.

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

https://codereview.chromium.org/1801473003/diff/1/test/cctest/cctest.status#o...
test/cctest/cctest.status:516: 'test-debug/DebugConditional': [FAIL],
I would have expected DebugConditional to be affected, too, since that's the
test case where I found the problem.

https://codereview.chromium.org/1801473003/diff/1/test/unittests/interpreter/...
File test/unittests/interpreter/source-position-table-unittest.cc (right):

https://codereview.chromium.org/1801473003/diff/1/test/unittests/interpreter/...
test/unittests/interpreter/source-position-table-unittest.cc:48: 
Maybe a good test case would be to just add each offset 4 times, and alternate
OVERWRITE_DUPLICATE based on (i % 2), or so.

[Yang, 2016-03-14 11:32:34]

https://codereview.chromium.org/1801473003/diff/1/src/interpreter/source-posi...
File src/interpreter/source-position-table.cc (right):

https://codereview.chromium.org/1801473003/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.cc:142: if (bytes_.size() > 0 &&
previous_.bytecode_offset == entry.bytecode_offset) {
On 2016/03/14 10:41:15, vogelheim wrote:
> I'm not sure this is correct... This change always keeps the "current" entry
in
> candidate_ - much like earlier versions of this code did - but it compares
> against previous_ - not candidate_ - to determine whether to write an entry.
> 
> So if we write entries A, B, C, I expect something like this to happen:
> 
> [p_ == previous_, c_ == candidate_]
> 
> - p_: {0,..}, c_: {-1,...}
> - AddEntry(A)
>   - p_: {0,..}, c_: A
> - AddEntry(B)
>   - p_: A, c_: B
> - AddEntry(C)
>   - ...
> 
> If B + C have the same offset and OVERWRITE_DUPLICATE, wouldn't it still write
> B, since in the last step's AddEntry it will compare C against A, rather then
B?
> 
> 
> 
>   
> 
> 

You are right. Fixed.

https://codereview.chromium.org/1801473003/diff/1/src/interpreter/source-posi...
File src/interpreter/source-position-table.h (right):

https://codereview.chromium.org/1801473003/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.h:48: enum OnDuplicateCodeOffset {
On 2016/03/14 10:41:15, vogelheim wrote:
> style nitpick: c++ style wants an enum declaration before the constructors.
> 
> Ref: https://google.github.io/styleguide/cppguide.html#Declaration_Order

Done.

https://codereview.chromium.org/1801473003/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.h:64: static const int
kUninitializedCandidateOffset = -1;
On 2016/03/14 10:41:15, vogelheim wrote:
> Also style nitpick: consts also go before methods.

Done.

https://codereview.chromium.org/1801473003/diff/1/src/interpreter/source-posi...
src/interpreter/source-position-table.h:71: PositionTableEntry candidate_;
On 2016/03/14 10:41:15, vogelheim wrote:
> Maybe add mini-comments on what these do, e.g.:
> 
> ... candidate_;  // Next entry to be written, unless offset !=
> kUninitializedCandidateOffset.
> ... previous_;  // Previously written entry, used to compute deltas.

Done.

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

https://codereview.chromium.org/1801473003/diff/1/test/cctest/cctest.status#o...
test/cctest/cctest.status:516: 'test-debug/DebugConditional': [FAIL],
On 2016/03/14 10:41:15, vogelheim wrote:
> I would have expected DebugConditional to be affected, too, since that's the
> test case where I found the problem.

No. This CL is not the fix for this. We are going to have separate test
expectations for ignition and FCG. This CL simply makes sure that at a return,
the second break location overrides the first one if they have the same bytecode
offset.

https://codereview.chromium.org/1801473003/diff/1/test/unittests/interpreter/...
File test/unittests/interpreter/source-position-table-unittest.cc (right):

https://codereview.chromium.org/1801473003/diff/1/test/unittests/interpreter/...
test/unittests/interpreter/source-position-table-unittest.cc:48: 
On 2016/03/14 10:41:15, vogelheim wrote:
> Maybe a good test case would be to just add each offset 4 times, and alternate
> OVERWRITE_DUPLICATE based on (i % 2), or so.

Done.

[Yang, 2016-03-14 11:33:15]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-14 11:33:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1801473003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1801473003/20001

[commit-bot: I haz the power, 2016-03-14 11:55:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-14 11:55:17]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-03-14 14:54:55]

LGTM, thanks!

https://codereview.chromium.org/1801473003/diff/20001/test/mjsunit/ignition/e...
File test/mjsunit/ignition/elided-instruction.js (right):

https://codereview.chromium.org/1801473003/diff/20001/test/mjsunit/ignition/e...
test/mjsunit/ignition/elided-instruction.js:5: // Flags: --expose-debug-as debug
--ignition --nostress-opt --noalways-opt
I guess this will set clusterfuzz on ignition, but given we are talking about
doing that anyway explicitly this should be ok.

[Yang, 2016-03-14 15:02:59]

On 2016/03/14 14:54:55, rmcilroy wrote:
> LGTM, thanks!
> 
>
https://codereview.chromium.org/1801473003/diff/20001/test/mjsunit/ignition/e...
> File test/mjsunit/ignition/elided-instruction.js (right):
> 
>
https://codereview.chromium.org/1801473003/diff/20001/test/mjsunit/ignition/e...
> test/mjsunit/ignition/elided-instruction.js:5: // Flags: --expose-debug-as
debug
> --ignition --nostress-opt --noalways-opt
> I guess this will set clusterfuzz on ignition, but given we are talking about
> doing that anyway explicitly this should be ok.

Totally forgot about that. We could also do that via mjsunit.status, if you
prefer that.

[vogelheim, 2016-03-14 17:24:08]

lgtm

[Yang, 2016-03-15 08:03:35]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-03-15 08:03:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/1801473003/#ps40001
(title: "do not use --ignition flag")

[commit-bot: I haz the power, 2016-03-15 08:03:44]

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

[commit-bot: I haz the power, 2016-03-15 08:26:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-15 08:28:09]

Description was changed from

==========
[ignition, debugger] correctly set position for return with elided bytecode.

We may not emit bytecode for the evaluation of the to-be-returned
expression. In that case we cannot set two return positions for a return
statement (one before and one after the expression evaluation). This
sets the interpreter apart from full-codegen.

Make sure that we always have the second of the two return positions.

Note that we end up with separate test cases for ignition and FCG.

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

to

==========
[ignition, debugger] correctly set position for return with elided bytecode.

We may not emit bytecode for the evaluation of the to-be-returned
expression. In that case we cannot set two return positions for a return
statement (one before and one after the expression evaluation). This
sets the interpreter apart from full-codegen.

Make sure that we always have the second of the two return positions.

Note that we end up with separate test cases for ignition and FCG.

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

Committed: https://crrev.com/3c1dc424d3f2f651ad5f250817eb10c7078da2bc
Cr-Commit-Position: refs/heads/master@{#34771}
==========

[commit-bot: I haz the power, 2016-03-15 08:28:10]

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