Move SimulatorHelper into V8 out of profiler clients.

This patch is based on alph's CL https://codereview.chromium.org/2128613004/.

This patch makes GetStackSample propogate the register state when using
simulator helper, and adds argument to avoid using register state from simulator
when pass the native register state.

BUG=v8:4789
LOG=N

Review-Url: https://codereview.chromium.org/2189513002
Cr-Commit-Position: refs/heads/master@{#38554}

include/v8-profiler.h

@@ -60,12 +60,47 @@ struct TickSample {
         frames_count(0),
         has_external_callback(false),
         update_stats(true) {}
+
+  /**
+   * Initialize a tick sample from the isolate.
+   * \param isolate The isolate.
+   * \param state Execution state.
+   * \param record_c_entry_frame Include or skip the runtime function.
+   * \param update_stats Whether update the sample to the aggregated stats.
+   * \param use_simulator_reg_state When set to true and V8 is running under a
+   *                                simulator, the method will use the simulator
+   *                                register state rather than the one provided
+   *                                with |state| argument. Otherwise the method
+   *                                will use provided register |state| as is.
+   */
   void Init(Isolate* isolate, const v8::RegisterState& state,
-            RecordCEntryFrame record_c_entry_frame, bool update_stats);
-  static bool GetStackSample(Isolate* isolate, const v8::RegisterState& state,
+            RecordCEntryFrame record_c_entry_frame, bool update_stats,
+            bool use_simulator_reg_state = true);
+  /**
+   * Get a call stack sample from the isolate.
+   * \param isolate The isolate.
+   * \param state Register state.
+   * \param record_c_entry_frame Include or skip the runtime function.
+   * \param frames Caller allocated buffer to store stack frames.
+   * \param frames_limit Maximum number of frames to capture. The buffer must
+   *                     be large enough to hold the number of frames.
+   * \param sample_info The sample info is filled up by the function
+   *                    provides number of actual captured stack frames and
+   *                    the current VM state.
+   * \param use_simulator_reg_state When set to true and V8 is running under a
+   *                                simulator, the method will use the simulator
+   *                                register state rather than the one provided
+   *                                with |state| argument. Otherwise the method
+   *                                will use provided register |state| as is.
+   * \note GetStackSample is thread and signal safe and should only be called
+   *                      when the JS thread is paused or interrupted.
+   *                      Otherwise the behavior is undefined.
+   */
+  static bool GetStackSample(Isolate* isolate, v8::RegisterState* state,
                              RecordCEntryFrame record_c_entry_frame,
                              void** frames, size_t frames_limit,
-                             v8::SampleInfo* sample_info);
+                             v8::SampleInfo* sample_info,
+                             bool use_simulator_reg_state = true);
   StateTag state;  // The state of the VM.
   void* pc;        // Instruction pointer.
   union {

src/api.cc

@@ -7672,20 +7672,14 @@ bool Isolate::GetHeapCodeAndMetadataStatistics(
 
 void Isolate::GetStackSample(const RegisterState& state, void** frames,
                              size_t frames_limit, SampleInfo* sample_info) {
-#if defined(USE_SIMULATOR)
-  RegisterState regs;
-  i::Isolate* isolate = reinterpret_cast<i::Isolate*>(this);
-  if (!i::SimulatorHelper::FillRegisters(isolate, &regs)) {
+  RegisterState regs = state;
+  if (TickSample::GetStackSample(this, &regs, TickSample::kSkipCEntryFrame,
+                                 frames, frames_limit, sample_info)) {
+    return;
+  }
   sample_info->frames_count = 0;
   sample_info->vm_state = OTHER;
   sample_info->external_callback_entry = nullptr;
-    return;
-  }
-#else
-  const RegisterState& regs = state;
-#endif
-  TickSample::GetStackSample(this, regs, TickSample::kSkipCEntryFrame, frames,
-                             frames_limit, sample_info);
 }
 
 size_t Isolate::NumberOfPhantomHandleResetsSinceLastCall() {

src/log.cc

@@ -645,15 +645,9 @@ class Ticker: public sampler::Sampler {
 
   void SampleStack(const v8::RegisterState& state) override {
     if (!profiler_) return;
-#if defined(USE_SIMULATOR)
-    Isolate* i_isolate = reinterpret_cast<Isolate*>(isolate());
-    v8::RegisterState regs;
-    if (!SimulatorHelper::FillRegisters(i_isolate, &regs)) return;
-#else
-    const v8::RegisterState& regs = state;
-#endif
-    v8::TickSample sample;
-    sample.Init(isolate(), regs, v8::TickSample::kIncludeCEntryFrame, true);
+    Isolate* isolate = reinterpret_cast<Isolate*>(this->isolate());
+    TickSample sample;
+    sample.Init(isolate, state, TickSample::kIncludeCEntryFrame, true);
     profiler_->Insert(&sample);
   }
 

src/profiler/cpu-profiler.cc

@@ -23,18 +23,11 @@ class CpuSampler : public sampler::Sampler {
       : sampler::Sampler(reinterpret_cast<v8::Isolate*>(isolate)),
         processor_(processor) {}
 
-  void SampleStack(const v8::RegisterState& state) override {
-    v8::Isolate* v8_isolate = isolate();
-    Isolate* i_isolate = reinterpret_cast<Isolate*>(v8_isolate);
-#if defined(USE_SIMULATOR)
-    v8::RegisterState regs;
-    if (!SimulatorHelper::FillRegisters(i_isolate, &regs)) return;
-#else
-    const v8::RegisterState& regs = state;
-#endif
+  void SampleStack(const v8::RegisterState& regs) override {
     TickSample* sample = processor_->StartTickSample();
-    if (sample == NULL) return;
-    sample->Init(i_isolate, regs, TickSample::kIncludeCEntryFrame, true);
+    if (sample == nullptr) return;
+    Isolate* isolate = reinterpret_cast<Isolate*>(this->isolate());
+    sample->Init(isolate, regs, TickSample::kIncludeCEntryFrame, true);
     if (is_counting_samples_ && !sample->timestamp.IsNull()) {
       if (sample->state == JS) ++js_sample_count_;
       if (sample->state == EXTERNAL) ++external_sample_count_;
@@ -77,7 +70,7 @@ void ProfilerEventsProcessor::AddDeoptStack(Isolate* isolate, Address from,
   regs.sp = fp - fp_to_sp_delta;
   regs.fp = fp;
   regs.pc = from;
-  record.sample.Init(isolate, regs, TickSample::kSkipCEntryFrame, false);
+  record.sample.Init(isolate, regs, TickSample::kSkipCEntryFrame, false, false);
   ticks_from_vm_buffer_.Enqueue(record);
 }
 
@@ -92,7 +85,8 @@ void ProfilerEventsProcessor::AddCurrentStack(Isolate* isolate,
     regs.fp = frame->fp();
     regs.pc = frame->pc();
   }
-  record.sample.Init(isolate, regs, TickSample::kSkipCEntryFrame, update_stats);
+  record.sample.Init(isolate, regs, TickSample::kSkipCEntryFrame, update_stats,
+                     false);
   ticks_from_vm_buffer_.Enqueue(record);
 }
 

src/profiler/tick-sample.cc

@@ -11,7 +11,6 @@
 #include "src/vm-state-inl.h"
 
 namespace v8 {
-
 namespace {
 
 bool IsSamePage(i::byte* ptr1, i::byte* ptr2) {
@@ -77,19 +76,92 @@ bool IsNoFrameRegion(i::Address address) {
 
 }  // namespace
 
+namespace internal {
+namespace {
+
+#if defined(USE_SIMULATOR)
+class SimulatorHelper {
+ public:
+  // Returns true if register values were successfully retrieved
+  // from the simulator, otherwise returns false.
+  static bool FillRegisters(Isolate* isolate, v8::RegisterState* state);
+};
+
+bool SimulatorHelper::FillRegisters(Isolate* isolate,
+                                    v8::RegisterState* state) {
+  Simulator* simulator = isolate->thread_local_top()->simulator_;
+  // Check if there is active simulator.
+  if (simulator == NULL) return false;
+#if V8_TARGET_ARCH_ARM
+  if (!simulator->has_bad_pc()) {
+    state->pc = reinterpret_cast<Address>(simulator->get_pc());
+  }
+  state->sp = reinterpret_cast<Address>(simulator->get_register(Simulator::sp));
+  state->fp =
+      reinterpret_cast<Address>(simulator->get_register(Simulator::r11));
+#elif V8_TARGET_ARCH_ARM64
+  state->pc = reinterpret_cast<Address>(simulator->pc());
+  state->sp = reinterpret_cast<Address>(simulator->sp());
+  state->fp = reinterpret_cast<Address>(simulator->fp());
+#elif V8_TARGET_ARCH_MIPS || V8_TARGET_ARCH_MIPS64
+  if (!simulator->has_bad_pc()) {
+    state->pc = reinterpret_cast<Address>(simulator->get_pc());
+  }
+  state->sp = reinterpret_cast<Address>(simulator->get_register(Simulator::sp));
+  state->fp = reinterpret_cast<Address>(simulator->get_register(Simulator::fp));
+#elif V8_TARGET_ARCH_PPC
+  if (!simulator->has_bad_pc()) {
+    state->pc = reinterpret_cast<Address>(simulator->get_pc());
+  }
+  state->sp = reinterpret_cast<Address>(simulator->get_register(Simulator::sp));
+  state->fp = reinterpret_cast<Address>(simulator->get_register(Simulator::fp));
+#elif V8_TARGET_ARCH_S390
+  if (!simulator->has_bad_pc()) {
+    state->pc = reinterpret_cast<Address>(simulator->get_pc());
+  }
+  state->sp = reinterpret_cast<Address>(simulator->get_register(Simulator::sp));
+  state->fp = reinterpret_cast<Address>(simulator->get_register(Simulator::fp));
+#endif
+  if (state->sp == 0 || state->fp == 0) {
+    // It possible that the simulator is interrupted while it is updating
+    // the sp or fp register. ARM64 simulator does this in two steps:
+    // first setting it to zero and then setting it to the new value.
+    // Bailout if sp/fp doesn't contain the new value.
+    //
+    // FIXME: The above doesn't really solve the issue.
+    // If a 64-bit target is executed on a 32-bit host even the final
+    // write is non-atomic, so it might obtain a half of the result.
+    // Moreover as long as the register set code uses memcpy (as of now),
+    // it is not guaranteed to be atomic even when both host and target
+    // are of same bitness.
+    return false;
+  }
+  return true;
+}
+#endif  // USE_SIMULATOR
+
+}  // namespace
+}  // namespace internal
+
 //
 // StackTracer implementation
 //
 DISABLE_ASAN void TickSample::Init(Isolate* v8_isolate,
-                                   const RegisterState& regs,
+                                   const RegisterState& reg_state,
                                    RecordCEntryFrame record_c_entry_frame,
-                                   bool update_stats) {
+                                   bool update_stats,
+                                   bool use_simulator_reg_state) {
   this->update_stats = update_stats;
-
   SampleInfo info;
-  if (GetStackSample(v8_isolate, const_cast<RegisterState&>(regs),
-                     record_c_entry_frame, reinterpret_cast<void**>(&stack[0]),
-                     kMaxFramesCount, &info)) {
+  RegisterState regs = reg_state;
+  if (!GetStackSample(v8_isolate, &regs, record_c_entry_frame, stack,
+                      kMaxFramesCount, &info, use_simulator_reg_state)) {
+    // It is executing JS but failed to collect a stack trace.
+    // Mark the sample as spoiled.
+    pc = nullptr;
+    return;
+  }
+
   state = info.vm_state;
   pc = regs.pc;
   frames_count = static_cast<unsigned>(info.frames_count);
@@ -106,17 +178,13 @@ DISABLE_ASAN void TickSample::Init(Isolate* v8_isolate,
   } else {
     tos = nullptr;
   }
-  } else {
-    // It is executing JS but failed to collect a stack trace.
-    // Mark the sample as spoiled.
-    pc = nullptr;
-  }
 }
 
-bool TickSample::GetStackSample(Isolate* v8_isolate, const RegisterState& regs,
+bool TickSample::GetStackSample(Isolate* v8_isolate, RegisterState* regs,
                                 RecordCEntryFrame record_c_entry_frame,
                                 void** frames, size_t frames_limit,
-                                v8::SampleInfo* sample_info) {
+                                v8::SampleInfo* sample_info,
+                                bool use_simulator_reg_state) {
   i::Isolate* isolate = reinterpret_cast<i::Isolate*>(v8_isolate);
   sample_info->frames_count = 0;
   sample_info->vm_state = isolate->current_vm_state();
@@ -125,10 +193,18 @@ bool TickSample::GetStackSample(Isolate* v8_isolate, const RegisterState& regs,
 
   i::Address js_entry_sp = isolate->js_entry_sp();
   if (js_entry_sp == nullptr) return true;  // Not executing JS now.
-  DCHECK(regs.sp);
 
-  if (regs.pc && IsNoFrameRegion(static_cast<i::Address>(regs.pc))) {
-    // Can't collect stack.
+#if defined(USE_SIMULATOR)
+  if (use_simulator_reg_state) {
+    if (!i::SimulatorHelper::FillRegisters(isolate, regs)) return false;
+  }
+#else
+  USE(use_simulator_reg_state);
+#endif
+  DCHECK(regs->sp);
+
+  if (regs->pc && IsNoFrameRegion(static_cast<i::Address>(regs->pc))) {
+    // The frame is not setup, so it'd be hard to iterate the stack. Bailout.
     return false;
   }
 
@@ -142,8 +218,8 @@ bool TickSample::GetStackSample(Isolate* v8_isolate, const RegisterState& regs,
         *scope->callback_entrypoint_address();
   }
 
-  i::SafeStackFrameIterator it(isolate, reinterpret_cast<i::Address>(regs.fp),
-                               reinterpret_cast<i::Address>(regs.sp),
+  i::SafeStackFrameIterator it(isolate, reinterpret_cast<i::Address>(regs->fp),
+                               reinterpret_cast<i::Address>(regs->sp),
                                js_entry_sp);
 
   // If at this point iterator does not see any frames,
@@ -180,67 +256,14 @@ bool TickSample::GetStackSample(Isolate* v8_isolate, const RegisterState& regs,
 namespace internal {
 
 void TickSample::Init(Isolate* isolate, const v8::RegisterState& state,
-                      RecordCEntryFrame record_c_entry_frame,
-                      bool update_stats) {
+                      RecordCEntryFrame record_c_entry_frame, bool update_stats,
+                      bool use_simulator_reg_state) {
   v8::TickSample::Init(reinterpret_cast<v8::Isolate*>(isolate), state,
-                       record_c_entry_frame, update_stats);
+                       record_c_entry_frame, update_stats,
+                       use_simulator_reg_state);
   if (pc == nullptr) return;
   timestamp = base::TimeTicks::HighResolutionNow();
 }
 
-#if defined(USE_SIMULATOR)
-bool SimulatorHelper::FillRegisters(Isolate* isolate,
-                                    v8::RegisterState* state) {
-  Simulator* simulator = isolate->thread_local_top()->simulator_;
-  // Check if there is active simulator.
-  if (simulator == NULL) return false;
-#if V8_TARGET_ARCH_ARM
-  if (!simulator->has_bad_pc()) {
-    state->pc = reinterpret_cast<Address>(simulator->get_pc());
-  }
-  state->sp = reinterpret_cast<Address>(simulator->get_register(Simulator::sp));
-  state->fp =
-      reinterpret_cast<Address>(simulator->get_register(Simulator::r11));
-#elif V8_TARGET_ARCH_ARM64
-  state->pc = reinterpret_cast<Address>(simulator->pc());
-  state->sp = reinterpret_cast<Address>(simulator->sp());
-  state->fp = reinterpret_cast<Address>(simulator->fp());
-#elif V8_TARGET_ARCH_MIPS || V8_TARGET_ARCH_MIPS64
-  if (!simulator->has_bad_pc()) {
-    state->pc = reinterpret_cast<Address>(simulator->get_pc());
-  }
-  state->sp = reinterpret_cast<Address>(simulator->get_register(Simulator::sp));
-  state->fp = reinterpret_cast<Address>(simulator->get_register(Simulator::fp));
-#elif V8_TARGET_ARCH_PPC
-  if (!simulator->has_bad_pc()) {
-    state->pc = reinterpret_cast<Address>(simulator->get_pc());
-  }
-  state->sp = reinterpret_cast<Address>(simulator->get_register(Simulator::sp));
-  state->fp = reinterpret_cast<Address>(simulator->get_register(Simulator::fp));
-#elif V8_TARGET_ARCH_S390
-  if (!simulator->has_bad_pc()) {
-    state->pc = reinterpret_cast<Address>(simulator->get_pc());
-  }
-  state->sp = reinterpret_cast<Address>(simulator->get_register(Simulator::sp));
-  state->fp = reinterpret_cast<Address>(simulator->get_register(Simulator::fp));
-#endif
-  if (state->sp == 0 || state->fp == 0) {
-    // It possible that the simulator is interrupted while it is updating
-    // the sp or fp register. ARM64 simulator does this in two steps:
-    // first setting it to zero and then setting it to the new value.
-    // Bailout if sp/fp doesn't contain the new value.
-    //
-    // FIXME: The above doesn't really solve the issue.
-    // If a 64-bit target is executed on a 32-bit host even the final
-    // write is non-atomic, so it might obtain a half of the result.
-    // Moreover as long as the register set code uses memcpy (as of now),
-    // it is not guaranteed to be atomic even when both host and target
-    // are of same bitness.
-    return false;
-  }
-  return true;
-}
-#endif  // USE_SIMULATOR
-
 }  // namespace internal
 }  // namespace v8

src/profiler/tick-sample.h

@@ -15,21 +15,12 @@ namespace internal {
 class Isolate;
 
 struct TickSample : public v8::TickSample {
-  TickSample() : v8::TickSample() {}
   void Init(Isolate* isolate, const v8::RegisterState& state,
-            RecordCEntryFrame record_c_entry_frame, bool update_stats);
+            RecordCEntryFrame record_c_entry_frame, bool update_stats,
+            bool use_simulator_reg_state = true);
   base::TimeTicks timestamp;
 };
 
-#if defined(USE_SIMULATOR)
-class SimulatorHelper {
- public:
-  // Returns true if register values were successfully retrieved
-  // from the simulator, otherwise returns false.
-  static bool FillRegisters(Isolate* isolate, v8::RegisterState* state);
-};
-#endif  // USE_SIMULATOR
-
 }  // namespace internal
 }  // namespace v8
 

test/cctest/libsampler/test-sampler.cc

@@ -41,10 +41,9 @@ class TestSampler : public Sampler {
   explicit TestSampler(Isolate* isolate) : Sampler(isolate) {}
 
   void SampleStack(const v8::RegisterState& regs) override {
-    void* frames[Sampler::kMaxFramesCount];
+    void* frames[kMaxFramesCount];
     SampleInfo sample_info;
-    isolate()->GetStackSample(regs, reinterpret_cast<void**>(frames),
-                              Sampler::kMaxFramesCount, &sample_info);
+    isolate()->GetStackSample(regs, frames, kMaxFramesCount, &sample_info);
     if (is_counting_samples_) {
       if (sample_info.vm_state == JS) ++js_sample_count_;
       if (sample_info.vm_state == EXTERNAL) ++external_sample_count_;