[interpreter] Ensure optimizations preserve source positions.

src/interpreter/bytecode-peephole-optimizer.cc

Index: src/interpreter/bytecode-peephole-optimizer.cc
diff --git a/src/interpreter/bytecode-peephole-optimizer.cc b/src/interpreter/bytecode-peephole-optimizer.cc
index 790b270abddf3cccb90ba3a6b834c1a87795891f..4e0b58c9b09315f6a87f201550cbaba0e48c8d47 100644
--- a/src/interpreter/bytecode-peephole-optimizer.cc
+++ b/src/interpreter/bytecode-peephole-optimizer.cc
@@ -173,6 +173,12 @@ namespace {
 void TransformLdaStarToLdrLdar(Bytecode new_bytecode, BytecodeNode* const last,
                                BytecodeNode* const current) {
   DCHECK_EQ(current->bytecode(), Bytecode::kStar);
+  if (current->source_info().is_statement() &&
+      last->source_info().is_statement()) {

[rmcilroy, 2016/06/07 09:46:10]

As discussed offline, I think we need to not do this if
current->source_info().is_statement() (irrespective of last's source position).

[oth, 2016/06/07 13:46:17]

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

+    // The transform won't maintain causal ordering for the debugger.
+    return;

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

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?

[oth, 2016/06/07 13:46:17]

Done.
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.

+  }
+
   //
   // An example transformation here would be:
   //
@@ -188,12 +194,11 @@ void TransformLdaStarToLdrLdar(Bytecode new_bytecode, BytecodeNode* const last,
                         current->operand_scale());
 
   // If there was a source position on |current| transfer it to the
-  // updated |last| to maintain the debugger's causal view. ie. if an
-  // expression position LdrGlobal is the bytecode that could throw
-  // and if a statement position it needs to be placed before the
-  // store to R occurs.
-  last->source_info().Update(current->source_info());
-  current->source_info().set_invalid();
+  // updated |last| to maintain the debugger's causal view.
+  if (last->source_info().is_statement()) {
+    last->source_info().Update(current->source_info());
+    current->source_info().set_invalid();
+  }

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

As discussed offline - I don't think this block does anything any longer, we
should probably just remove it.

[oth, 2016/06/07 13:46:17]

Done.

 }
 
 }  // namespace