Merge "simpleperf: fix an abort caused by ip zero in kernel callchain."

9 files changed, 85 insertions, 53 deletions

diff --git a/simpleperf/cmd_debug_unwind.cpp b/simpleperf/cmd_debug_unwind.cpp
index b2ea5264..c2019bd3 100644
--- a/simpleperf/cmd_debug_unwind.cpp
+++ b/simpleperf/cmd_debug_unwind.cpp
@@ -229,7 +229,6 @@ bool DebugUnwindCommand::ProcessRecord(Record* record) {
     if (selected_time_ != 0u && r.Timestamp() != selected_time_) {
       return true;
     }
-    r.AdjustCallChainGeneratedByKernel();
     uint64_t need_type = PERF_SAMPLE_CALLCHAIN | PERF_SAMPLE_REGS_USER | PERF_SAMPLE_STACK_USER;
     if ((r.sample_type & need_type) == need_type && r.regs_user_data.reg_mask != 0 &&
         r.GetValidStackSize() > 0) {
diff --git a/simpleperf/cmd_debug_unwind_test.cpp b/simpleperf/cmd_debug_unwind_test.cpp
index 7b99afaa..3eddb38d 100644
--- a/simpleperf/cmd_debug_unwind_test.cpp
+++ b/simpleperf/cmd_debug_unwind_test.cpp
@@ -65,3 +65,12 @@ TEST(cmd_debug_unwind, symfs_option) {
   ASSERT_TRUE(reader->ReadMetaInfoFeature(&info_map));
   ASSERT_EQ(info_map["debug_unwind"], "true");
 }
+
+TEST(cmd_debug_unwind, unwind_with_ip_zero_in_callchain) {
+  TemporaryFile tmp_file;
+  CaptureStdout capture;
+  ASSERT_TRUE(capture.Start());
+  ASSERT_TRUE(DebugUnwindCmd()->Run({"-i", GetTestData(PERF_DATA_WITH_IP_ZERO_IN_CALLCHAIN),
+                                     "-o", tmp_file.path}));
+  ASSERT_NE(capture.Finish().find("Unwinding sample count: 1"), std::string::npos);
+}
diff --git a/simpleperf/cmd_record.cpp b/simpleperf/cmd_record.cpp
index e58e6594..b3472c1f 100644
--- a/simpleperf/cmd_record.cpp
+++ b/simpleperf/cmd_record.cpp
@@ -1141,7 +1141,7 @@ bool RecordCommand::SaveRecordAfterUnwinding(Record* record) {
     }
     // ExcludeKernelCallChain() should go after UnwindRecord() to notice the generated user call
     // chain.
-    if (r.InKernel() && exclude_kernel_callchain_ && r.ExcludeKernelCallChain() == 0u) {
+    if (r.InKernel() && exclude_kernel_callchain_ && !r.ExcludeKernelCallChain()) {
       // If current record contains no user callchain, skip it.
       return true;
     }
@@ -1160,7 +1160,7 @@ bool RecordCommand::SaveRecordWithoutUnwinding(Record* record) {
     if (fp_callchain_sampling_ || dwarf_callchain_sampling_) {
       r.AdjustCallChainGeneratedByKernel();
     }
-    if (r.InKernel() && exclude_kernel_callchain_ && r.ExcludeKernelCallChain() == 0u) {
+    if (r.InKernel() && exclude_kernel_callchain_ && !r.ExcludeKernelCallChain()) {
       // If current record contains no user callchain, skip it.
       return true;
     }
diff --git a/simpleperf/get_test_data.h b/simpleperf/get_test_data.h
index 742804a8..114b6452 100644
--- a/simpleperf/get_test_data.h
+++ b/simpleperf/get_test_data.h
@@ -133,4 +133,6 @@ static const std::string PERF_DATA_WITH_KERNEL_SYMBOLS_AVAILABLE_FALSE = "perf_w
 
 static const std::string PERF_DATA_WITH_INTERPRETER_FRAMES = "perf_with_interpreter_frames.data";
 
+static const std::string PERF_DATA_WITH_IP_ZERO_IN_CALLCHAIN = "perf_with_ip_zero_in_callchain.data";
+
 #endif  // SIMPLE_PERF_GET_TEST_DATA_H_
diff --git a/simpleperf/record.cpp b/simpleperf/record.cpp
index e468ff3e..60ade898 100644
--- a/simpleperf/record.cpp
+++ b/simpleperf/record.cpp
@@ -581,29 +581,31 @@ void SampleRecord::ReplaceRegAndStackWithCallChain(const std::vector<uint64_t>&
   BuildBinaryWithNewCallChain(new_size, ips);
 }
 
-size_t SampleRecord::ExcludeKernelCallChain() {
-  size_t user_callchain_length = 0u;
-  if (sample_type & PERF_SAMPLE_CALLCHAIN) {
-    size_t i;
-    for (i = 0; i < callchain_data.ip_nr; ++i) {
-      if (callchain_data.ips[i] == PERF_CONTEXT_USER) {
-        i++;
-        if (i < callchain_data.ip_nr) {
-          ip_data.ip = callchain_data.ips[i];
-          if (sample_type & PERF_SAMPLE_IP) {
-            *reinterpret_cast<uint64_t*>(binary_ + header_size()) = ip_data.ip;
-          }
-          header.misc = (header.misc & ~PERF_RECORD_MISC_KERNEL) | PERF_RECORD_MISC_USER;
-          reinterpret_cast<perf_event_header*>(binary_)->misc = header.misc;
-        }
-        break;
-      } else {
-        callchain_data.ips[i] = PERF_CONTEXT_USER;
+bool SampleRecord::ExcludeKernelCallChain() {
+  if (!(sample_type & PERF_SAMPLE_CALLCHAIN)) {
+    return true;
+  }
+  size_t i;
+  for (i = 0; i < callchain_data.ip_nr; ++i) {
+    if (callchain_data.ips[i] == PERF_CONTEXT_USER) {
+      break;
+    }
+    // Erase kernel callchain.
+    callchain_data.ips[i] = PERF_CONTEXT_USER;
+  }
+  while (++i < callchain_data.ip_nr) {
+    if (callchain_data.ips[i] < PERF_CONTEXT_MAX) {
+      // Change the sample to make it hit the user space ip address.
+      ip_data.ip = callchain_data.ips[i];
+      if (sample_type & PERF_SAMPLE_IP) {
+        *reinterpret_cast<uint64_t*>(binary_ + header_size()) = ip_data.ip;
       }
+      header.misc = (header.misc & ~PERF_RECORD_MISC_CPUMODE_MASK) | PERF_RECORD_MISC_USER;
+      reinterpret_cast<perf_event_header*>(binary_)->misc = header.misc;
+      return true;
     }
-    user_callchain_length = callchain_data.ip_nr - i;
   }
-  return user_callchain_length;
+  return false;
 }
 
 bool SampleRecord::HasUserCallChain() const {
@@ -687,7 +689,8 @@ void SampleRecord::BuildBinaryWithNewCallChain(uint32_t new_size,
   callchain_data.ips = p64;
   callchain_data.ip_nr += 1 + ips.size();
   *--p64 = callchain_data.ip_nr;
-  CHECK_EQ(callchain_pos, static_cast<size_t>(reinterpret_cast<char*>(p64) - new_binary));
+  CHECK_EQ(callchain_pos, static_cast<size_t>(reinterpret_cast<char*>(p64) - new_binary))
+    << "record time " << time_data.time;
   if (new_binary != binary_) {
     UpdateBinary(new_binary);
   }
@@ -778,24 +781,33 @@ void SampleRecord::AdjustCallChainGeneratedByKernel() {
   // The kernel stores return addrs in the callchain, but we want the addrs of call instructions
   // along the callchain.
   uint64_t* ips = callchain_data.ips;
+  uint64_t context = header.misc == PERF_RECORD_MISC_KERNEL ? PERF_CONTEXT_KERNEL
+                                                            : PERF_CONTEXT_USER;
   bool first_frame = true;
-  for (uint64_t i = 0; i < callchain_data.ip_nr; ++i) {
-    if (ips[i] > 0 && ips[i] < PERF_CONTEXT_MAX) {
+  for (size_t i = 0; i < callchain_data.ip_nr; ++i) {
+    if (ips[i] < PERF_CONTEXT_MAX) {
       if (first_frame) {
         first_frame = false;
       } else {
-        // Here we want to change the return addr to the addr of the previous instruction. We don't
-        // need to find the exact start addr of the previous instruction. A location in
+        if (ips[i] < 2) {
+          // A wrong ip address, erase it.
+          ips[i] = context;
+        } else {
+          // Here we want to change the return addr to the addr of the previous instruction. We
+          // don't need to find the exact start addr of the previous instruction. A location in
         // [start_addr_of_call_inst, start_addr_of_next_inst) is enough.
 #if defined(__arm__) || defined(__aarch64__)
-        // If we are built for arm/aarch64, this may be a callchain of thumb code. For thumb code,
-        // the real instruction addr is (ip & ~1), and ip - 2 can used to hit the address range
-        // of the previous instruction. For non thumb code, any addr in [ip - 4, ip - 1] is fine.
-        ips[i] -= 2;
+          // If we are built for arm/aarch64, this may be a callchain of thumb code. For thumb code,
+          // the real instruction addr is (ip & ~1), and ip - 2 can used to hit the address range
+          // of the previous instruction. For non thumb code, any addr in [ip - 4, ip - 1] is fine.
+          ips[i] -= 2;
 #else
-        ips[i]--;
+          ips[i]--;
 #endif
+        }
       }
+    } else {
+      context = ips[i];
     }
   }
 }
diff --git a/simpleperf/record.h b/simpleperf/record.h
index 810abbac..e002370c 100644
--- a/simpleperf/record.h
+++ b/simpleperf/record.h
@@ -397,7 +397,9 @@ struct SampleRecord : public Record {
                const std::vector<char>& stack, uint64_t dyn_stack_size);
 
   void ReplaceRegAndStackWithCallChain(const std::vector<uint64_t>& ips);
-  size_t ExcludeKernelCallChain();
+  // Remove kernel callchain, return true if there is a user space callchain left, otherwise
+  // return false.
+  bool ExcludeKernelCallChain();
   bool HasUserCallChain() const;
   void UpdateUserCallChain(const std::vector<uint64_t>& user_ips);
 
diff --git a/simpleperf/record_file_reader.cpp b/simpleperf/record_file_reader.cpp
index 46b72665..1703c0a7 100644
--- a/simpleperf/record_file_reader.cpp
+++ b/simpleperf/record_file_reader.cpp
@@ -234,20 +234,6 @@ bool RecordFileReader::ReadRecord(std::unique_ptr<Record>& record) {
     }
     if (record->type() == SIMPLE_PERF_RECORD_EVENT_ID) {
       ProcessEventIdRecord(*static_cast<EventIdRecord*>(record.get()));
-    } else if (record->type() == PERF_RECORD_SAMPLE) {
-      SampleRecord* r = static_cast<SampleRecord*>(record.get());
-      // Although we have removed ip == 0 callchains when recording dwarf based callgraph,
-      // stack frame based callgraph can also generate ip == 0 callchains. Remove them here
-      // to avoid caller's effort.
-      if (r->sample_type & PERF_SAMPLE_CALLCHAIN) {
-        size_t i;
-        for (i = 0; i < r->callchain_data.ip_nr; ++i) {
-          if (r->callchain_data.ips[i] == 0) {
-            break;
-          }
-        }
-        r->callchain_data.ip_nr = i;
-      }
     }
   }
   return true;
diff --git a/simpleperf/record_test.cpp b/simpleperf/record_test.cpp
index a36448b0..c14fdacb 100644
--- a/simpleperf/record_test.cpp
+++ b/simpleperf/record_test.cpp
@@ -61,11 +61,11 @@ TEST_F(RecordTest, SampleRecordMatchBinary) {
 
 TEST_F(RecordTest, SampleRecord_exclude_kernel_callchain) {
   SampleRecord r(event_attr, 0, 1, 0, 0, 0, 0, 0, {}, {}, 0);
-  ASSERT_EQ(0u, r.ExcludeKernelCallChain());
+  ASSERT_TRUE(r.ExcludeKernelCallChain());
 
   event_attr.sample_type |= PERF_SAMPLE_CALLCHAIN;
   SampleRecord r1(event_attr, 0, 1, 0, 0, 0, 0, 0, {PERF_CONTEXT_USER, 2}, {}, 0);
-  ASSERT_EQ(1u, r1.ExcludeKernelCallChain());
+  ASSERT_TRUE(r1.ExcludeKernelCallChain());
   ASSERT_EQ(2u, r1.ip_data.ip);
   SampleRecord r2(event_attr, r1.BinaryForTestingOnly());
   ASSERT_EQ(1u, r.ip_data.ip);
@@ -74,7 +74,7 @@ TEST_F(RecordTest, SampleRecord_exclude_kernel_callchain) {
   ASSERT_EQ(2u, r2.callchain_data.ips[1]);
 
   SampleRecord r3(event_attr, 0, 1, 0, 0, 0, 0, 0, {1, PERF_CONTEXT_USER, 2}, {}, 0);
-  ASSERT_EQ(1u, r3.ExcludeKernelCallChain());
+  ASSERT_TRUE(r3.ExcludeKernelCallChain());
   ASSERT_EQ(2u, r3.ip_data.ip);
   SampleRecord r4(event_attr, r3.BinaryForTestingOnly());
   ASSERT_EQ(2u, r4.ip_data.ip);
@@ -84,9 +84,18 @@ TEST_F(RecordTest, SampleRecord_exclude_kernel_callchain) {
   ASSERT_EQ(2u, r4.callchain_data.ips[2]);
 
   SampleRecord r5(event_attr, 0, 1, 0, 0, 0, 0, 0, {1, 2}, {}, 0);
-  ASSERT_EQ(0u, r5.ExcludeKernelCallChain());
+  ASSERT_FALSE(r5.ExcludeKernelCallChain());
   SampleRecord r6(event_attr, 0, 1, 0, 0, 0, 0, 0, {1, 2, PERF_CONTEXT_USER}, {}, 0);
-  ASSERT_EQ(0u, r6.ExcludeKernelCallChain());
+  ASSERT_FALSE(r6.ExcludeKernelCallChain());
+
+  // Process consecutive context values.
+  SampleRecord r7(event_attr, 0, 1, 0, 0, 0, 0, 0,
+                  {1, 2, PERF_CONTEXT_USER, PERF_CONTEXT_USER, 3, 4}, {}, 0);
+  r7.header.misc = PERF_RECORD_MISC_KERNEL;
+  ASSERT_TRUE(r7.ExcludeKernelCallChain());
+  CheckRecordEqual(r7, SampleRecord(event_attr, 0, 3, 0, 0, 0, 0, 0,
+                                    {PERF_CONTEXT_USER, PERF_CONTEXT_USER, PERF_CONTEXT_USER,
+                                     PERF_CONTEXT_USER, 3, 4}, {}, 0));
 }
 
 TEST_F(RecordTest, SampleRecord_ReplaceRegAndStackWithCallChain) {
@@ -109,3 +118,16 @@ TEST_F(RecordTest, SampleRecord_UpdateUserCallChain) {
   SampleRecord expected(event_attr, 0, 1, 2, 3, 4, 5, 6, {1, PERF_CONTEXT_USER, 3, 4, 5}, {}, 0);
   CheckRecordEqual(r, expected);
 }
+
+TEST_F(RecordTest, SampleRecord_AdjustCallChainGeneratedByKernel) {
+  event_attr.sample_type |= PERF_SAMPLE_CALLCHAIN | PERF_SAMPLE_REGS_USER | PERF_SAMPLE_STACK_USER;
+  SampleRecord r(event_attr, 0, 1, 2, 3, 4, 5, 6, {1, 5, 0, PERF_CONTEXT_USER, 6, 0}, {}, 0);
+  r.header.misc = PERF_RECORD_MISC_KERNEL;
+  r.AdjustCallChainGeneratedByKernel();
+  uint64_t adjustValue = (GetBuildArch() == ARCH_ARM || GetBuildArch() == ARCH_ARM64) ? 2 : 1;
+  SampleRecord expected(event_attr, 0, 1, 2, 3, 4, 5, 6,
+                        {1, 5 - adjustValue, PERF_CONTEXT_KERNEL, PERF_CONTEXT_USER,
+                         6 - adjustValue, PERF_CONTEXT_USER}, {}, 0);
+  expected.header.misc = PERF_RECORD_MISC_KERNEL;
+  CheckRecordEqual(r, expected);
+}
diff --git a/simpleperf/testdata/perf_with_ip_zero_in_callchain.data b/simpleperf/testdata/perf_with_ip_zero_in_callchain.data
new file mode 100644
index 00000000..37e7eed9
--- /dev/null
+++ b/simpleperf/testdata/perf_with_ip_zero_in_callchain.data