From 6f14272775fdf15ea8d7d616bc75ce6103a2226b Mon Sep 17 00:00:00 2001 From: Yabin Cui Date: Wed, 7 Mar 2018 15:47:15 -0800 Subject: 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) --- simpleperf/CallChainJoiner.cpp | 2 +- simpleperf/CallChainJoiner_test.cpp | 5 ++++- simpleperf/cmd_debug_unwind.cpp | 8 ++++---- simpleperf/cmd_record.cpp | 15 +++++++-------- simpleperf/dso.cpp | 20 +++++++++----------- simpleperf/dso.h | 6 +++--- simpleperf/environment.cpp | 34 +++++++++++++++++++++++++--------- simpleperf/environment.h | 14 ++++++++++++-- simpleperf/environment_test.cpp | 2 +- 9 files changed, 66 insertions(+), 40 deletions(-) diff --git a/simpleperf/CallChainJoiner.cpp b/simpleperf/CallChainJoiner.cpp index b93df403..c3a3f940 100644 --- 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 tmpfile = CreateTempFileUsedInRecording(); + std::unique_ptr 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 3998e6cf..f33b97d5 100644 --- 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 scoped_temp_files_; }; TEST_F(CallChainJoinerTest, smoke) { diff --git a/simpleperf/cmd_debug_unwind.cpp b/simpleperf/cmd_debug_unwind.cpp index ccb0b153..ed962f9e 100644 --- a/simpleperf/cmd_debug_unwind.cpp +++ b/simpleperf/cmd_debug_unwind.cpp @@ -137,6 +137,7 @@ bool DebugUnwindCommand::Run(const std::vector& 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& 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 tmpfile = CreateTempFileUsedInRecording(); - if (!Workload::RunCmd({"mv", output_filename_, tmpfile->path})) { + std::unique_ptr 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 reader = RecordFileReader::CreateInstance(tmpfile->path); + std::unique_ptr reader = RecordFileReader::CreateInstance(tmp_file->path); if (!reader) { return false; } diff --git a/simpleperf/cmd_record.cpp b/simpleperf/cmd_record.cpp index 13313e0d..bb659c7f 100644 --- a/simpleperf/cmd_record.cpp +++ b/simpleperf/cmd_record.cpp @@ -314,6 +314,7 @@ bool RecordCommand::Run(const std::vector& 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& 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 tmpfile = CreateTempFileUsedInRecording(); - if (!Workload::RunCmd({"mv", record_filename_, tmpfile->path})) { + std::unique_ptr tmp_file = ScopedTempFiles::CreateTempFile(); + if (!Workload::RunCmd({"mv", record_filename_, tmp_file->path})) { return false; } - std::unique_ptr reader = RecordFileReader::CreateInstance(tmpfile->path); + std::unique_ptr 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 tmpfile = CreateTempFileUsedInRecording(); - if (!Workload::RunCmd({"mv", record_filename_, tmpfile->path})) { + std::unique_ptr 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 reader = RecordFileReader::CreateInstance(tmpfile->path); + std::unique_ptr 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 7959d4b9..4825ac72 100644 --- a/simpleperf/dso.cpp +++ b/simpleperf/dso.cpp @@ -60,8 +60,8 @@ bool Dso::read_kernel_symbols_from_proc_; std::unordered_map Dso::build_id_map_; size_t Dso::dso_count_; uint32_t Dso::g_dump_id_; -std::unique_ptr Dso::vdso_64bit_; -std::unique_ptr 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 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 cb0e51d7..4f3df0bd 100644 --- a/simpleperf/dso.h +++ b/simpleperf/dso.h @@ -100,7 +100,7 @@ class Dso { static void SetBuildIds( const std::vector>& build_ids); static BuildId FindExpectedBuildIdForPath(const std::string& path); - static void SetVdsoFile(std::unique_ptr vdso_file, bool is_64bit); + static void SetVdsoFile(const std::string& vdso_file, bool is_64bit); static std::unique_ptr CreateDso(DsoType dso_type, const std::string& dso_path, bool force_64bit = false); @@ -153,8 +153,8 @@ class Dso { static std::unordered_map build_id_map_; static size_t dso_count_; static uint32_t g_dump_id_; - static std::unique_ptr vdso_64bit_; - static std::unique_ptr 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 e89757a6..48f4ad54 100644 --- 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(static_cast(vdso_map->start_addr)), vdso_map->len); - std::unique_ptr tmpfile = CreateTempFileUsedInRecording(); - if (!android::base::WriteStringToFile(s, tmpfile->path)) { + std::unique_ptr 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 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 CreateTempFileUsedInRecording() { - CHECK(!temp_dir_used_in_recording.empty()); - return std::unique_ptr(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 ScopedTempFiles::CreateTempFile(bool delete_in_destructor) { + CHECK(!tmp_dir_.empty()); + std::unique_ptr 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 33e4c680..84794f2c 100644 --- 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 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 CreateTempFile(bool delete_in_destructor = true); + + private: + static std::string tmp_dir_; + static std::vector files_to_delete_; +}; bool SignalIsIgnored(int signo); diff --git a/simpleperf/environment_test.cpp b/simpleperf/environment_test.cpp index 25eee968..5b7c81e6 100644 --- 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::CreateDso(DSO_ELF_FILE, "[vdso]", sizeof(size_t) == sizeof(uint64_t)); -- cgit v1.2.3