simpleperf: improve managing temp files.

Instead of relying on callers to delete temp files, support managing
all temp files in ScopedTempFiles.

Bug: http://b/73127105
Test: run simpleperf_unit_test and simpleperf manually.
Change-Id: Ib73065754657320ebd244f676e3f851544ff2718
(cherry picked from commit c68e66dcf067c052319c8da13a6a49ff06481fa5)

diff --git a/simpleperf/CallChainJoiner.cpp b/simpleperf/CallChainJoiner.cpp
index b93df40..c3a3f94 100644 (file)
--- a/simpleperf/CallChainJoiner.cpp
+++ b/simpleperf/CallChainJoiner.cpp
@@ -259,7 +259,7 @@ static bool ReadCallChainInReverseOrder(FILE* fp, pid_t& pid, pid_t& tid,
 }
 
 static FILE* CreateTempFp() {
-  std::unique_ptr<TemporaryFile> tmpfile = CreateTempFileUsedInRecording();
+  std::unique_ptr<TemporaryFile> tmpfile = ScopedTempFiles::CreateTempFile();
   FILE* fp = fdopen(tmpfile->release(), "web+");
   if (fp == nullptr) {
     PLOG(ERROR) << "fdopen";

diff --git a/simpleperf/CallChainJoiner_test.cpp b/simpleperf/CallChainJoiner_test.cpp
index 3998e6c..f33b97d 100644 (file)
--- a/simpleperf/CallChainJoiner_test.cpp
+++ b/simpleperf/CallChainJoiner_test.cpp
@@ -158,8 +158,11 @@ class CallChainJoinerTest : public ::testing::Test {
 #else
     std::string tmpdir = "/tmp";
 #endif
-    SetTempDirectoryUsedInRecording(tmpdir);
+    scoped_temp_files_.reset(new ScopedTempFiles(tmpdir));
   }
+
+ private:
+  std::unique_ptr<ScopedTempFiles> scoped_temp_files_;
 };
 
 TEST_F(CallChainJoinerTest, smoke) {

diff --git a/simpleperf/cmd_debug_unwind.cpp b/simpleperf/cmd_debug_unwind.cpp
index ccb0b15..ed962f9 100644 (file)
--- a/simpleperf/cmd_debug_unwind.cpp
+++ b/simpleperf/cmd_debug_unwind.cpp
@@ -137,6 +137,7 @@ bool DebugUnwindCommand::Run(const std::vector<std::string>& args) {
   if (!ParseOptions(args)) {
     return false;
   }
+  ScopedTempFiles scoped_temp_files(android::base::Dirname(output_filename_));
 
   // 2. Read input perf.data, and generate new perf.data.
   if (!UnwindRecordFile()) {
@@ -180,7 +181,6 @@ bool DebugUnwindCommand::ParseOptions(const std::vector<std::string>& args) {
       return false;
     }
   }
-  SetTempDirectoryUsedInRecording(android::base::Dirname(output_filename_));
   return true;
 }
 
@@ -299,14 +299,14 @@ bool DebugUnwindCommand::JoinCallChains() {
     return false;
   }
   writer_.reset();
-  std::unique_ptr<TemporaryFile> tmpfile = CreateTempFileUsedInRecording();
-  if (!Workload::RunCmd({"mv", output_filename_, tmpfile->path})) {
+  std::unique_ptr<TemporaryFile> tmp_file = ScopedTempFiles::CreateTempFile();
+  if (!Workload::RunCmd({"mv", output_filename_, tmp_file->path})) {
     return false;
   }
 
   // 3. Read records from the temporary file, and write records with joined call chains back
   // to record_filename_.
-  std::unique_ptr<RecordFileReader> reader = RecordFileReader::CreateInstance(tmpfile->path);
+  std::unique_ptr<RecordFileReader> reader = RecordFileReader::CreateInstance(tmp_file->path);
   if (!reader) {
     return false;
   }

diff --git a/simpleperf/cmd_record.cpp b/simpleperf/cmd_record.cpp
index 13313e0..bb659c7 100644 (file)
--- a/simpleperf/cmd_record.cpp
+++ b/simpleperf/cmd_record.cpp
@@ -314,6 +314,7 @@ bool RecordCommand::Run(const std::vector<std::string>& args) {
   if (!ParseOptions(args, &workload_args)) {
     return false;
   }
+  ScopedTempFiles scoped_temp_files(android::base::Dirname(record_filename_));
   if (!app_package_name_.empty() && !in_app_context_) {
     // Some users want to profile non debuggable apps on rooted devices. If we use run-as,
     // it will be impossible when using --app. So don't switch to app's context when we are
@@ -802,8 +803,6 @@ bool RecordCommand::ParseOptions(const std::vector<std::string>& args,
   for (; i < args.size(); ++i) {
     non_option_args->push_back(args[i]);
   }
-
-  SetTempDirectoryUsedInRecording(android::base::Dirname(record_filename_));
   return true;
 }
 
@@ -1141,11 +1140,11 @@ bool RecordCommand::PostUnwindRecords() {
     return false;
   }
   record_file_writer_.reset();
-  std::unique_ptr<TemporaryFile> tmpfile = CreateTempFileUsedInRecording();
-  if (!Workload::RunCmd({"mv", record_filename_, tmpfile->path})) {
+  std::unique_ptr<TemporaryFile> tmp_file = ScopedTempFiles::CreateTempFile();
+  if (!Workload::RunCmd({"mv", record_filename_, tmp_file->path})) {
     return false;
   }
-  std::unique_ptr<RecordFileReader> reader = RecordFileReader::CreateInstance(tmpfile->path);
+  std::unique_ptr<RecordFileReader> reader = RecordFileReader::CreateInstance(tmp_file->path);
   if (!reader) {
     return false;
   }
@@ -1173,14 +1172,14 @@ bool RecordCommand::JoinCallChains() {
     return false;
   }
   record_file_writer_.reset();
-  std::unique_ptr<TemporaryFile> tmpfile = CreateTempFileUsedInRecording();
-  if (!Workload::RunCmd({"mv", record_filename_, tmpfile->path})) {
+  std::unique_ptr<TemporaryFile> tmp_file = ScopedTempFiles::CreateTempFile();
+  if (!Workload::RunCmd({"mv", record_filename_, tmp_file->path})) {
     return false;
   }
 
   // 3. Read records from the temporary file, and write record with joined call chains back
   // to record_filename_.
-  std::unique_ptr<RecordFileReader> reader = RecordFileReader::CreateInstance(tmpfile->path);
+  std::unique_ptr<RecordFileReader> reader = RecordFileReader::CreateInstance(tmp_file->path);
   record_file_writer_ = CreateRecordFile(record_filename_);
   if (!reader || !record_file_writer_) {
     return false;

diff --git a/simpleperf/dso.cpp b/simpleperf/dso.cpp
index 7959d4b..4825ac7 100644 (file)
--- a/simpleperf/dso.cpp
+++ b/simpleperf/dso.cpp
@@ -60,8 +60,8 @@ bool Dso::read_kernel_symbols_from_proc_;
 std::unordered_map<std::string, BuildId> Dso::build_id_map_;
 size_t Dso::dso_count_;
 uint32_t Dso::g_dump_id_;
-std::unique_ptr<TemporaryFile> Dso::vdso_64bit_;
-std::unique_ptr<TemporaryFile> Dso::vdso_32bit_;
+std::string Dso::vdso_64bit_;
+std::string Dso::vdso_32bit_;
 
 void Dso::SetDemangle(bool demangle) { demangle_ = demangle; }
 
@@ -121,11 +121,11 @@ void Dso::SetBuildIds(
   build_id_map_ = std::move(map);
 }
 
-void Dso::SetVdsoFile(std::unique_ptr<TemporaryFile> vdso_file, bool is_64bit) {
+void Dso::SetVdsoFile(const std::string& vdso_file, bool is_64bit) {
   if (is_64bit) {
-    vdso_64bit_ = std::move(vdso_file);
+    vdso_64bit_ = vdso_file;
   } else {
-    vdso_32bit_ = std::move(vdso_file);
+    vdso_32bit_ = vdso_file;
   }
 }
 
@@ -170,10 +170,10 @@ Dso::Dso(DsoType type, const std::string& path, bool force_64bit)
       debug_file_path_ = path_in_symfs;
     }
   } else if (path == "[vdso]") {
-    if (force_64bit && vdso_64bit_ != nullptr) {
-      debug_file_path_ = vdso_64bit_->path;
-    } else if (!force_64bit && vdso_32bit_ != nullptr) {
-      debug_file_path_ = vdso_32bit_->path;
+    if (force_64bit && !vdso_64bit_.empty()) {
+      debug_file_path_ = vdso_64bit_;
+    } else if (!force_64bit && !vdso_32bit_.empty()) {
+      debug_file_path_ = vdso_32bit_;
     }
   }
   size_t pos = path.find_last_of("/\\");
@@ -196,8 +196,6 @@ Dso::~Dso() {
     read_kernel_symbols_from_proc_ = false;
     build_id_map_.clear();
     g_dump_id_ = 0;
-    vdso_64bit_ = nullptr;
-    vdso_32bit_ = nullptr;
   }
 }
 

diff --git a/simpleperf/dso.h b/simpleperf/dso.h
index cb0e51d..4f3df0b 100644 (file)
--- a/simpleperf/dso.h
+++ b/simpleperf/dso.h
@@ -100,7 +100,7 @@ class Dso {
   static void SetBuildIds(
       const std::vector<std::pair<std::string, BuildId>>& build_ids);
   static BuildId FindExpectedBuildIdForPath(const std::string& path);
-  static void SetVdsoFile(std::unique_ptr<TemporaryFile> vdso_file, bool is_64bit);
+  static void SetVdsoFile(const std::string& vdso_file, bool is_64bit);
 
   static std::unique_ptr<Dso> CreateDso(DsoType dso_type, const std::string& dso_path,
                                         bool force_64bit = false);
@@ -153,8 +153,8 @@ class Dso {
   static std::unordered_map<std::string, BuildId> build_id_map_;
   static size_t dso_count_;
   static uint32_t g_dump_id_;
-  static std::unique_ptr<TemporaryFile> vdso_64bit_;
-  static std::unique_ptr<TemporaryFile> vdso_32bit_;
+  static std::string vdso_64bit_;
+  static std::string vdso_32bit_;
 
   Dso(DsoType type, const std::string& path, bool force_64bit);
   void Load();

diff --git a/simpleperf/environment.cpp b/simpleperf/environment.cpp
index e89757a..48f4ad5 100644 (file)
--- a/simpleperf/environment.cpp
+++ b/simpleperf/environment.cpp
@@ -487,11 +487,11 @@ void PrepareVdsoFile() {
   std::string s(vdso_map->len, '\0');
   memcpy(&s[0], reinterpret_cast<void*>(static_cast<uintptr_t>(vdso_map->start_addr)),
          vdso_map->len);
-  std::unique_ptr<TemporaryFile> tmpfile = CreateTempFileUsedInRecording();
-  if (!android::base::WriteStringToFile(s, tmpfile->path)) {
+  std::unique_ptr<TemporaryFile> tmpfile = ScopedTempFiles::CreateTempFile();
+  if (!android::base::WriteStringToFd(s, tmpfile->release())) {
     return;
   }
-  Dso::SetVdsoFile(std::move(tmpfile), sizeof(size_t) == sizeof(uint64_t));
+  Dso::SetVdsoFile(tmpfile->path, sizeof(size_t) == sizeof(uint64_t));
 }
 
 static bool HasOpenedAppApkFile(int pid) {
@@ -689,14 +689,30 @@ void AllowMoreOpenedFiles() {
   }
 }
 
-static std::string temp_dir_used_in_recording;
-void SetTempDirectoryUsedInRecording(const std::string& tmp_dir) {
-  temp_dir_used_in_recording = tmp_dir;
+std::string ScopedTempFiles::tmp_dir_;
+std::vector<std::string> ScopedTempFiles::files_to_delete_;
+
+ScopedTempFiles::ScopedTempFiles(const std::string& tmp_dir) {
+  CHECK(tmp_dir_.empty());  // No other ScopedTempFiles.
+  tmp_dir_ = tmp_dir;
 }
 
-std::unique_ptr<TemporaryFile> CreateTempFileUsedInRecording() {
-  CHECK(!temp_dir_used_in_recording.empty());
-  return std::unique_ptr<TemporaryFile>(new TemporaryFile(temp_dir_used_in_recording));
+ScopedTempFiles::~ScopedTempFiles() {
+  tmp_dir_.clear();
+  for (auto& file : files_to_delete_) {
+    unlink(file.c_str());
+  }
+  files_to_delete_.clear();
+}
+
+std::unique_ptr<TemporaryFile> ScopedTempFiles::CreateTempFile(bool delete_in_destructor) {
+  CHECK(!tmp_dir_.empty());
+  std::unique_ptr<TemporaryFile> tmp_file(new TemporaryFile(tmp_dir_));
+  CHECK_NE(tmp_file->fd, -1);
+  if (delete_in_destructor) {
+    files_to_delete_.push_back(tmp_file->path);
+  }
+  return tmp_file;
 }
 
 bool SignalIsIgnored(int signo) {

diff --git a/simpleperf/environment.h b/simpleperf/environment.h
index 33e4c68..84794f2 100644 (file)
--- a/simpleperf/environment.h
+++ b/simpleperf/environment.h
@@ -103,8 +103,18 @@ void SetDefaultAppPackageName(const std::string& package_name);
 const std::string& GetDefaultAppPackageName();
 void AllowMoreOpenedFiles();
 
-void SetTempDirectoryUsedInRecording(const std::string& tmp_dir);
-std::unique_ptr<TemporaryFile> CreateTempFileUsedInRecording();
+class ScopedTempFiles {
+ public:
+  ScopedTempFiles(const std::string& tmp_dir);
+  ~ScopedTempFiles();
+  // If delete_in_destructor = true, the temp file will be deleted in the destructor of
+  // ScopedTempFile. Otherwise, it should be deleted by the caller.
+  static std::unique_ptr<TemporaryFile> CreateTempFile(bool delete_in_destructor = true);
+
+ private:
+  static std::string tmp_dir_;
+  static std::vector<std::string> files_to_delete_;
+};
 
 bool SignalIsIgnored(int signo);
 

diff --git a/simpleperf/environment_test.cpp b/simpleperf/environment_test.cpp
index 25eee96..5b7c81e 100644 (file)
--- a/simpleperf/environment_test.cpp
+++ b/simpleperf/environment_test.cpp
@@ -37,7 +37,7 @@ TEST(environment, PrepareVdsoFile) {
     return;
   }
   TemporaryDir tmpdir;
-  SetTempDirectoryUsedInRecording(tmpdir.path);
+  ScopedTempFiles scoped_temp_files(tmpdir.path);
   PrepareVdsoFile();
   std::unique_ptr<Dso> dso = Dso::CreateDso(DSO_ELF_FILE, "[vdso]",
                                             sizeof(size_t) == sizeof(uint64_t));