Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(956)

Unified Diff: src/sampler.cc

Issue 1341413002: [profiler] Make no frame region detection code more robust [x86/x64] (Closed) Base URL: https://chromium.googlesource.com/v8/v8.git@master
Patch Set: Fix crashes on simulators Created 5 years, 3 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: src/sampler.cc
diff --git a/src/sampler.cc b/src/sampler.cc
index 4e669ca6dc13611fdda5bacc728d2f1227592646..3b641fc448890d0080f2e2b8f9c09eb6c50b09f5 100644
--- a/src/sampler.cc
+++ b/src/sampler.cc
@@ -173,6 +173,67 @@ class PlatformDataCommon : public Malloced {
ThreadId profiled_thread_id_;
};
+
+bool IsSamePage(byte* ptr1, byte* ptr2) {
+ const uint32_t kPageSize = 4096;
+ uintptr_t mask = ~static_cast<uintptr_t>(kPageSize - 1);
+ return (reinterpret_cast<uintptr_t>(ptr1) & mask) ==
+ (reinterpret_cast<uintptr_t>(ptr2) & mask);
+}
+
+
+// Check if the code at specified address could potentially be a
+// frame setup code.
+bool IsNoFrameRegion(Address address) {
+ struct Pattern {
+ int bytes_count;
+ byte bytes[8];
+ int offsets[4];
+ };
+ byte* pc = reinterpret_cast<byte*>(address);
+ static Pattern patterns[] = {
+#if V8_HOST_ARCH_IA32
+ // push %ebp
+ // mov %esp,%ebp
+ {3, {0x55, 0x89, 0xe5}, {0, 1, -1}},
+ // pop %ebp
+ // ret N
+ {2, {0x5d, 0xc2}, {0, 1, -1}},
+ // pop %ebp
+ // ret
+ {2, {0x5d, 0xc3}, {0, 1, -1}},
+#elif V8_HOST_ARCH_X64
+ // pushq %rbp
+ // movq %rsp,%rbp
+ {4, {0x55, 0x48, 0x89, 0xe5}, {0, 1, -1}},
+ // popq %rbp
+ // ret N
+ {2, {0x5d, 0xc2}, {0, 1, -1}},
+ // popq %rbp
+ // ret
+ {2, {0x5d, 0xc3}, {0, 1, -1}},
+#endif
+ {0, {}, {}}
+ };
+ for (Pattern* pattern = patterns; pattern->bytes_count; ++pattern) {
+ for (int* offset_ptr = pattern->offsets; *offset_ptr != -1; ++offset_ptr) {
+ int offset = *offset_ptr;
+ if (!offset || IsSamePage(pc, pc - offset)) {
+ if (!memcmp(pc - offset, pattern->bytes, pattern->bytes_count))
+ return true;
+ } else {
+ // It is not safe to examine bytes on another page as it might not be
+ // allocated thus causing a SEGFAULT.
+ // Check the pattern part that's on the same page and
+ // pessimistically assume it could be the entire pattern match.
+ if (!memcmp(pc, pattern->bytes + offset, pattern->bytes_count - offset))
+ return true;
+ }
+ }
+ }
+ return false;
+}
+
} // namespace
#if defined(USE_SIGNALS)
@@ -232,7 +293,9 @@ class SimulatorHelper {
inline void FillRegisters(v8::RegisterState* state) {
#if V8_TARGET_ARCH_ARM
- state->pc = reinterpret_cast<Address>(simulator_->get_pc());
+ 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(
@@ -243,19 +306,30 @@ class SimulatorHelper {
// the sp or fp register. ARM64 simulator does this in two steps:
// first setting it to zero and then setting it to a 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;
}
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
- state->pc = reinterpret_cast<Address>(simulator_->get_pc());
+ 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
- state->pc = reinterpret_cast<Address>(simulator_->get_pc());
+ 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 =
@@ -592,6 +666,11 @@ DISABLE_ASAN void TickSample::Init(Isolate* isolate,
Address js_entry_sp = isolate->js_entry_sp();
if (js_entry_sp == 0) return; // Not executing JS now.
+ if (pc && IsNoFrameRegion(pc)) {
+ pc = 0;
+ return;
+ }
+
ExternalCallbackScope* scope = isolate->external_callback_scope();
Address handler = Isolate::handler(isolate->thread_local_top());
// If there is a handler on top of the external callback scope then
« no previous file with comments | « no previous file | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698