[interpreter] Ensure optimizations preserve source positions.

[interpreter] Ensure optimizations preserve source positions.

The optimization stages in the bytecode generation pipeline must
preserve source position information. Failure to preserve
source position information could result in single stepping
in the debugger misbehaving or mis-reporting in exception stack traces.

This change adds tests intended to check optimizations do not damage
source position info.

BUG=v8:4280
LOG=N

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

[oth, 2016-06-06 05:09:47]

oth@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[oth, 2016-06-06 05:09:47]

Ross, PTAL, thanks.

[rmcilroy, 2016-06-07 09:46:11]

Overall looks good. Couple of comments, and please have Yang have a look too.

https://codereview.chromium.org/2042633002/diff/1/src/interpreter/bytecode-pe...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/2042633002/diff/1/src/interpreter/bytecode-pe...
src/interpreter/bytecode-peephole-optimizer.cc:177:
last->source_info().is_statement()) {
As discussed offline, I think we need to not do this if
current->source_info().is_statement() (irrespective of last's source position).

https://codereview.chromium.org/2042633002/diff/1/src/interpreter/bytecode-pe...
src/interpreter/bytecode-peephole-optimizer.cc:179: return;
This returns here without doing anything, but then returns "true" in
"ChangeLdaToLdr". Could we move the check into ChangeLdaToLdr

As an alternative (possibly clearer), could we split out
TransformCurrentBytecode and TransformLastAndCurrentBytecodes as separate
top-level calls in optimize, then we could move the statement/statement check
into TransformLastAndCurrentBytecodes, since we probably shouldn't be messing
with both if both are statements. WDYT?

https://codereview.chromium.org/2042633002/diff/1/src/interpreter/bytecode-pe...
src/interpreter/bytecode-peephole-optimizer.cc:201: }
As discussed offline - I don't think this block does anything any longer, we
should probably just remove it.

https://codereview.chromium.org/2042633002/diff/1/src/interpreter/bytecodes.cc
File src/interpreter/bytecodes.cc (right):

https://codereview.chromium.org/2042633002/diff/1/src/interpreter/bytecodes.c...
src/interpreter/bytecodes.cc:495: bytecode == Bytecode::kLdrUndefined ||
bytecode == Bytecode::kMov ||
nit - Could we add IsRegisterLoadWithoutEffects, for kLdrUndefined, kStar and
kMov (avoiding the need for IsLdarOrStar and giving somewhere to put any more
Ldr bytecodes we might add).

https://codereview.chromium.org/2042633002/diff/1/src/interpreter/bytecodes.c...
src/interpreter/bytecodes.cc:497: (IsJump(bytecode) &&
!IsJumpIfToBoolean(bytecode)));
nit - could we add IsJumpWithoutExternalEffect() for clarity.

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/sou...
File test/cctest/interpreter/source-position-matcher.cc (right):

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/sou...
test/cctest/interpreter/source-position-matcher.cc:16: struct PTECompare {
I don't think it's worth abbreviating this given you only use the name once ->
PositionTableEntryComparer ?

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/sou...
test/cctest/interpreter/source-position-matcher.cc:41: //    statement positions
may be moved when their initial posiiton is
/s/posiiton/position

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/sou...
test/cctest/interpreter/source-position-matcher.cc:68: // position to ease
validation).
I think it's a bit confusing to terminate with the statement position. does that
really help with validation? I think it is clearer to use original/optimized as
the statement positions (which you do below) and only have expression in the
vector - you seem to skip the statements when checking properties of the source
positions in the vector anyway AFAIKT.

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/sou...
test/cctest/interpreter/source-position-matcher.cc:86: if
(NewExpressionPositionsInOptimized(&original_expression_entries,
nit - HasNewExpressionPositionsInOptimized

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/sou...
test/cctest/interpreter/source-position-matcher.cc:98: return false;
nit - comment that message is printed in CompareExpressionPositions

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/sou...
test/cctest/interpreter/source-position-matcher.cc:169: printf("Original
expression source positions");
Is some of this logging printfs? Maybe you should add a log_error function to
make it clear when you are logging an error message.

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/sou...
test/cctest/interpreter/source-position-matcher.cc:227:
CHECK(iterator.current_bytecode() != Bytecode::kStar);
Not sure why this CHECK is here. It couldn't be Star if
IsWithoutExternalSideEffects could it?

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/sou...
File test/cctest/interpreter/source-position-matcher.h (right):

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/sou...
test/cctest/interpreter/source-position-matcher.h:43: int number_of_locals()
const { return number_of_locals_; }
This seems unused. Are you intending to add a test based on whether locals are
updated?

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/tes...
File test/cctest/interpreter/test-source-positions.cc (right):

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-source-positions.cc:107: size_t bitmask) {
Could we just have bools for each of the flags or an ENUM rather than a bitmap,
I think that would be much easier to understand.

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-source-positions.cc:136: for (size_t i = 1; i <
count; ++i) {
I'd prefer we had seperate tests for each optimization level. E.g.,

TEST(SourcePositionsEquivalentPeepHole) {
  SourcePositionsEquivalent(true, false);
}

TEST(SourcePositionsEquivalentReoPeepHole) {
  SourcePositionsEquivalent(true, true);
}

To make it obvious if one fails which optimization stage is at fault.

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-source-positions.cc:143: printf("Unoptimized:\n");
remove logging

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-source-positions.cc:177: ignition_flag = true;
nit - You could set these flags (and initialize the interpreter) in
OptimizedBytecodeSourcePositionTester

[oth, 2016-06-07 13:46:18]

Thanks, incorporated the bulk of this. Looking for a better way to select flag
combinations before suggesting to look again.

Cheers
Orion

https://codereview.chromium.org/2042633002/diff/1/src/interpreter/bytecode-pe...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/2042633002/diff/1/src/interpreter/bytecode-pe...
src/interpreter/bytecode-peephole-optimizer.cc:177:
last->source_info().is_statement()) {
On 2016/06/07 09:46:10, rmcilroy wrote:
> As discussed offline, I think we need to not do this if
> current->source_info().is_statement() (irrespective of last's source
position).

Yes, thanks for examining this. I agree.

                                        2nd Bytecode
                                           Star
                            Nothing        Expr         Statement

               Nothing       Yes           Yes             No

1st Bytecode
LdaGlobal      Expr          Yes           Yes             No


               Statement     Yes           Yes             No

https://codereview.chromium.org/2042633002/diff/1/src/interpreter/bytecode-pe...
src/interpreter/bytecode-peephole-optimizer.cc:179: return;
On 2016/06/07 09:46:11, rmcilroy wrote:
> This returns here without doing anything, but then returns "true" in
> "ChangeLdaToLdr". Could we move the check into ChangeLdaToLdr

Done.

> As an alternative (possibly clearer), could we split out
> TransformCurrentBytecode and TransformLastAndCurrentBytecodes as separate
> top-level calls in optimize, then we could move the statement/statement check
> into TransformLastAndCurrentBytecodes, since we probably shouldn't be messing
> with both if both are statements. WDYT?

Done, good idea. Since we only have the one transform in
TransformLastAndCurrent, the check is in the branch to do the transform since we
don't know if this rule would apply to other potential transforms.

https://codereview.chromium.org/2042633002/diff/1/src/interpreter/bytecode-pe...
src/interpreter/bytecode-peephole-optimizer.cc:201: }
On 2016/06/07 09:46:11, rmcilroy wrote:
> As discussed offline - I don't think this block does anything any longer, we
> should probably just remove it.

Done.

https://codereview.chromium.org/2042633002/diff/1/src/interpreter/bytecodes.cc
File src/interpreter/bytecodes.cc (right):

https://codereview.chromium.org/2042633002/diff/1/src/interpreter/bytecodes.c...
src/interpreter/bytecodes.cc:495: bytecode == Bytecode::kLdrUndefined ||
bytecode == Bytecode::kMov ||
On 2016/06/07 09:46:11, rmcilroy wrote:
> nit - Could we add IsRegisterLoadWithoutEffects, for kLdrUndefined, kStar and
> kMov (avoiding the need for IsLdarOrStar and giving somewhere to put any more
> Ldr bytecodes we might add).

Done.

The thought had occurred whether we should annotate bytecodes with this
information so each is explicitly classified. This would be less error prone
should bytecodes change. I don't propose doing it here, but something we might
consider.

https://codereview.chromium.org/2042633002/diff/1/src/interpreter/bytecodes.c...
src/interpreter/bytecodes.cc:497: (IsJump(bytecode) &&
!IsJumpIfToBoolean(bytecode)));
On 2016/06/07 09:46:11, rmcilroy wrote:
> nit - could we add IsJumpWithoutExternalEffect() for clarity.

Done.

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/sou...
File test/cctest/interpreter/source-position-matcher.cc (right):

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/sou...
test/cctest/interpreter/source-position-matcher.cc:16: struct PTECompare {
On 2016/06/07 09:46:11, rmcilroy wrote:
> I don't think it's worth abbreviating this given you only use the name once ->
> PositionTableEntryComparer ?

Done.

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/sou...
test/cctest/interpreter/source-position-matcher.cc:41: //    statement positions
may be moved when their initial posiiton is
On 2016/06/07 09:46:11, rmcilroy wrote:
> /s/posiiton/position

Done.

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/sou...
test/cctest/interpreter/source-position-matcher.cc:68: // position to ease
validation).
On 2016/06/07 09:46:11, rmcilroy wrote:
> I think it's a bit confusing to terminate with the statement position. does
that
> really help with validation? I think it is clearer to use original/optimized
as
> the statement positions (which you do below) and only have expression in the
> vector - you seem to skip the statements when checking properties of the
source
> positions in the vector anyway AFAIKT.

Okay, done. The statement position is used as a back marker, but now passed as
an argument.

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/sou...
test/cctest/interpreter/source-position-matcher.cc:86: if
(NewExpressionPositionsInOptimized(&original_expression_entries,
On 2016/06/07 09:46:11, rmcilroy wrote:
> nit - HasNewExpressionPositionsInOptimized

Done.

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/sou...
test/cctest/interpreter/source-position-matcher.cc:98: return false;
On 2016/06/07 09:46:11, rmcilroy wrote:
> nit - comment that message is printed in CompareExpressionPositions

Done.

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/sou...
test/cctest/interpreter/source-position-matcher.cc:169: printf("Original
expression source positions");
On 2016/06/07 09:46:11, rmcilroy wrote:
> Is some of this logging printfs? Maybe you should add a log_error function to
> make it clear when you are logging an error message.

I can't see anything in v8 tests that attempts to be this helpful. Removed
these.

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/sou...
test/cctest/interpreter/source-position-matcher.cc:227:
CHECK(iterator.current_bytecode() != Bytecode::kStar);
On 2016/06/07 09:46:11, rmcilroy wrote:
> Not sure why this CHECK is here. It couldn't be Star if
> IsWithoutExternalSideEffects could it?

It's just left over cruft from an earlier revision. Removed.

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/sou...
File test/cctest/interpreter/source-position-matcher.h (right):

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/sou...
test/cctest/interpreter/source-position-matcher.h:43: int number_of_locals()
const { return number_of_locals_; }
On 2016/06/07 09:46:11, rmcilroy wrote:
> This seems unused. Are you intending to add a test based on whether locals are
> updated?

Removed, at least for now. I think the check is pretty hard.

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/tes...
File test/cctest/interpreter/test-source-positions.cc (right):

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-source-positions.cc:107: size_t bitmask) {
On 2016/06/07 09:46:11, rmcilroy wrote:
> Could we just have bools for each of the flags or an ENUM rather than a
bitmap,
> I think that would be much easier to understand.

The intent was adding a new optimization stage would just entail editing a
single place - the array of flags. Per comment below, looking to see if some
kind of parameterized test in google test can help.

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-source-positions.cc:136: for (size_t i = 1; i <
count; ++i) {
On 2016/06/07 09:46:11, rmcilroy wrote:
> I'd prefer we had seperate tests for each optimization level. E.g.,
> 
> TEST(SourcePositionsEquivalentPeepHole) {
>   SourcePositionsEquivalent(true, false);
> }
> 
> TEST(SourcePositionsEquivalentReoPeepHole) {
>   SourcePositionsEquivalent(true, true);
> }
> 
> To make it obvious if one fails which optimization stage is at fault.

I'm not sure this scales. The code currently test all combinations of relevant
flags. There's at least two more flags in the pipeline meaning 15 TEST(xxx) to
get full coverage versus one test that gets everything.

I'm looking around to see if there's a better way to do this in googletest.

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-source-positions.cc:143: printf("Unoptimized:\n");
On 2016/06/07 09:46:11, rmcilroy wrote:
> remove logging

Done.

https://codereview.chromium.org/2042633002/diff/1/test/cctest/interpreter/tes...
test/cctest/interpreter/test-source-positions.cc:177: ignition_flag = true;
On 2016/06/07 09:46:11, rmcilroy wrote:
> nit - You could set these flags (and initialize the interpreter) in
> OptimizedBytecodeSourcePositionTester

Done.

[oth, 2016-06-08 08:17:48]

On 2016/06/07 13:46:18, oth wrote:
> Thanks, incorporated the bulk of this. Looking for a better way to select flag
> combinations before suggesting to look again.

The CL is revised to have unique test names for each combination of flags so
long as folks remember to make the relevant updates. There are two additional
flags in subsequent CLs that will be added after this.

Thanks
Orion

[oth, 2016-06-08 09:26:37]

oth@chromium.org changed reviewers:
+ yangguao@chromium.org

[oth, 2016-06-08 09:27:17]

+Yang, PTAL, thanks.

[rmcilroy, 2016-06-08 11:31:40]

LGTM if Yang is happy.

https://codereview.chromium.org/2042633002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/2042633002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:198: // NB If the Star is tagged
with a statement position, we can't
/s/NB/Note:/

https://codereview.chromium.org/2042633002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecodes.cc (right):

https://codereview.chromium.org/2042633002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.cc:279: bool Bytecodes::IsJumpWithoutEffects(Bytecode
bytecode) {
// static

https://codereview.chromium.org/2042633002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.cc:282: 
// static

https://codereview.chromium.org/2042633002/diff/100001/test/cctest/interprete...
File test/cctest/interpreter/source-position-matcher.cc (right):

https://codereview.chromium.org/2042633002/diff/100001/test/cctest/interprete...
test/cctest/interpreter/source-position-matcher.cc:67: // position to ease
validation).
update comment.

https://codereview.chromium.org/2042633002/diff/100001/test/cctest/interprete...
test/cctest/interpreter/source-position-matcher.cc:97: // Message logged in
CompareExpressionPositions().
No messages logged any longer? FWIW, I liked the messages and thought they would
be useful in tracking down a test failure :(.

[oth, 2016-06-08 14:18:25]

Thanks.

https://codereview.chromium.org/2042633002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-peephole-optimizer.cc (right):

https://codereview.chromium.org/2042633002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-peephole-optimizer.cc:198: // NB If the Star is tagged
with a statement position, we can't
On 2016/06/08 11:31:40, rmcilroy wrote:
> /s/NB/Note:/

Done.

https://codereview.chromium.org/2042633002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecodes.cc (right):

https://codereview.chromium.org/2042633002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.cc:279: bool Bytecodes::IsJumpWithoutEffects(Bytecode
bytecode) {
On 2016/06/08 11:31:40, rmcilroy wrote:
> // static

Done.

https://codereview.chromium.org/2042633002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecodes.cc:282: 
On 2016/06/08 11:31:40, rmcilroy wrote:
> // static

Done.

https://codereview.chromium.org/2042633002/diff/100001/test/cctest/interprete...
File test/cctest/interpreter/source-position-matcher.cc (right):

https://codereview.chromium.org/2042633002/diff/100001/test/cctest/interprete...
test/cctest/interpreter/source-position-matcher.cc:67: // position to ease
validation).
On 2016/06/08 11:31:40, rmcilroy wrote:
> update comment.

Done.

https://codereview.chromium.org/2042633002/diff/100001/test/cctest/interprete...
test/cctest/interpreter/source-position-matcher.cc:97: // Message logged in
CompareExpressionPositions().
On 2016/06/08 11:31:40, rmcilroy wrote:
> No messages logged any longer? FWIW, I liked the messages and thought they
would
> be useful in tracking down a test failure :(.

There doesn't seem to be an existing precedent in the code base. It shouldn't be
that hard to debug.

[oth, 2016-06-08 15:05:12]

Description was changed from

==========
[interpreter] Ensure optimizations preserve source positions.

The optimization stages in the bytecode generation pipeline must
preserve source position information. Failure to preserve
source position information could result in single stepping
in the debugger misbehaving or mis-reporting in exception stack traces.

This change adds tests intended to check optimizations do not damage
source position info.

BUG=v8:4280
LOG=N
==========

to

==========
[interpreter] Ensure optimizations preserve source positions.

The optimization stages in the bytecode generation pipeline must
preserve source position information. Failure to preserve
source position information could result in single stepping
in the debugger misbehaving or mis-reporting in exception stack traces.

This change adds tests intended to check optimizations do not damage
source position info.

BUG=v8:4280
LOG=N
==========

[oth, 2016-06-08 15:05:12]

oth@chromium.org changed reviewers:
+ yangguo@chromium.org
- yangguao@chromium.org

[oth, 2016-06-08 15:06:16]

+Yang. 

Yang, apologies, I fatfingered your LDAP when attempting to send this out. PTAL,
thanks.

[Yang, 2016-06-09 06:02:47]

https://codereview.chromium.org/2042633002/diff/120001/test/cctest/interprete...
File test/cctest/interpreter/bytecode_expectations/PropertyLoads.golden (right):

https://codereview.chromium.org/2042633002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/bytecode_expectations/PropertyLoads.golden:801:
B(LdrKeyedProperty), R(arg0), U8(5), R(0),
Is this correct?

If the keyed property load throws, we will end up with expression position 59
instead of 56 now.

Imo we should either drop the position at Ldar but keep the one at
LdrKeyedProperty, or change the position at Ldar to 56.

[oth, 2016-06-09 08:51:17]

Thanks Yang. There's an explanation to the issue below. Please see if the answer
makes this CL agreeable.

https://codereview.chromium.org/2042633002/diff/120001/test/cctest/interprete...
File test/cctest/interpreter/bytecode_expectations/PropertyLoads.golden (right):

https://codereview.chromium.org/2042633002/diff/120001/test/cctest/interprete...
test/cctest/interpreter/bytecode_expectations/PropertyLoads.golden:801:
B(LdrKeyedProperty), R(arg0), U8(5), R(0),
On 2016/06/09 06:02:46, Yang wrote:
> Is this correct?
> 
> If the keyed property load throws, we will end up with expression position 59
> instead of 56 now.
> 
> Imo we should either drop the position at Ldar but keep the one at
> LdrKeyedProperty, or change the position at Ldar to 56.

The issue spotted here is that the old bytecode in this test is wrong. There's a
change in this CL to the peephole optimizer for this.

Before this CL:

Repro.js:

function f(a, b) {
    var c = a[b];
    c = a[b];
    return c;
}

f({value:1}, "value");

The unoptimized bytecode looks like:

   11 E> 0x3fd969ed117c @    0 : 64                StackCheck 
   33 S> 0x3fd969ed117d @    1 : 19 03             Ldar a0
         0x3fd969ed117f @    3 : 1a f9             Star r1
   33 E> 0x3fd969ed1181 @    5 : 19 02             Ldar a1
         0x3fd969ed1183 @    7 : 1e f9 01          LdaKeyedProperty r1, [1]
   33 E> 0x3fd969ed1186 @   10 : 1a fa             Star r0
   42 S> 0x3fd969ed1188 @   12 : 19 03             Ldar a0
         0x3fd969ed118a @   14 : 1a f9             Star r1
   47 E> 0x3fd969ed118c @   16 : 19 02             Ldar a1
         0x3fd969ed118e @   18 : 1e f9 03          LdaKeyedProperty r1, [3]
   44 E> 0x3fd969ed1191 @   21 : 1a fa             Star r0
   56 S> 0x3fd969ed1193 @   23 : 19 fa             Ldar r0
   66 S> 0x3fd969ed1195 @   25 : 67                Return 

The peephole optimizer has then (erroneously) moved the expression position 44 E
from a Star bytecode to the LdrKeyedProperty.

   11 E> 0x2042f1cd117c @    0 : 64                StackCheck 
   33 S> 0x2042f1cd117d @    1 : 75                Nop 
   33 E> 0x2042f1cd117e @    2 : 19 02             Ldar a1
   33 E> 0x2042f1cd1180 @    4 : 1f 03 01 fa       LdrKeyedProperty a0, [1], r0
         0x2042f1cd1184 @    8 : 19 fa             Ldar r0
   42 S> 0x2042f1cd1186 @   10 : 75                Nop 
   47 E> 0x2042f1cd1187 @   11 : 19 02             Ldar a1
   44 E> 0x2042f1cd1189 @   13 : 1f 03 03 fa       LdrKeyedProperty a0, [3], r0
         0x2042f1cd118d @   17 : 19 fa             Ldar r0
   56 S> 0x2042f1cd118f @   19 : 75                Nop 
   66 S> 0x2042f1cd1190 @   20 : 67                Return 

So I think we're better with this CL than we were before. I have been trying
hard to have the right logic for source positions in the optimizations, but was
wrong here.

[Yang, 2016-06-09 08:57:28]

On 2016/06/09 08:51:17, oth wrote:
> Thanks Yang. There's an explanation to the issue below. Please see if the
answer
> makes this CL agreeable.
> 
>
https://codereview.chromium.org/2042633002/diff/120001/test/cctest/interprete...
> File test/cctest/interpreter/bytecode_expectations/PropertyLoads.golden
(right):
> 
>
https://codereview.chromium.org/2042633002/diff/120001/test/cctest/interprete...
> test/cctest/interpreter/bytecode_expectations/PropertyLoads.golden:801:
> B(LdrKeyedProperty), R(arg0), U8(5), R(0),
> On 2016/06/09 06:02:46, Yang wrote:
> > Is this correct?
> > 
> > If the keyed property load throws, we will end up with expression position
59
> > instead of 56 now.
> > 
> > Imo we should either drop the position at Ldar but keep the one at
> > LdrKeyedProperty, or change the position at Ldar to 56.
> 
> The issue spotted here is that the old bytecode in this test is wrong. There's
a
> change in this CL to the peephole optimizer for this.
> 
> Before this CL:
> 
> Repro.js:
> 
> function f(a, b) {
>     var c = a[b];
>     c = a[b];
>     return c;
> }
> 
> f({value:1}, "value");
> 
> The unoptimized bytecode looks like:
> 
>    11 E> 0x3fd969ed117c @    0 : 64                StackCheck 
>    33 S> 0x3fd969ed117d @    1 : 19 03             Ldar a0
>          0x3fd969ed117f @    3 : 1a f9             Star r1
>    33 E> 0x3fd969ed1181 @    5 : 19 02             Ldar a1
>          0x3fd969ed1183 @    7 : 1e f9 01          LdaKeyedProperty r1, [1]
>    33 E> 0x3fd969ed1186 @   10 : 1a fa             Star r0
>    42 S> 0x3fd969ed1188 @   12 : 19 03             Ldar a0
>          0x3fd969ed118a @   14 : 1a f9             Star r1
>    47 E> 0x3fd969ed118c @   16 : 19 02             Ldar a1
>          0x3fd969ed118e @   18 : 1e f9 03          LdaKeyedProperty r1, [3]
>    44 E> 0x3fd969ed1191 @   21 : 1a fa             Star r0
>    56 S> 0x3fd969ed1193 @   23 : 19 fa             Ldar r0
>    66 S> 0x3fd969ed1195 @   25 : 67                Return 
> 
> The peephole optimizer has then (erroneously) moved the expression position 44
E
> from a Star bytecode to the LdrKeyedProperty.
> 
>    11 E> 0x2042f1cd117c @    0 : 64                StackCheck 
>    33 S> 0x2042f1cd117d @    1 : 75                Nop 
>    33 E> 0x2042f1cd117e @    2 : 19 02             Ldar a1
>    33 E> 0x2042f1cd1180 @    4 : 1f 03 01 fa       LdrKeyedProperty a0, [1],
r0
>          0x2042f1cd1184 @    8 : 19 fa             Ldar r0
>    42 S> 0x2042f1cd1186 @   10 : 75                Nop 
>    47 E> 0x2042f1cd1187 @   11 : 19 02             Ldar a1
>    44 E> 0x2042f1cd1189 @   13 : 1f 03 03 fa       LdrKeyedProperty a0, [3],
r0
>          0x2042f1cd118d @   17 : 19 fa             Ldar r0
>    56 S> 0x2042f1cd118f @   19 : 75                Nop 
>    66 S> 0x2042f1cd1190 @   20 : 67                Return 
> 
> So I think we're better with this CL than we were before. I have been trying
> hard to have the right logic for source positions in the optimizations, but
was
> wrong here.

I see. LGTM then.

[oth, 2016-06-09 11:36:35]

The CQ bit was checked by oth@chromium.org

[oth, 2016-06-09 11:36:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2042633002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-09 11:36:45]

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

[commit-bot: I haz the power, 2016-06-09 12:03:57]

Description was changed from

==========
[interpreter] Ensure optimizations preserve source positions.

The optimization stages in the bytecode generation pipeline must
preserve source position information. Failure to preserve
source position information could result in single stepping
in the debugger misbehaving or mis-reporting in exception stack traces.

This change adds tests intended to check optimizations do not damage
source position info.

BUG=v8:4280
LOG=N
==========

to

==========
[interpreter] Ensure optimizations preserve source positions.

The optimization stages in the bytecode generation pipeline must
preserve source position information. Failure to preserve
source position information could result in single stepping
in the debugger misbehaving or mis-reporting in exception stack traces.

This change adds tests intended to check optimizations do not damage
source position info.

BUG=v8:4280
LOG=N
==========

[commit-bot: I haz the power, 2016-06-09 12:03:58]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-09 12:04:47]

Description was changed from

==========
[interpreter] Ensure optimizations preserve source positions.

The optimization stages in the bytecode generation pipeline must
preserve source position information. Failure to preserve
source position information could result in single stepping
in the debugger misbehaving or mis-reporting in exception stack traces.

This change adds tests intended to check optimizations do not damage
source position info.

BUG=v8:4280
LOG=N
==========

to

==========
[interpreter] Ensure optimizations preserve source positions.

The optimization stages in the bytecode generation pipeline must
preserve source position information. Failure to preserve
source position information could result in single stepping
in the debugger misbehaving or mis-reporting in exception stack traces.

This change adds tests intended to check optimizations do not damage
source position info.

BUG=v8:4280
LOG=N

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

[commit-bot: I haz the power, 2016-06-09 12:04:48]

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

[xidachen, 2016-06-09 18:10:00]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2055863002/ by xidachen@chromium.org.

The reason for reverting is: causing compile error:
https://build.chromium.org/p/chromium/builders/Win/builds/44180

Bug filed here:
https://bugs.chromium.org/p/chromium/issues/detail?id=618757.