[vm] Don't touch the PCMarker slot in the Dart profiler walker.

rmacnak-google · commit-bot@chromium.org · commit 499b5b23e1fb · 2018-12-07T20:48:49.000Z
- The PCMarker slot may be uninitialized (okay, a few skipped samples) and on a guard page (not okay, crash). - With the upcoming "bare instructions" mode, some Dart frames will lack a PC marker slot. Instead, add a sentinel value to the entry frame *above* its FP. Change-Id: I70d737a6f2f76febe61fe774932127f5274565b1 Reviewed-on: https://dart-review.googlesource.com/c/86502 Commit-Queue: Ryan Macnak <rmacnak@google.com> Reviewed-by: Régis Crelier <regis@google.com>
diff --git a/runtime/vm/interpreter.cc b/runtime/vm/interpreter.cc
@@ -3019,11 +3019,6 @@ void Interpreter::JumpToFrame(uword pc, uword sp, uword fp, Thread* thread) {
   // in the previous C++ frames.
   StackResource::Unwind(thread);
 
-  // Set the tag.
-  thread->set_vm_tag(VMTag::kDartInterpretedTagId);
-  // Clear top exit frame.
-  thread->set_top_exit_frame_info(0);
-
   fp_ = reinterpret_cast<RawObject**>(fp);
 
   if (pc == StubCode::RunExceptionHandler().EntryPoint()) {
@@ -3041,6 +3036,11 @@ void Interpreter::JumpToFrame(uword pc, uword sp, uword fp, Thread* thread) {
     pc_ = pc;
   }
 
+  // Set the tag.
+  thread->set_vm_tag(VMTag::kDartInterpretedTagId);
+  // Clear top exit frame.
+  thread->set_top_exit_frame_info(0);
+
   buf->Longjmp();
   UNREACHABLE();
 }
diff --git a/runtime/vm/profiler.cc b/runtime/vm/profiler.cc
@@ -515,11 +515,8 @@ class ProfilerDartStackWalker : public ProfilerStackWalker {
   ProfilerDartStackWalker(Thread* thread,
                           Sample* sample,
                           SampleBuffer* sample_buffer,
-                          uword stack_lower,
-                          uword stack_upper,
                           uword pc,
                           uword fp,
-                          uword sp,
                           bool allocation_sample,
                           intptr_t skip_count = 0)
       : ProfilerStackWalker((thread->isolate() != NULL)
@@ -530,10 +527,7 @@ class ProfilerDartStackWalker : public ProfilerStackWalker {
                             skip_count),
         thread_(thread),
         pc_(reinterpret_cast<uword*>(pc)),
-        fp_(reinterpret_cast<uword*>(fp)),
-        sp_(reinterpret_cast<uword*>(sp)),
-        stack_upper_(stack_upper),
-        stack_lower_(stack_lower) {}
+        fp_(reinterpret_cast<uword*>(fp)) {}
 
   bool IsInterpretedFrame(uword* fp) {
 #if defined(DART_PRECOMPILED_RUNTIME)
@@ -600,6 +594,19 @@ class ProfilerDartStackWalker : public ProfilerStackWalker {
 
     sample_->set_exit_frame_sample(has_exit_frame);
 
+    if (!has_exit_frame && !in_interpreted_frame &&
+        (CallerPC(in_interpreted_frame) == EntryMarker(in_interpreted_frame))) {
+      // During the prologue of a function, CallerPC will return the caller's
+      // caller. For most frames, the missing PC will be added during profile
+      // processing. However, during this stack walk, it can cause us to fail
+      // to identify the entry frame and lead the stack walk into the weeds.
+      // Do not continue the stalk walk since this might be a false positive
+      // from a Smi or unboxed value.
+      RELEASE_ASSERT(!has_exit_frame);
+      sample_->set_ignore_sample(true);
+      return;
+    }
+
     for (;;) {
       // Skip entry frame.
       if (StubCode::InInvocationStub(reinterpret_cast<uword>(pc_),
@@ -620,20 +627,6 @@ class ProfilerDartStackWalker : public ProfilerStackWalker {
                                                    in_interpreted_frame));
       }
 
-#if !defined(TARGET_ARCH_DBC)
-      RawCode* marker = PCMarker(in_interpreted_frame);
-      if (marker == StubCode::InvokeDartCode().raw() ||
-          marker == StubCode::InvokeDartCodeFromBytecode().raw()) {
-        // During the prologue of a function, CallerPC will return the caller's
-        // caller. For most frames, the missing PC will be added during profile
-        // processing. However, during this stack walk, it can cause us to fail
-        // to identify the entry frame and lead the stack walk into the weeds.
-        RELEASE_ASSERT(!has_exit_frame);
-        sample_->set_ignore_sample(true);
-        return;
-      }
-#endif
-
       if (!Append(reinterpret_cast<uword>(pc_))) {
         break;  // Sample is full.
       }
@@ -648,14 +641,6 @@ class ProfilerDartStackWalker : public ProfilerStackWalker {
   }
 
  private:
-  uword InitialReturnAddress() const {
-    ASSERT(sp_ != NULL);
-    // MSan/ASan are unaware of frames initialized by generated code.
-    MSAN_UNPOISON(sp_, kWordSize);
-    ASAN_UNPOISON(sp_, kWordSize);
-    return *(sp_);
-  }
-
   uword* CallerPC(bool interp) const {
     ASSERT(fp_ != NULL);
     uword* caller_pc_ptr =
@@ -676,20 +661,6 @@ class ProfilerDartStackWalker : public ProfilerStackWalker {
     return reinterpret_cast<uword*>(*caller_fp_ptr);
   }
 
-  RawCode* PCMarker(bool interp) const {
-    ASSERT(fp_ != NULL);
-    if (interp) {
-      // We don't need this extra check for the interpreter because its frame
-      // build is atomic from the profiler's point of view.
-      return NULL;
-    }
-    uword* pc_marker_ptr = fp_ + kPcMarkerSlotFromFp;
-    // MSan/ASan are unaware of frames initialized by generated code.
-    MSAN_UNPOISON(pc_marker_ptr, kWordSize);
-    ASAN_UNPOISON(pc_marker_ptr, kWordSize);
-    return reinterpret_cast<RawCode*>(*pc_marker_ptr);
-  }
-
   uword* ExitLink(bool interp) const {
     ASSERT(fp_ != NULL);
     uword* exit_link_ptr =
@@ -700,23 +671,21 @@ class ProfilerDartStackWalker : public ProfilerStackWalker {
     return reinterpret_cast<uword*>(*exit_link_ptr);
   }
 
-  bool ValidFramePointer() const { return ValidFramePointer(fp_); }
-
-  bool ValidFramePointer(uword* fp) const {
-    if (fp == NULL) {
-      return false;
-    }
-    uword cursor = reinterpret_cast<uword>(fp);
-    cursor += sizeof(fp);
-    return (cursor >= stack_lower_) && (cursor < stack_upper_);
+  // Note because of stack guards, it is important that this marker lives
+  // above FP.
+  uword* EntryMarker(bool interp) const {
+    ASSERT(!interp);
+    ASSERT(fp_ != NULL);
+    uword* entry_marker_ptr = fp_ + kSavedCallerPcSlotFromFp + 1;
+    // MSan/ASan are unaware of frames initialized by generated code.
+    MSAN_UNPOISON(entry_marker_ptr, kWordSize);
+    ASAN_UNPOISON(entry_marker_ptr, kWordSize);
+    return reinterpret_cast<uword*>(*entry_marker_ptr);
   }
 
   Thread* const thread_;
   uword* pc_;
   uword* fp_;
-  uword* sp_;
-  const uword stack_upper_;
-  const uword stack_lower_;
 };
 
 // If the VM is compiled without frame pointers (which is the default on
@@ -1218,8 +1187,7 @@ void Profiler::SampleAllocation(Thread* thread, intptr_t cid) {
     native_stack_walker.walk();
   } else if (exited_dart_code) {
     ProfilerDartStackWalker dart_exit_stack_walker(
-        thread, sample, sample_buffer, stack_lower, stack_upper, pc, fp, sp,
-        /* allocation_sample*/ true);
+        thread, sample, sample_buffer, pc, fp, /* allocation_sample*/ true);
     dart_exit_stack_walker.walk();
   } else {
     // Fall back.
@@ -1418,9 +1386,8 @@ void Profiler::SampleThread(Thread* thread,
       &counters_, (isolate != NULL) ? isolate->main_port() : ILLEGAL_PORT,
       sample, sample_buffer, stack_lower, stack_upper, pc, fp, sp);
   const bool exited_dart_code = thread->HasExitedDartCode();
-  ProfilerDartStackWalker dart_stack_walker(thread, sample, sample_buffer,
-                                            stack_lower, stack_upper, pc, fp,
-                                            sp, /* allocation_sample*/ false);
+  ProfilerDartStackWalker dart_stack_walker(thread, sample, sample_buffer, pc,
+                                            fp, /* allocation_sample*/ false);
 
   // All memory access is done inside CollectSample.
   CollectSample(isolate, exited_dart_code, in_dart_code, sample,
diff --git a/runtime/vm/simulator_dbc.cc b/runtime/vm/simulator_dbc.cc
@@ -3981,11 +3981,6 @@ void Simulator::JumpToFrame(uword pc, uword sp, uword fp, Thread* thread) {
   // in the previous C++ frames.
   StackResource::Unwind(thread);
 
-  // Set the tag.
-  thread->set_vm_tag(VMTag::kDartCompiledTagId);
-  // Clear top exit frame.
-  thread->set_top_exit_frame_info(0);
-
   fp_ = reinterpret_cast<RawObject**>(fp);
 
   if (pc == StubCode::RunExceptionHandler().EntryPoint()) {
@@ -4003,6 +3998,11 @@ void Simulator::JumpToFrame(uword pc, uword sp, uword fp, Thread* thread) {
     pc_ = pc;
   }
 
+  // Set the tag.
+  thread->set_vm_tag(VMTag::kDartCompiledTagId);
+  // Clear top exit frame.
+  thread->set_top_exit_frame_info(0);
+
   buf->Longjmp();
   UNREACHABLE();
 }
diff --git a/runtime/vm/stub_code_arm.cc b/runtime/vm/stub_code_arm.cc
@@ -938,7 +938,7 @@ void StubCode::GenerateAllocateArrayStub(Assembler* assembler) {
 //   R2 : arguments array.
 //   R3 : current thread.
 void StubCode::GenerateInvokeDartCodeStub(Assembler* assembler) {
-  // Save frame pointer coming in.
+  __ Push(LR);  // Marker for the profiler.
   __ EnterFrame((1 << FP) | (1 << LR), 0);
 
   // Push code object to PC marker slot.
@@ -1050,6 +1050,7 @@ void StubCode::GenerateInvokeDartCodeStub(Assembler* assembler) {
 
   // Restore the frame pointer and return.
   __ LeaveFrame((1 << FP) | (1 << LR));
+  __ Drop(1);
   __ Ret();
 }
 
diff --git a/runtime/vm/stub_code_arm64.cc b/runtime/vm/stub_code_arm64.cc
@@ -1015,6 +1015,7 @@ void StubCode::GenerateInvokeDartCodeStub(Assembler* assembler) {
   // Copy the C stack pointer (R31) into the stack pointer we'll actually use
   // to access the stack.
   __ SetupDartSP();
+  __ Push(LR);  // Marker for the profiler.
   __ EnterFrame(0);
 
   // Push code object to PC marker slot.
@@ -1136,6 +1137,7 @@ void StubCode::GenerateInvokeDartCodeStub(Assembler* assembler) {
 
   // Restore the frame pointer and C stack pointer and return.
   __ LeaveFrame();
+  __ Drop(1);
   __ RestoreCSP();
   __ ret();
 }
@@ -1154,6 +1156,7 @@ void StubCode::GenerateInvokeDartCodeFromBytecodeStub(Assembler* assembler) {
   // Copy the C stack pointer (R31) into the stack pointer we'll actually use
   // to access the stack.
   __ SetupDartSP();
+  __ Push(LR);  // Marker for the profiler.
   __ EnterFrame(0);
 
   // Push code object to PC marker slot.
@@ -1272,6 +1275,7 @@ void StubCode::GenerateInvokeDartCodeFromBytecodeStub(Assembler* assembler) {
 
   // Restore the frame pointer and C stack pointer and return.
   __ LeaveFrame();
+  __ Drop(1);
   __ RestoreCSP();
   __ ret();
 #endif  // defined(DART_PRECOMPILED_RUNTIME)
diff --git a/runtime/vm/stub_code_ia32.cc b/runtime/vm/stub_code_ia32.cc
@@ -732,12 +732,12 @@ void StubCode::GenerateAllocateArrayStub(Assembler* assembler) {
 //   ESP + 16 : current thread.
 // Uses EAX, EDX, ECX, EDI as temporary registers.
 void StubCode::GenerateInvokeDartCodeStub(Assembler* assembler) {
-  const intptr_t kTargetCodeOffset = 2 * kWordSize;
-  const intptr_t kArgumentsDescOffset = 3 * kWordSize;
-  const intptr_t kArgumentsOffset = 4 * kWordSize;
-  const intptr_t kThreadOffset = 5 * kWordSize;
+  const intptr_t kTargetCodeOffset = 3 * kWordSize;
+  const intptr_t kArgumentsDescOffset = 4 * kWordSize;
+  const intptr_t kArgumentsOffset = 5 * kWordSize;
+  const intptr_t kThreadOffset = 6 * kWordSize;
 
-  // Save frame pointer coming in.
+  __ pushl(Address(ESP, 0));  // Marker for the profiler.
   __ EnterFrame(0);
 
   // Push code object to PC marker slot.
@@ -833,6 +833,7 @@ void StubCode::GenerateInvokeDartCodeStub(Assembler* assembler) {
 
   // Restore the frame pointer.
   __ LeaveFrame();
+  __ popl(ECX);
 
   __ ret();
 }
diff --git a/runtime/vm/stub_code_x64.cc b/runtime/vm/stub_code_x64.cc
@@ -926,7 +926,7 @@ void StubCode::GenerateAllocateArrayStub(Assembler* assembler) {
 //   RDX : arguments array.
 //   RCX : current thread.
 void StubCode::GenerateInvokeDartCodeStub(Assembler* assembler) {
-  // Save frame pointer coming in.
+  __ pushq(Address(RSP, 0));  // Marker for the profiler.
   __ EnterFrame(0);
 
   const Register kTargetCodeReg = CallingConventions::kArg1Reg;
@@ -1052,6 +1052,7 @@ void StubCode::GenerateInvokeDartCodeStub(Assembler* assembler) {
 
   // Restore the frame pointer.
   __ LeaveFrame();
+  __ popq(RCX);
 
   __ ret();
 }
@@ -1067,7 +1068,7 @@ void StubCode::GenerateInvokeDartCodeFromBytecodeStub(Assembler* assembler) {
 #if defined(DART_PRECOMPILED_RUNTIME)
   __ Stop("Not using interpreter");
 #else
-  // Save frame pointer coming in.
+  __ pushq(Address(RSP, 0));  // Marker for the profiler.
   __ EnterFrame(0);
 
   const Register kTargetCodeReg = CallingConventions::kArg1Reg;
@@ -1193,6 +1194,7 @@ void StubCode::GenerateInvokeDartCodeFromBytecodeStub(Assembler* assembler) {
 
   // Restore the frame pointer.
   __ LeaveFrame();
+  __ popq(RCX);
 
   __ ret();
 #endif  // defined(DART_PRECOMPILED_RUNTIME)
