Merge "simpleperf: fix unknown binary in samples."

7 files changed, 60 insertions, 16 deletions

diff --git a/simpleperf/cmd_report.cpp b/simpleperf/cmd_report.cpp
index 8ebab61d..a3f80948 100644
--- a/simpleperf/cmd_report.cpp
+++ b/simpleperf/cmd_report.cpp
@@ -99,6 +99,7 @@ struct SampleTree {
   std::vector<SampleEntry*> samples;
   uint64_t total_samples;
   uint64_t total_period;
+  uint64_t total_error_callchains;
 };
 
 BUILD_COMPARE_VALUE_FUNCTION(CompareVaddrInFile, vaddr_in_file);
@@ -112,7 +113,8 @@ class ReportCmdSampleTreeBuilder
       : SampleTreeBuilder(sample_comparator),
         thread_tree_(thread_tree),
         total_samples_(0),
-        total_period_(0) {}
+        total_period_(0),
+        total_error_callchains_(0) {}
 
   void SetFilters(const std::unordered_set<int>& pid_filter,
                   const std::unordered_set<int>& tid_filter,
@@ -132,6 +134,7 @@ class ReportCmdSampleTreeBuilder
     sample_tree.samples = GetSamples();
     sample_tree.total_samples = total_samples_;
     sample_tree.total_period = total_period_;
+    sample_tree.total_error_callchains = total_error_callchains_;
     return sample_tree;
   }
 
