[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

Review URL: https://codereview.chromium.org/1801473003

Cr-Commit-Position: refs/heads/master@{#34771}

src/interpreter/bytecode-array-builder.cc

@@ -1197,8 +1197,9 @@ size_t BytecodeArrayBuilder::GetConstantPoolEntry(Handle<Object> object) {
 void BytecodeArrayBuilder::SetReturnPosition() {
   if (return_position_ == RelocInfo::kNoPosition) return;
   if (exit_seen_in_block_) return;
-  source_position_table_builder_.AddStatementPosition(bytecodes_.size(),
-                                                      return_position_);
+  source_position_table_builder_.AddStatementPosition(
+      bytecodes_.size(), return_position_,
+      SourcePositionTableBuilder::OVERWRITE_DUPLICATE);
 }
 
 void BytecodeArrayBuilder::SetStatementPosition(Statement* stmt) {

src/interpreter/source-position-table.cc

@@ -115,10 +115,11 @@ void DecodeEntry(ByteArray* bytes, int* index, PositionTableEntry* entry) {
 
 }  // namespace
 
-void SourcePositionTableBuilder::AddStatementPosition(size_t bytecode_offset,
-                                                      int source_position) {
+void SourcePositionTableBuilder::AddStatementPosition(
+    size_t bytecode_offset, int source_position,
+    SourcePositionTableBuilder::OnDuplicateCodeOffset on_duplicate) {
   int offset = static_cast<int>(bytecode_offset);
-  AddEntry({offset, source_position, true});
+  AddEntry({offset, source_position, true}, on_duplicate);
   LOG_CODE_EVENT(isolate_, CodeLinePosInfoAddStatementPositionEvent(
                                jit_handler_data_, offset, source_position));
   LOG_CODE_EVENT(isolate_, CodeLinePosInfoAddPositionEvent(
@@ -133,24 +134,34 @@ void SourcePositionTableBuilder::AddExpressionPosition(size_t bytecode_offset,
                                jit_handler_data_, offset, source_position));
 }
 
-void SourcePositionTableBuilder::AddEntry(const PositionTableEntry& entry) {
+void SourcePositionTableBuilder::AddEntry(
+    const PositionTableEntry& entry,
+    SourcePositionTableBuilder::OnDuplicateCodeOffset on_duplicate) {
   // Don't encode a new entry if this bytecode already has a source position
   // assigned.
-  if (bytes_.size() > 0 && previous_.bytecode_offset == entry.bytecode_offset) {
+  if (candidate_.bytecode_offset == entry.bytecode_offset) {
+    if (on_duplicate == OVERWRITE_DUPLICATE) candidate_ = entry;
     return;
   }
 
-  PositionTableEntry tmp(entry);
+  CommitEntry();
+  candidate_ = entry;
+}
+
+void SourcePositionTableBuilder::CommitEntry() {
+  if (candidate_.bytecode_offset == kUninitializedCandidateOffset) return;
+  PositionTableEntry tmp(candidate_);
   SubtractFromEntry(tmp, previous_);
   EncodeEntry(bytes_, tmp);
-  previous_ = entry;
+  previous_ = candidate_;
 
 #ifdef ENABLE_SLOW_DCHECKS
-  raw_entries_.push_back(entry);
+  raw_entries_.push_back(candidate_);
 #endif
 }
 
 Handle<ByteArray> SourcePositionTableBuilder::ToSourcePositionTable() {
+  CommitEntry();
   if (bytes_.empty()) return isolate_->factory()->empty_byte_array();
 
   Handle<ByteArray> table = isolate_->factory()->NewByteArray(

src/interpreter/source-position-table.h

@@ -36,28 +36,40 @@ struct PositionTableEntry {
 
 class SourcePositionTableBuilder : public PositionsRecorder {
  public:
+  enum OnDuplicateCodeOffset {
+    DISCARD_DUPLICATE,
+    OVERWRITE_DUPLICATE,
+  };
+
   explicit SourcePositionTableBuilder(Isolate* isolate, Zone* zone)
       : isolate_(isolate),
         bytes_(zone),
 #ifdef ENABLE_SLOW_DCHECKS
         raw_entries_(zone),
 #endif
-        previous_() {
+        candidate_(kUninitializedCandidateOffset, 0, false) {
   }
 
-  void AddStatementPosition(size_t bytecode_offset, int source_position);
+  void AddStatementPosition(
+      size_t bytecode_offset, int source_position,
+      OnDuplicateCodeOffset on_duplicate = DISCARD_DUPLICATE);
   void AddExpressionPosition(size_t bytecode_offset, int source_position);
   Handle<ByteArray> ToSourcePositionTable();
 
  private:
-  void AddEntry(const PositionTableEntry& entry);
+  static const int kUninitializedCandidateOffset = -1;
+
+  void AddEntry(const PositionTableEntry& entry,
+                OnDuplicateCodeOffset on_duplicate = DISCARD_DUPLICATE);
+  void CommitEntry();
 
   Isolate* isolate_;
   ZoneVector<byte> bytes_;
 #ifdef ENABLE_SLOW_DCHECKS
   ZoneVector<PositionTableEntry> raw_entries_;
 #endif
-  PositionTableEntry previous_;
+  PositionTableEntry candidate_;  // Next entry to be written, if initialized.
+  PositionTableEntry previous_;   // Previously written entry, to compute delta.
 };
 
 class SourcePositionTableIterator {

test/mjsunit/ignition/elided-instruction-no-ignition.js

@@ -0,0 +1,37 @@
+// Copyright 2016 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --expose-debug-as debug
+
+Debug = debug.Debug
+
+var exception = null;
+var break_count = 0;
+
+function listener(event, exec_state, event_data, data) {
+  if (event != Debug.DebugEvent.Break) return;
+  try {
+    print(event_data.sourceLineText());
+    var column = event_data.sourceColumn();
+    assertTrue(event_data.sourceLineText().indexOf(
+        `Break ${break_count++}. ${column}.`) > 0);
+    exec_state.prepareStep(Debug.StepAction.StepIn);
+  } catch (e) {
+    print(e + e.stack);
+    exception = e;
+  }
+};
+
+function f() {
+  var a = 1;                     // Break 2. 10.
+  return a;                      // Break 3. 2.
+}                                // Break 4. 0.
+
+Debug.setListener(listener);
+debugger;                        // Break 0. 0.
+f();                             // Break 1. 0.
+Debug.setListener(null);         // Break 5. 0.
+
+assertNull(exception);
+assertEquals(6, break_count);

test/mjsunit/ignition/elided-instruction.js

@@ -0,0 +1,41 @@
+// Copyright 2016 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --expose-debug-as debug
+
+Debug = debug.Debug
+
+var exception = null;
+var break_count = 0;
+
+function listener(event, exec_state, event_data, data) {
+  if (event != Debug.DebugEvent.Break) return;
+  try {
+    print(event_data.sourceLineText());
+    var column = event_data.sourceColumn();
+    assertTrue(event_data.sourceLineText().indexOf(
+        `Break ${break_count++}. ${column}.`) > 0);
+    exec_state.prepareStep(Debug.StepAction.StepIn);
+  } catch (e) {
+    print(e + e.stack);
+    exception = e;
+  }
+};
+
+function f() {
+  var a = 1;                     // Break 2. 10.
+  // This return statement emits no bytecode instruction for the evaluation of
+  // the to-be-returned expression. Therefore we cannot set a break location
+  // before the statement and a second break location immediately before
+  // returning to the caller.
+  return a;
+}                                // Break 3. 0.
+
+Debug.setListener(listener);
+debugger;                        // Break 0. 0.
+f();                             // Break 1. 0.
+Debug.setListener(null);         // Break 4. 0.
+
+assertNull(exception);
+assertEquals(5, break_count);

test/mjsunit/mjsunit.status

@@ -807,6 +807,9 @@
   # TODO(mythria, 4680): possibly problem with line numbers.
   'es6/regress/regress-468661': [FAIL],
 
+  # Debugger test cases that pass with ignition, but not full-codegen.
+  # These differences between full-codegen and ignition are deliberate.
+  'ignition/elided-instruction-no-ignition': [FAIL],
 }],  # ignition == True
 
 ['ignition == True and arch == arm64', {
@@ -823,6 +826,12 @@
   'regress/regress-1257': [SKIP],
 }],  # ignition == True and arch == arm
 
+['ignition == False', {
+  # Debugger test cases that pass with full-codegen, but not ignition.
+  # These differences between full-codegen and ignition are deliberate.
+  'ignition/elided-instruction': [FAIL],
+}],  # ignition == False
+
 ##############################################################################
 ['gcov_coverage', {
   # Tests taking too long.

test/unittests/interpreter/source-position-table-unittest.cc

@@ -33,6 +33,26 @@ TEST_F(SourcePositionTableTest, EncodeStatement) {
   CHECK(!builder.ToSourcePositionTable().is_null());
 }
 
+TEST_F(SourcePositionTableTest, EncodeStatementDuplicates) {
+  SourcePositionTableBuilder builder(isolate(), zone());
+  for (int i = 0; i < arraysize(offsets); i++) {
+    builder.AddStatementPosition(offsets[i], offsets[i]);
+    builder.AddStatementPosition(
+        offsets[i], offsets[i],
+        (i % 2 == 0) ? SourcePositionTableBuilder::OVERWRITE_DUPLICATE
+                     : SourcePositionTableBuilder::DISCARD_DUPLICATE);
+    builder.AddStatementPosition(offsets[i], offsets[i]);
+    builder.AddStatementPosition(
+        offsets[i], offsets[i],
+        (i % 2 == 0) ? SourcePositionTableBuilder::OVERWRITE_DUPLICATE
+                     : SourcePositionTableBuilder::DISCARD_DUPLICATE);
+  }
+
+  // To test correctness, we rely on the assertions in ToSourcePositionTable().
+  // (Also below.)
+  CHECK(!builder.ToSourcePositionTable().is_null());
+}
+
 TEST_F(SourcePositionTableTest, EncodeExpression) {
   SourcePositionTableBuilder builder(isolate(), zone());
   for (int i = 0; i < arraysize(offsets); i++) {