Skip samples where top function's stack frame is not setup properly

src/profile-generator.cc

Index: src/profile-generator.cc
diff --git a/src/profile-generator.cc b/src/profile-generator.cc
index b1b163b50ecdf2d59173eef568c84878fb50becf..5a645435a5355c001fac9c469f93a8ad47a83cf3 100644
--- a/src/profile-generator.cc
+++ b/src/profile-generator.cc
@@ -531,13 +531,16 @@ void CodeMap::DeleteAllCoveredCode(Address start, Address end) {
 }
 
 
-CodeEntry* CodeMap::FindEntry(Address addr) {
+CodeEntry* CodeMap::FindEntry(Address addr, Address* start) {
   CodeTree::Locator locator;
   if (tree_.FindGreatestLessThan(addr, &locator)) {
     // locator.key() <= addr. Need to check that addr is within entry.
     const CodeEntryInfo& entry = locator.value();
-    if (addr < (locator.key() + entry.size))
+    if (addr < (locator.key() + entry.size)) {
+      if (start)
+        *start = locator.key();
       return entry.entry;
+    }
   }
   return NULL;
 }
@@ -898,7 +901,24 @@ void ProfileGenerator::RecordTickSample(const TickSample& sample) {
   CodeEntry** entry = entries.start();
   memset(entry, 0, entries.length() * sizeof(*entry));
   if (sample.pc != NULL) {
-    *entry++ = code_map_.FindEntry(sample.pc);
+    Address start;
+    CodeEntry* pc_entry = code_map_.FindEntry(sample.pc, &start);
+    // If pc is in the function code before it setup stack frame or after the
+    // frame was destroyed SafeStackTraceFrameIterator incorrectly thinks that
+    // ebp cotains return address of the current function and skips caller's
+    // frame. Check for this case and just skip such samples.
+    if (pc_entry && pc_entry->frame_setup_offset() != -1) {
+      Code* code = Code::cast(HeapObject::FromAddress(start));
+      Address instruction_start = code->instruction_start();
+      // Frame setup code is at least 4 bytes.
+      if (sample.pc < instruction_start + pc_entry->frame_setup_offset() + 4)
+        return;

[loislo, 2013/04/26 13:36:56]

looks like you are skipping the other stack traces.

[yurys, 2013/04/26 13:43:51]

Yes, this is intentional, the comment above describes that.

+      Address frame_destroy = instruction_start +
+          pc_entry->frame_destroy_offset();
+      if (frame_destroy < sample.pc && sample.pc < frame_destroy + 4)
+        return;

[loislo, 2013/04/26 13:36:56]

ditto

[yurys, 2013/04/26 13:43:51]

See above.

+    }
+    *entry++ = pc_entry;

[loislo, 2013/04/26 13:36:56]

sounds like you are adding zero entry if the mapping wasn't found.

[yurys, 2013/04/26 13:43:51]

This part didn't change, it is normal case - pc could have pointed into some
stub code e.g.

 
     if (sample.has_external_callback) {
       // Don't use PC when in external callback code, as it can point