[vm,dyn_modules] Add flags to bytecode source positions.

There are two possible flags for each source position currently:
a flag that marks the source position as synthetic and a flag that
marks the source position as within a yield point.

Synthetic source positions in bytecode are treated the same as synthetic
source positions in compiled code. That is, they encode the source
position in the text that caused them to be synthesized, but denote that
the covered instructions are internal and not to be used for debugger
pause points or for call site/branch coverage information.

Adding these flags allow us to mark appropriate parts of the async
machinery as synthetic, and also allow us to mark all the bytecode
involved in yield points as having the same token position.

The latter fixes tests where the code would step over a previous
expression, thus being paused at the start of the await bytecode,
and would record the fp and token position there as the ones to
ignore. However, since a new source position wasn't emitted until the
direct call to the await method, the recorded token position would
be the token position prior to the await call, and so the change
in token position at the await call would trigger an early pause.

TEST=pkg/vm_service/test/async_single_step_exception_test
     pkg/vm_service/test/async_single_step_into_test
     pkg/vm_service/test/async_single_step_out_test
     pkg/vm_service/test/async_star_single_step_into_test
     pkg/vm_service/test/async_step_out_test
     pkg/vm_service/test/positive_token_pos_test
     pkg/vm_service/test/step_into_async_no_await_test

Cq-Include-Trybots: luci.dart.try:vm-dyn-linux-debug-x64-try,vm-aot-dyn-linux-debug-x64-try,vm-aot-dyn-linux-product-x64-try,vm-dyn-mac-debug-arm64-try,vm-aot-linux-debug-x64-try,vm-aot-linux-product-x64-try
Change-Id: Ic7642a74fb76227a473f461f360e84dd3d5a45a1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/453322
Reviewed-by: Alexander Markov <[email protected]>
Commit-Queue: Tess Strickland <[email protected]>

Author: sstrickl

pkg/dart2bytecode/docs/bytecode.md

