[vm,dyn_modules] Add Nops as needed to avoid overwriting source positions.

Previously, the onus was on the BytecodeGenerator to emit Nops in cases
where a source position may be overwritten by the next emitted
instruction.

Instead, change SourcePositions.add to return whether or not the
requested mapping would overwrite an existing one. With that,
Assembler.emitSourcePosition[ForCall] now emits Nop instructions if
needed to ensure that the requested source position is recorded.

TEST=pkg/vm_service/test/set_sdk_library_debuggable_test

Cq-Include-Trybots: luci.dart.try:vm-dyn-linux-debug-x64-try
Change-Id: Ieebef0b59dbee3c21e598acfc45c5b47a7c19543
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/484100
Commit-Queue: Tess Strickland <sstrickl@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>

pkg/dart2bytecode/lib/assembler.dart

@@ -84,13 +84,20 @@ class BytecodeAssembler {
 
   @pragma('vm:prefer-inline')
   void _emitSourcePosition() {
+    final position = currentSourcePosition == TreeNode.noOffset
+        ? SourcePositions.noSourcePosition
+        : currentSourcePosition;
     if (_emitSourcePositions && !isUnreachable) {
-      sourcePositions.add(
-          offset,
-          currentSourcePosition == TreeNode.noOffset
-              ? SourcePositions.noSourcePosition
-              : currentSourcePosition,
-          currentSourcePositionFlags);
+      bool added =
+          sourcePositions.add(offset, position, currentSourcePositionFlags);
+      if (!added) {
+        // There's already an entry for the current PC, so emit a Nop instruction
+        // to provide a new pc offset for the requested source position.
+        _emitNop();
+        added =
+            sourcePositions.add(offset, position, currentSourcePositionFlags);
+        assert(added);
+      }
     }
   }
 
@@ -707,14 +714,11 @@ class BytecodeAssembler {
     _emitInstructionA(Opcode.kCheckStack, ra);
   }
 
+  // Nops are never created by the bytecode generator, but rather by
+  // the assembler in order to provide unique pc offsets for source
+  // positions.
   @pragma('vm:prefer-inline')
-  void emitNop() {
-    // Currently, the only reason the bytecode generator emits a no-op
-    // instruction is when the next instruction to emit is unknown (e.g.,
-    // at the end of a visit<X> method), but a source position must be
-    // emitted after the last known instruction and not overwritten by
-    // the next emitted instruction.
-    emitSourcePosition();
+  void _emitNop() {
     _emitInstruction0(Opcode.kNop);
   }
 

pkg/dart2bytecode/lib/bytecode_generator.dart

@@ -3591,11 +3591,10 @@ class BytecodeGenerator extends RecursiveVisitor {
     _genDirectCallWithArgs(target, args,
         isFactory: target.isFactory, node: node);
     if (target == debugger) {
-      // The debugger needs a pause right after stepping out from the debugger
-      // function. Just using asm.emitSourcePosition() won't work here, because
-      // the next emitted instruction may have its own source position
-      // and thus would overwrite that one.
-      asm.emitNop();
+      // The debugger needs a pause for the current source position right after
+      // stepping out from the debugger function.
+      assert(asm.currentSourcePosition != TreeNode.noOffset);
+      asm.emitSourcePosition();
     }
   }
 

pkg/dart2bytecode/lib/source_positions.dart

@@ -44,8 +44,10 @@ class SourcePositions extends BytecodeDeclaration {
     return (value >> _numFlags, value & _flagMask);
   }
 
-  // Maps the given PC to the given file offset. If there is already an entry
-  // for the given PC, then the new information overwrites the old information.
+  // Adds a mapping from the PC to the given file offset as long as there's
+  // no mapping for that PC already, otherwise no change is made. Returns
+  // whether the requested mapping exists, which can be either because a new
+  // mapping was created or the mapping already existed before the request.
   //
   // Marks the source position as synthetic (not to be used by the debugger
   // or coverage calculations) if [(flags & syntheticFlag) != 0].
@@ -54,7 +56,7 @@ class SourcePositions extends BytecodeDeclaration {
   //
   // Assumes that the pc is greater than or equal to the pc used in the most
   // recent call to add, if any.
-  void add(int pc, int fileOffset, int flags) {
+  bool add(int pc, int fileOffset, int flags) {
     assert(fileOffset >= 0 || fileOffset == noSourcePosition);
     assert((flags & ~_flagMask) == 0);
     if (_lastPcAdded > pc) {
@@ -67,16 +69,18 @@ class SourcePositions extends BytecodeDeclaration {
       final lastPc = _positions[i];
       final lastFileOffset = _positions[i + 1];
       if (lastFileOffset == encodedFileOffset) {
-        // The last entry covers this PC offset as well.
-        return;
+        // The last entry covers this PC offset as well, or this is a repeated
+        // request for the same (pc, offset) mapping.
+        return true;
       }
       if (lastPc == pc) {
-        // The new entry for this PC overwrites the old one.
-        _positions.removeRange(i, i + 2);
+        // There's already a mapping for (pc, lastFileOffset).
+        return false;
       }
     }
     _positions.add(pc);
     _positions.add(encodedFileOffset);
+    return true;
   }
 
   bool get isEmpty => _positions.isEmpty;