@@ -179,6 +182,11 @@ class ReportCmdSampleTreeBuilder
                                      const uint64_t& acc_info) override {
     const ThreadEntry* thread = sample->thread;
     const MapEntry* map = thread_tree_->FindMap(thread, ip, in_kernel);
+    if (thread_tree_->IsUnknownDso(map->dso)) {
+      // The unwinders can give wrong ip addresses, which can't map to a valid dso. Skip them.
+      total_error_callchains_++;
+      return nullptr;
+    }
     uint64_t vaddr_in_file;
     const Symbol* symbol = thread_tree_->FindSymbol(map, ip, &vaddr_in_file);
     std::unique_ptr<SampleEntry> callchain_sample(new SampleEntry(
@@ -242,6 +250,7 @@ class ReportCmdSampleTreeBuilder
 
   uint64_t total_samples_;
   uint64_t total_period_;
+  uint64_t total_error_callchains_;
 };
 
 struct SampleTreeBuilderOptions {
@@ -808,6 +817,11 @@ bool ReportCommand::PrintReport() {
     fprintf(report_fp, "Event: %s (type %u, config %llu)\n", attr.name.c_str(),
             attr.attr.type, attr.attr.config);
     fprintf(report_fp, "Samples: %" PRIu64 "\n", sample_tree.total_samples);
+    if (sample_tree.total_error_callchains != 0) {
+      fprintf(report_fp, "Error Callchains: %" PRIu64 ", %f%%\n",
+              sample_tree.total_error_callchains,
+              sample_tree.total_error_callchains * 100.0 / sample_tree.total_samples);
+    }
     fprintf(report_fp, "Event count: %" PRIu64 "\n\n", sample_tree.total_period);
     sample_tree_displayer_->DisplaySamples(report_fp, sample_tree.samples, &sample_tree);
   }
diff --git a/simpleperf/cmd_report_sample.cpp b/simpleperf/cmd_report_sample.cpp
index 886d1b4c..24d1bc0d 100644
--- a/simpleperf/cmd_report_sample.cpp
+++ b/simpleperf/cmd_report_sample.cpp
@@ -88,12 +88,10 @@ class ReportSampleCommand : public Command {
   bool DumpProtobufReport(const std::string& filename);
   bool ProcessRecord(std::unique_ptr<Record> record);
   bool PrintSampleRecordInProtobuf(const SampleRecord& record);
-  void GetCallEntry(const ThreadEntry* thread, bool in_kernel, uint64_t ip,
-                    uint64_t* pvaddr_in_file, uint32_t* pfile_id,
-                    int32_t* psymbol_id);
-  void GetCallEntry(const ThreadEntry* thread, bool in_kernel, uint64_t ip,
-                    uint64_t* pvaddr_in_file, Dso** pdso,
-                    const Symbol** psymbol);
+  bool GetCallEntry(const ThreadEntry* thread, bool in_kernel, uint64_t ip, bool omit_unknown_dso,
+                    uint64_t* pvaddr_in_file, uint32_t* pfile_id, int32_t* psymbol_id);
+  bool GetCallEntry(const ThreadEntry* thread, bool in_kernel, uint64_t ip, bool omit_unknown_dso,
+                    uint64_t* pvaddr_in_file, Dso** pdso, const Symbol** psymbol);
   bool PrintLostSituationInProtobuf();
   bool PrintFileInfoInProtobuf();
   bool PrintSampleRecord(const SampleRecord& record);
@@ -364,8 +362,9 @@ bool ReportSampleCommand::PrintSampleRecordInProtobuf(const SampleRecord& r) {
   bool in_kernel = r.InKernel();
   const ThreadEntry* thread =
       thread_tree_.FindThreadOrNew(r.tid_data.pid, r.tid_data.tid);
-  GetCallEntry(thread, in_kernel, r.ip_data.ip, &vaddr_in_file, &file_id,
-               &symbol_id);
+  bool ret = GetCallEntry(thread, in_kernel, r.ip_data.ip, false, &vaddr_in_file, &file_id,
+                          &symbol_id);
+  CHECK(ret);
   proto::Sample_CallChainEntry* callchain = sample->add_callchain();
   callchain->set_vaddr_in_file(vaddr_in_file);
   callchain->set_file_id(file_id);
@@ -395,8 +394,9 @@ bool ReportSampleCommand::PrintSampleRecordInProtobuf(const SampleRecord& r) {
             continue;
           }
         }
-        GetCallEntry(thread, in_kernel, ip, &vaddr_in_file, &file_id,
-                     &symbol_id);
+        if (!GetCallEntry(thread, in_kernel, ip, true, &vaddr_in_file, &file_id, &symbol_id)) {
+          break;
+        }
         callchain = sample->add_callchain();
         callchain->set_vaddr_in_file(vaddr_in_file);
         callchain->set_file_id(file_id);
@@ -412,14 +412,17 @@ bool ReportSampleCommand::PrintSampleRecordInProtobuf(const SampleRecord& r) {
   return true;
 }
 
-void ReportSampleCommand::GetCallEntry(const ThreadEntry* thread,
+bool ReportSampleCommand::GetCallEntry(const ThreadEntry* thread,
                                        bool in_kernel, uint64_t ip,
+                                       bool omit_unknown_dso,
                                        uint64_t* pvaddr_in_file,
                                        uint32_t* pfile_id,
                                        int32_t* psymbol_id) {
   Dso* dso;
   const Symbol* symbol;
-  GetCallEntry(thread, in_kernel, ip, pvaddr_in_file, &dso, &symbol);
+  if (!GetCallEntry(thread, in_kernel, ip, omit_unknown_dso, pvaddr_in_file, &dso, &symbol)) {
+    return false;
+  }
   if (!dso->GetDumpId(pfile_id)) {
     *pfile_id = dso->CreateDumpId();
   }
@@ -430,18 +433,24 @@ void ReportSampleCommand::GetCallEntry(const ThreadEntry* thread,
   } else {
     *psymbol_id = -1;
   }
+  return true;
 }
 
-void ReportSampleCommand::GetCallEntry(const ThreadEntry* thread,
+bool ReportSampleCommand::GetCallEntry(const ThreadEntry* thread,
                                        bool in_kernel, uint64_t ip,
+                                       bool omit_unknown_dso,
                                        uint64_t* pvaddr_in_file, Dso** pdso,
                                        const Symbol** psymbol) {
   const MapEntry* map = thread_tree_.FindMap(thread, ip, in_kernel);
+  if (omit_unknown_dso && thread_tree_.IsUnknownDso(map->dso)) {
+    return false;
+  }
   *psymbol = thread_tree_.FindSymbol(map, ip, pvaddr_in_file, pdso);
   // If we can't find symbol, use the dso shown in the map.
   if (*psymbol == thread_tree_.UnknownSymbol()) {
     *pdso = map->dso;
   }
+  return true;
 }
 
 bool ReportSampleCommand::PrintLostSituationInProtobuf() {
@@ -511,7 +520,8 @@ bool ReportSampleCommand::PrintSampleRecord(const SampleRecord& r) {
   bool in_kernel = r.InKernel();
   const ThreadEntry* thread =
       thread_tree_.FindThreadOrNew(r.tid_data.pid, r.tid_data.tid);
-  GetCallEntry(thread, in_kernel, r.ip_data.ip, &vaddr_in_file, &dso, &symbol);
+  bool ret = GetCallEntry(thread, in_kernel, r.ip_data.ip, false, &vaddr_in_file, &dso, &symbol);
+  CHECK(ret);
   FprintIndented(report_fp_, 1, "vaddr_in_file: %" PRIx64 "\n", vaddr_in_file);
   FprintIndented(report_fp_, 1, "file: %s\n", dso->Path().c_str());
   FprintIndented(report_fp_, 1, "symbol: %s\n", symbol->DemangledName());
@@ -541,7 +551,9 @@ bool ReportSampleCommand::PrintSampleRecord(const SampleRecord& r) {
             continue;
           }
         }
-        GetCallEntry(thread, in_kernel, ip, &vaddr_in_file, &dso, &symbol);
+        if (!GetCallEntry(thread, in_kernel, ip, true, &vaddr_in_file, &dso, &symbol)) {
+          break;
+        }
         FprintIndented(report_fp_, 2, "vaddr_in_file: %" PRIx64 "\n",
                        vaddr_in_file);
         FprintIndented(report_fp_, 2, "file: %s\n", dso->Path().c_str());
diff --git a/simpleperf/cmd_report_sample_test.cpp b/simpleperf/cmd_report_sample_test.cpp
index d07f0fd9..81d90988 100644
--- a/simpleperf/cmd_report_sample_test.cpp
+++ b/simpleperf/cmd_report_sample_test.cpp
@@ -54,3 +54,16 @@ TEST(cmd_report_sample, protobuf_option) {
   ASSERT_TRUE(android::base::ReadFileToString(tmpfile2.path, &data));
   ASSERT_NE(data.find("file:"), std::string::npos);
 }
+
+TEST(cmd_report_sample, no_skipped_file_id) {
+  TemporaryFile tmpfile;
+  TemporaryFile tmpfile2;
+  ASSERT_TRUE(ReportSampleCmd()->Run({"-i", GetTestData(PERF_DATA_WITH_WRONG_IP_IN_CALLCHAIN),
+                                     "-o", tmpfile.path, "--protobuf"}));
+  ASSERT_TRUE(ReportSampleCmd()->Run({"--dump-protobuf-report", tmpfile.path, "-o",
+                                      tmpfile2.path}));
+  // If wrong ips in callchain are omitted, "unknown" file path will not be generated.
+  std::string data;
+  ASSERT_TRUE(android::base::ReadFileToString(tmpfile2.path, &data));
+  ASSERT_EQ(data.find("unknown"), std::string::npos);
+}
diff --git a/simpleperf/get_test_data.h b/simpleperf/get_test_data.h
index 9fb2293c..16659dca 100644
--- a/simpleperf/get_test_data.h
+++ b/simpleperf/get_test_data.h
@@ -105,4 +105,7 @@ static const std::string PERF_DATA_MAX_STACK_AND_PERCENT_LIMIT = "perf_test_max_
 // generated by `dd if=/dev/zero of=invalid_perf.data bs=1024 count=1`.
 static const std::string INVALID_PERF_DATA = "invalid_perf.data";
 
+// generated by recording an app.
+static const std::string PERF_DATA_WITH_WRONG_IP_IN_CALLCHAIN = "wrong_ip_callchain_perf.data";
+
 #endif  // SIMPLE_PERF_GET_TEST_DATA_H_
diff --git a/simpleperf/testdata/wrong_ip_callchain_perf.data b/simpleperf/testdata/wrong_ip_callchain_perf.data
new file mode 100644
index 00000000..633f2d10
--- /dev/null
+++ b/simpleperf/testdata/wrong_ip_callchain_perf.data
diff --git a/simpleperf/thread_tree.cpp b/simpleperf/thread_tree.cpp
index c40607dc..9def6f21 100644
--- a/simpleperf/thread_tree.cpp
+++ b/simpleperf/thread_tree.cpp
@@ -322,6 +322,7 @@ std::vector<Dso*> ThreadTree::GetAllDsos() const {
   for (auto& p : user_dso_tree_) {
     result.push_back(p.second.get());
   }
+  result.push_back(unknown_dso_.get());
   return result;
 }
 
diff --git a/simpleperf/thread_tree.h b/simpleperf/thread_tree.h
index e6536bb2..79a4439c 100644
--- a/simpleperf/thread_tree.h
+++ b/simpleperf/thread_tree.h
@@ -100,6 +100,7 @@ class ThreadTree {
   const Symbol* FindSymbol(const MapEntry* map, uint64_t ip,
                            uint64_t* pvaddr_in_file, Dso** pdso = nullptr);
   const Symbol* FindKernelSymbol(uint64_t ip);
+  bool IsUnknownDso(const Dso* dso) const { return dso == unknown_dso_.get(); }
   const Symbol* UnknownSymbol() const { return &unknown_symbol_; }
 
   void ShowIpForUnknownSymbol() { show_ip_for_unknown_symbol_ = true; }