@@ -850,9 +850,27 @@ the 'sourcePositions' section of BytecodeFile.
 ```
 type SourcePositions {
   UInt numEntries;
-  // Encoded list of pairs (PC, FileOffset), ordered by PC (offset of bytecode instruction).
+  // Encoded list of pairs (PC, encodedFileOffset), ordered by PC
+  // (offset of bytecode instruction).
+  //
+  // An encodedFileOffset encodes a fileOffset and a set of flags.
+  //
+  // If the encodedFileOffset is non-negative, then the encodedFileOffset
+  // is the same numeric value as the fileOffset.
+  //
+  // If the encodedFileOffset is -1, then there is no source position
+  // associated with the given PC.
+  //
+  // Otherwise, encodedFileOffset = -((fileOffset << n) | flags) -1,
+  // where n is the number of flags below:
+  // - synthetic (bit 0): set if the source position corresponds to
+  //   synthetic code (e.g., a source position that should not be used
+  //   for debugger breakpoints or pausing or for code coverage).
+  // - yieldPoint (bit 1): set if the source position corresponds to
+  //   bytecode that implements a yield point.
+  //
   // For two consecutive entries (PC1, FileOffset1), (PC2, FileOffset2) all
-  // bytecode instructions in range [PC1,PC2) correspond to a source
+  // bytecode instructions in range [PC1,PC2) correspond to an encoded source
   // position FileOffset1.
   SourcePositionEntry[numEntries] entries
 }

pkg/dart2bytecode/lib/assembler.dart

@@ -63,6 +63,7 @@ class BytecodeAssembler {
   final bool _emitSourcePositions;
   bool isUnreachable = false;
   int currentSourcePosition = TreeNode.noOffset;
+  int currentSourcePositionFlags = 0;
 
   BytecodeAssembler(BytecodeOptions options)
       : _emitSourcePositions = options.emitSourcePositions;
@@ -81,12 +82,22 @@ class BytecodeAssembler {
     }
   }
 
+  @pragma('vm:prefer-inline')
+  void _emitSourcePosition() {
+    if (_emitSourcePositions && !isUnreachable) {
+      sourcePositions.add(
+          offset,
+          currentSourcePosition == TreeNode.noOffset
+              ? SourcePositions.noSourcePosition
+              : currentSourcePosition,
+          currentSourcePositionFlags);
+    }
+  }
+
   @pragma('vm:prefer-inline')
   void emitSourcePosition() {
-    if (_emitSourcePositions &&
-        !isUnreachable &&
-        currentSourcePosition != TreeNode.noOffset) {
-      sourcePositions.add(offset, currentSourcePosition);
+    if (currentSourcePosition != TreeNode.noOffset) {
+      _emitSourcePosition();
     }
   }
 
@@ -95,15 +106,7 @@ class BytecodeAssembler {
   // source position to distinguish these calls and avoid stopping at them
   // while single stepping.
   @pragma('vm:prefer-inline')
-  void emitSourcePositionForCall() {
-    if (_emitSourcePositions && !isUnreachable) {
-      sourcePositions.add(
-          offset,
-          currentSourcePosition == TreeNode.noOffset
-              ? SourcePositions.syntheticCodeMarker
-              : currentSourcePosition);
-    }
-  }
+  void emitSourcePositionForCall() => _emitSourcePosition();
 
   void _grow() {
     final newSize = _buffer.length << 1;

pkg/dart2bytecode/lib/bytecode_generator.dart

@@ -1021,8 +1021,11 @@ class BytecodeGenerator extends RecursiveVisitor {
   // variable 'foo' has type 'dynamic'.
   late final implicitCallName = Name('implicit:call');
 
-  void _recordSourcePosition(int fileOffset) {
+  void _recordSourcePosition(int fileOffset, [int? flags]) {
     asm.currentSourcePosition = fileOffset;
+    if (flags != null) {
+      asm.currentSourcePositionFlags = flags;
+    }
     maxSourcePosition = math.max(maxSourcePosition, fileOffset);
   }
 
@@ -1031,9 +1034,11 @@ class BytecodeGenerator extends RecursiveVisitor {
       return;
     }
     final savedSourcePosition = asm.currentSourcePosition;
-    _recordSourcePosition(node.fileOffset);
+    final savedFlags = asm.currentSourcePositionFlags;
+    _recordSourcePosition(node.fileOffset, 0);
     node.accept(this);
     asm.currentSourcePosition = savedSourcePosition;
+    asm.currentSourcePositionFlags = savedFlags;
   }
 
   void _generateNodeList(List<TreeNode> nodes) {
@@ -1216,6 +1221,9 @@ class BytecodeGenerator extends RecursiveVisitor {
           break;
       }
       if (returnMethod != null) {
+        // Unlike other async machinery, this can't be marked synthetic
+        // as the method may return directly from the direct call and so
+        // the debugger needs to pause at it, not the following return.
         asm.emitPopLocal(locals.returnVarIndexInFrame);
         asm.emitPush(locals.suspendStateVarIndexInFrame);
         asm.emitPush(locals.returnVarIndexInFrame);
@@ -1633,17 +1641,19 @@ class BytecodeGenerator extends RecursiveVisitor {
     savedAssemblers = null;
     currentLoopDepth = 0;
     savedMaxSourcePositions = <int>[];
-    if (node is Procedure) {
-      maxSourcePosition = node.fileStartOffset;
-    } else if (node is Constructor) {
-      maxSourcePosition = node.startFileOffset;
-    } else {
-      maxSourcePosition = node.fileOffset;
-    }
 
     locals = new LocalVariables(node, options, staticTypeContext);
     locals.enterScope(node);
 
+    final int startPosition;
+    if (node is Procedure) {
+      startPosition = node.fileStartOffset;
+    } else if (node is Constructor) {
+      startPosition = node.startFileOffset;
+    } else {
+      startPosition = node.fileOffset;
+    }
+    _recordSourcePosition(startPosition, SourcePositions.syntheticFlag);
     _genPrologue(node, node.function);
     _setupInitialContext(node.function);
     _emitFirstDebugCheck(node, node.function);
@@ -1677,6 +1687,9 @@ class BytecodeGenerator extends RecursiveVisitor {
     if (!locals.isSuspendableFunction) {
       return;
     }
+
+    final savedFlags = asm.currentSourcePositionFlags;
+    asm.currentSourcePositionFlags |= SourcePositions.syntheticFlag;
     Procedure initMethod;
     switch (function!.dartAsyncMarker) {
       case AsyncMarker.Async:
@@ -1696,6 +1709,10 @@ class BytecodeGenerator extends RecursiveVisitor {
     asm.emitPopLocal(locals.suspendStateVarIndexInFrame);
 
     if (function.dartAsyncMarker != AsyncMarker.Async) {
+      final savedFlags = asm.currentSourcePositionFlags;
+      // Mark all of the code in this block as within the yield point.
+      asm.currentSourcePositionFlags |= SourcePositions.yieldPointFlag;
+      asm.emitSourcePositionForCall();
       // Suspend async* and sync* functions after prologue is finished.
       Label done = Label();
       asm.emitSuspend(done);
@@ -1710,6 +1727,7 @@ class BytecodeGenerator extends RecursiveVisitor {
 
       asm.bind(done);
       asm.emitDrop1(); // Discard result of Suspend.
+      asm.currentSourcePositionFlags = savedFlags;
     }
 
     if (function.dartAsyncMarker == AsyncMarker.SyncStar &&
@@ -1729,6 +1747,7 @@ class BytecodeGenerator extends RecursiveVisitor {
       asyncTryBlock.needsStackTrace = true;
       asyncTryBlock.types.add(cp.addType(const DynamicType()));
     }
+    asm.currentSourcePositionFlags = savedFlags;
   }
 
   void _endSuspendableFunction(FunctionNode? function) {
@@ -1744,6 +1763,8 @@ class BytecodeGenerator extends RecursiveVisitor {
       // Exception handlers are reachable although there are no labels or jumps.
       asm.isUnreachable = false;
 
+      final savedFlags = asm.currentSourcePositionFlags;
+      asm.currentSourcePositionFlags |= SourcePositions.syntheticFlag;
       asm.emitSetFrame(locals.frameSize);
 
       final rethrowException = Label();
@@ -1765,6 +1786,7 @@ class BytecodeGenerator extends RecursiveVisitor {
       asm.emitMoveSpecial(SpecialIndex.stackTrace, temp);
       asm.emitPush(temp);
       asm.emitThrow(1);
+      asm.currentSourcePositionFlags = savedFlags;
     }
   }
 
@@ -1954,9 +1976,11 @@ class BytecodeGenerator extends RecursiveVisitor {
       }
     }
 
+    // The CheckStack below is the instruction which should be used for function
+    // entry breakpoints.
+    _recordInitialSourcePositionForFunction(node, function);
     // CheckStack must see a properly initialized context when stress-testing
     // stack trace collection.
-    _recordInitialSourcePositionForFunction(node, function);
     asm.emitCheckStack(0);
 
     if (locals.hasFunctionTypeArgsVar && isClosure) {
@@ -2063,7 +2087,8 @@ class BytecodeGenerator extends RecursiveVisitor {
 
   void _recordInitialSourcePositionForFunction(
       TreeNode node, FunctionNode? function) {
-    _recordSourcePosition(_initialSourcePositionForFunction(node, function));
+    final position = _initialSourcePositionForFunction(node, function);
+    _recordSourcePosition(position, 0);
   }
 
   void _emitFirstDebugCheck(TreeNode node, FunctionNode? function) {
@@ -2417,7 +2442,7 @@ class BytecodeGenerator extends RecursiveVisitor {
 
     final int closureFunctionIndex = cp.addClosureFunction(closureIndex);
 
-    maxSourcePosition = math.max(maxSourcePosition, function.fileOffset);
+    _recordSourcePosition(function.fileOffset, SourcePositions.syntheticFlag);
     _genPrologue(node, function);
     _setupInitialContext(function);
     _emitFirstDebugCheck(node, function);
@@ -2695,7 +2720,8 @@ class BytecodeGenerator extends RecursiveVisitor {
     if (options.emitSourcePositions) {
       asm.emitSourcePosition();
     }
-    if (options.emitDebuggerStops) {
+    if (options.emitDebuggerStops &&
+        (asm.currentSourcePositionFlags & SourcePositions.syntheticFlag) == 0) {
       asm.emitDebugCheck();
     }
   }
@@ -3413,8 +3439,7 @@ class BytecodeGenerator extends RecursiveVisitor {
     tryCatches![tryCatch]!.needsStackTrace = true;
 
     // Allow breakpoint on explicit rethrow statement.
-    _emitSourcePosition();
-    _genRethrow(tryCatch);
+    _genRethrow(tryCatch, isSynthetic: false);
   }
 
   bool _hasNonTrivialInitializer(Field field) {
@@ -4147,7 +4172,13 @@ class BytecodeGenerator extends RecursiveVisitor {
     }
   }
 
-  void _genRethrow(TreeNode node) {
+  void _genRethrow(TreeNode node, {required bool isSynthetic}) {
+    final savedFlags = asm.currentSourcePositionFlags;
+    if (isSynthetic) {
+      asm.currentSourcePositionFlags |= SourcePositions.syntheticFlag;
+    } else {
+      asm.currentSourcePositionFlags &= ~SourcePositions.syntheticFlag;
+    }
     final capturedExceptionVar = locals.capturedExceptionVar(node);
     if (capturedExceptionVar != null) {
       assert(locals.isCaptured(capturedExceptionVar));
@@ -4165,6 +4196,7 @@ class BytecodeGenerator extends RecursiveVisitor {
     }
 
     asm.emitThrow(1);
+    asm.currentSourcePositionFlags = savedFlags;
   }
 
   @override
@@ -4241,7 +4273,7 @@ class BytecodeGenerator extends RecursiveVisitor {
 
     if (!hasCatchAll) {
       tryBlock.needsStackTrace = true;
-      _genRethrow(node);
+      _genRethrow(node, isSynthetic: true);
     }
 
     asm.bind(done);
@@ -4277,7 +4309,7 @@ class BytecodeGenerator extends RecursiveVisitor {
     _generateNode(node.finalizer);
 
     tryBlock.needsStackTrace = true; // For rethrowing.
-    _genRethrow(node);
+    _genRethrow(node, isSynthetic: true);
 
     for (var finallyBlock in finallyBlocks[node]!) {
       asm.bind(finallyBlock.entry);
@@ -4457,6 +4489,14 @@ class BytecodeGenerator extends RecursiveVisitor {
   void visitAwaitExpression(AwaitExpression node) {
     _generateNode(node.operand);
 
+    // The rest of the await expression bytecode is the yield point.
+    // Emit a source position at the start to ensure that if the debugger
+    // is paused anywhere in this bytecode, a request to step over the await
+    // doesn't pause at either the direct call or the return, which have the
+    // same source position information.
+    final savedFlags = asm.currentSourcePositionFlags;
+    asm.currentSourcePositionFlags |= SourcePositions.yieldPointFlag;
+    asm.emitSourcePositionForCall();
     final int temp = locals.tempIndexInFrame(node);
     asm.emitPopLocal(temp);
 
@@ -4478,6 +4518,7 @@ class BytecodeGenerator extends RecursiveVisitor {
       _genDirectCall(_await, objectTable.getArgDescHandle(2), 2);
     }
     asm.emitReturnTOS();
+    asm.currentSourcePositionFlags = savedFlags;
 
     asm.bind(done);
   }