Write record allocs from interrupt handler.
There are many cases where there are no more allocations
when a dump of the record allocs might occur. Move the entries
being written to file in the interrupt handler.
Update the unit tests for this new functionality and add a new
unit test that verifies no allocations occur while the entries
are being written.
Fix the unit tests so that the record allocs files get deleted
properly.
Test: All unit tests pass.
Test: Ran 1000 iterations of unit tests to verify no flakes.
Change-Id: I20941596c1dda5a761522050dc155b06f3f3e735
diff --git a/libc/malloc_debug/RecordData.cpp b/libc/malloc_debug/RecordData.cpp
index 5c550c0..a829a09 100644
--- a/libc/malloc_debug/RecordData.cpp
+++ b/libc/malloc_debug/RecordData.cpp
@@ -48,44 +48,44 @@
RecordEntry::RecordEntry() : tid_(gettid()) {
}
-std::string ThreadCompleteEntry::GetString() const {
- return android::base::StringPrintf("%d: thread_done 0x0\n", tid_);
+bool ThreadCompleteEntry::Write(int fd) const {
+ return dprintf(fd, "%d: thread_done 0x0\n", tid_) > 0;
}
AllocEntry::AllocEntry(void* pointer) : pointer_(pointer) {}
MallocEntry::MallocEntry(void* pointer, size_t size) : AllocEntry(pointer), size_(size) {}
-std::string MallocEntry::GetString() const {
- return android::base::StringPrintf("%d: malloc %p %zu\n", tid_, pointer_, size_);
+bool MallocEntry::Write(int fd) const {
+ return dprintf(fd, "%d: malloc %p %zu\n", tid_, pointer_, size_) > 0;
}
FreeEntry::FreeEntry(void* pointer) : AllocEntry(pointer) {}
-std::string FreeEntry::GetString() const {
- return android::base::StringPrintf("%d: free %p\n", tid_, pointer_);
+bool FreeEntry::Write(int fd) const {
+ return dprintf(fd, "%d: free %p\n", tid_, pointer_) > 0;
}
CallocEntry::CallocEntry(void* pointer, size_t nmemb, size_t size)
: MallocEntry(pointer, size), nmemb_(nmemb) {}
-std::string CallocEntry::GetString() const {
- return android::base::StringPrintf("%d: calloc %p %zu %zu\n", tid_, pointer_, nmemb_, size_);
+bool CallocEntry::Write(int fd) const {
+ return dprintf(fd, "%d: calloc %p %zu %zu\n", tid_, pointer_, nmemb_, size_) > 0;
}
ReallocEntry::ReallocEntry(void* pointer, size_t size, void* old_pointer)
: MallocEntry(pointer, size), old_pointer_(old_pointer) {}
-std::string ReallocEntry::GetString() const {
- return android::base::StringPrintf("%d: realloc %p %p %zu\n", tid_, pointer_, old_pointer_, size_);
+bool ReallocEntry::Write(int fd) const {
+ return dprintf(fd, "%d: realloc %p %p %zu\n", tid_, pointer_, old_pointer_, size_) > 0;
}
// aligned_alloc, posix_memalign, memalign, pvalloc, valloc all recorded with this class.
MemalignEntry::MemalignEntry(void* pointer, size_t size, size_t alignment)
: MallocEntry(pointer, size), alignment_(alignment) {}
-std::string MemalignEntry::GetString() const {
- return android::base::StringPrintf("%d: memalign %p %zu %zu\n", tid_, pointer_, alignment_, size_);
+bool MemalignEntry::Write(int fd) const {
+ return dprintf(fd, "%d: memalign %p %zu %zu\n", tid_, pointer_, alignment_, size_) > 0;
}
struct ThreadData {
@@ -112,59 +112,37 @@
}
}
-static void RecordDump(int, siginfo_t*, void*) {
- // It's not necessarily safe to do the dump here, instead wait for the
- // next allocation call to do the dump.
- g_debug->record->SetToDump();
+RecordData* RecordData::record_obj_ = nullptr;
+
+void RecordData::WriteData(int, siginfo_t*, void*) {
+ // Dump from here, the function must not allocate so this is safe.
+ record_obj_->WriteEntries();
}
-void RecordData::Dump() {
- std::lock_guard<std::mutex> lock(dump_lock_);
-
- // Make it so that no more entries can be added while dumping.
- unsigned int last_entry_index = cur_index_.exchange(static_cast<unsigned int>(num_entries_));
- if (dump_ == false) {
- // Multiple Dump() calls from different threads, and we lost. Do nothing.
+void RecordData::WriteEntries() {
+ std::lock_guard<std::mutex> entries_lock(entries_lock_);
+ if (cur_index_ == 0) {
+ info_log("No alloc entries to write.");
return;
}
- // cur_index_ keeps getting incremented even if we hit the num_entries_.
- // If that happens, cap the entries to dump by num_entries_.
- if (last_entry_index > num_entries_) {
- last_entry_index = num_entries_;
- }
-
int dump_fd =
open(dump_file_.c_str(), O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC | O_NOFOLLOW, 0755);
- if (dump_fd != -1) {
- for (size_t i = 0; i < last_entry_index; i++) {
- std::string line = entries_[i]->GetString();
- ssize_t bytes = write(dump_fd, line.c_str(), line.length());
- if (bytes == -1 || static_cast<size_t>(bytes) != line.length()) {
- error_log("Failed to write record alloc information: %s", strerror(errno));
- // Free all of the rest of the errors, we don't have any way
- // to dump a partial list of the entries.
- for (i++; i < last_entry_index; i++) {
- delete entries_[i];
- entries_[i] = nullptr;
- }
- break;
- }
- delete entries_[i];
- entries_[i] = nullptr;
- }
- close(dump_fd);
-
- // Mark the entries dumped.
- cur_index_ = 0U;
- } else {
+ if (dump_fd == -1) {
error_log("Cannot create record alloc file %s: %s", dump_file_.c_str(), strerror(errno));
- // Since we couldn't create the file, reset the entries dumped back
- // to the original value.
- cur_index_ = last_entry_index;
+ return;
}
- dump_ = false;
+ for (size_t i = 0; i < cur_index_; i++) {
+ if (!entries_[i]->Write(dump_fd)) {
+ error_log("Failed to write record alloc information: %s", strerror(errno));
+ break;
+ }
+ }
+ close(dump_fd);
+
+ // Mark the entries dumped.
+ cur_index_ = 0U;
}
RecordData::RecordData() {
@@ -172,8 +150,9 @@
}
bool RecordData::Initialize(const Config& config) {
+ record_obj_ = this;
struct sigaction64 dump_act = {};
- dump_act.sa_sigaction = RecordDump;
+ dump_act.sa_sigaction = RecordData::WriteData;
dump_act.sa_flags = SA_RESTART | SA_SIGINFO | SA_ONSTACK;
if (sigaction64(config.record_allocs_signal(), &dump_act, nullptr) != 0) {
error_log("Unable to set up record dump signal function: %s", strerror(errno));
@@ -186,24 +165,27 @@
config.record_allocs_signal(), getpid());
}
- num_entries_ = config.record_allocs_num_entries();
- entries_ = new const RecordEntry*[num_entries_];
- cur_index_ = 0;
- dump_ = false;
+ entries_.resize(config.record_allocs_num_entries());
+ cur_index_ = 0U;
dump_file_ = config.record_allocs_file();
return true;
}
RecordData::~RecordData() {
- delete[] entries_;
pthread_key_delete(key_);
}
void RecordData::AddEntryOnly(const RecordEntry* entry) {
- unsigned int entry_index = cur_index_.fetch_add(1);
- if (entry_index < num_entries_) {
- entries_[entry_index] = entry;
+ std::lock_guard<std::mutex> entries_lock(entries_lock_);
+ if (cur_index_ == entries_.size()) {
+ // Maxed out, throw the entry away.
+ return;
+ }
+
+ entries_[cur_index_++].reset(entry);
+ if (cur_index_ == entries_.size()) {
+ info_log("Maximum number of records added, all new operations will be dropped.");
}
}
@@ -215,9 +197,4 @@
}
AddEntryOnly(entry);
-
- // Check to see if it's time to dump the entries.
- if (dump_) {
- Dump();
- }
}
diff --git a/libc/malloc_debug/RecordData.h b/libc/malloc_debug/RecordData.h
index 3d37529..43dba6a 100644
--- a/libc/malloc_debug/RecordData.h
+++ b/libc/malloc_debug/RecordData.h
@@ -29,12 +29,15 @@
#pragma once
#include <pthread.h>
+#include <signal.h>
#include <stdint.h>
#include <unistd.h>
#include <atomic>
+#include <memory>
#include <mutex>
#include <string>
+#include <vector>
#include <platform/bionic/macros.h>
@@ -43,7 +46,7 @@
RecordEntry();
virtual ~RecordEntry() = default;
- virtual std::string GetString() const = 0;
+ virtual bool Write(int fd) const = 0;
protected:
pid_t tid_;
@@ -57,7 +60,7 @@
ThreadCompleteEntry() = default;
virtual ~ThreadCompleteEntry() = default;
- std::string GetString() const override;
+ bool Write(int fd) const override;
private:
BIONIC_DISALLOW_COPY_AND_ASSIGN(ThreadCompleteEntry);
@@ -80,7 +83,7 @@
MallocEntry(void* pointer, size_t size);
virtual ~MallocEntry() = default;
- std::string GetString() const override;
+ bool Write(int fd) const override;
protected:
size_t size_;
@@ -94,7 +97,7 @@
explicit FreeEntry(void* pointer);
virtual ~FreeEntry() = default;
- std::string GetString() const override;
+ bool Write(int fd) const override;
private:
BIONIC_DISALLOW_COPY_AND_ASSIGN(FreeEntry);
@@ -105,7 +108,7 @@
CallocEntry(void* pointer, size_t size, size_t nmemb);
virtual ~CallocEntry() = default;
- std::string GetString() const override;
+ bool Write(int fd) const override;
protected:
size_t nmemb_;
@@ -119,7 +122,7 @@
ReallocEntry(void* pointer, size_t size, void* old_pointer);
virtual ~ReallocEntry() = default;
- std::string GetString() const override;
+ bool Write(int fd) const override;
protected:
void* old_pointer_;
@@ -134,7 +137,7 @@
MemalignEntry(void* pointer, size_t size, size_t alignment);
virtual ~MemalignEntry() = default;
- std::string GetString() const override;
+ bool Write(int fd) const override;
protected:
size_t alignment_;
@@ -155,19 +158,18 @@
void AddEntry(const RecordEntry* entry);
void AddEntryOnly(const RecordEntry* entry);
- void SetToDump() { dump_ = true; }
-
pthread_key_t key() { return key_; }
private:
- void Dump();
+ static void WriteData(int, siginfo_t*, void*);
+ static RecordData* record_obj_;
- std::mutex dump_lock_;
+ void WriteEntries();
+
+ std::mutex entries_lock_;
pthread_key_t key_;
- const RecordEntry** entries_ = nullptr;
- size_t num_entries_ = 0;
- std::atomic_uint cur_index_;
- std::atomic_bool dump_;
+ std::vector<std::unique_ptr<const RecordEntry>> entries_;
+ size_t cur_index_;
std::string dump_file_;
BIONIC_DISALLOW_COPY_AND_ASSIGN(RecordData);
diff --git a/libc/malloc_debug/tests/malloc_debug_unit_tests.cpp b/libc/malloc_debug/tests/malloc_debug_unit_tests.cpp
index 961ddd3..c6378f5 100644
--- a/libc/malloc_debug/tests/malloc_debug_unit_tests.cpp
+++ b/libc/malloc_debug/tests/malloc_debug_unit_tests.cpp
@@ -81,6 +81,9 @@
bool debug_write_malloc_leak_info(FILE*);
void debug_dump_heap(const char*);
+void malloc_enable();
+void malloc_disable();
+
__END_DECLS
// Change the slow threshold since some tests take more than 2 seconds.
@@ -108,16 +111,16 @@
initialized = false;
resetLogs();
backtrace_fake_clear_all();
- if (!record_filename.empty()) {
- // Delete the record data file if it exists.
- unlink(record_filename.c_str());
- }
}
void TearDown() override {
if (initialized) {
debug_finalize();
}
+ if (!record_filename.empty()) {
+ // Try to delete the record data file even it doesn't exist.
+ unlink(record_filename.c_str());
+ }
}
void Init(const char* options) {
@@ -2227,12 +2230,6 @@
// Dump all of the data accumulated so far.
ASSERT_TRUE(kill(getpid(), SIGRTMAX - 18) == 0);
- sleep(1);
-
- // This triggers the dumping.
- pointer = debug_malloc(110);
- ASSERT_TRUE(pointer != nullptr);
- expected += android::base::StringPrintf("%d: malloc %p 110\n", getpid(), pointer);
// Read all of the contents.
std::string actual;
@@ -2242,8 +2239,6 @@
ASSERT_STREQ("", getFakeLogBuf().c_str());
ASSERT_STREQ("", getFakeLogPrint().c_str());
-
- debug_free(pointer);
}
TEST_F(MallocDebugTest, record_allocs_no_header) {
@@ -2282,11 +2277,6 @@
// Dump all of the data accumulated so far.
ASSERT_TRUE(kill(getpid(), SIGRTMAX - 18) == 0);
- sleep(1);
-
- // This triggers the dumping.
- pointer = debug_malloc(110);
- ASSERT_TRUE(pointer != nullptr);
// Read all of the contents.
std::string actual;
@@ -2295,9 +2285,9 @@
ASSERT_STREQ(expected.c_str(), actual.c_str());
ASSERT_STREQ("", getFakeLogBuf().c_str());
- ASSERT_STREQ("", getFakeLogPrint().c_str());
-
- debug_free(pointer);
+ ASSERT_STREQ(
+ "4 malloc_debug Maximum number of records added, all new operations will be dropped.\n",
+ getFakeLogPrint().c_str());
}
TEST_F(MallocDebugTest, record_allocs_thread_done) {
@@ -2319,12 +2309,6 @@
// Dump all of the data accumulated so far.
ASSERT_TRUE(kill(getpid(), SIGRTMAX - 18) == 0);
- sleep(1);
-
- // This triggers the dumping.
- pointer = debug_malloc(23);
- ASSERT_TRUE(pointer != nullptr);
- expected += android::base::StringPrintf("%d: malloc %p 23\n", getpid(), pointer);
// Read all of the contents.
std::string actual;
@@ -2334,14 +2318,12 @@
ASSERT_STREQ("", getFakeLogBuf().c_str());
ASSERT_STREQ("", getFakeLogPrint().c_str());
-
- debug_free(pointer);
}
TEST_F(MallocDebugTest, record_allocs_file_name_fail) {
InitRecordAllocs("record_allocs=5");
- // Delete the special.txt file and create a symbolic link there to
+ // Delete the records file and create a symbolic link there to
// make sure the create file will fail.
unlink(record_filename.c_str());
@@ -2357,12 +2339,6 @@
// Dump all of the data accumulated so far.
ASSERT_TRUE(kill(getpid(), SIGRTMAX - 18) == 0);
- sleep(1);
-
- // This triggers the dumping.
- pointer = debug_malloc(110);
- ASSERT_TRUE(pointer != nullptr);
- expected += android::base::StringPrintf("%d: malloc %p 110\n", getpid(), pointer);
// Read all of the contents.
std::string actual;
@@ -2373,11 +2349,6 @@
// Dump all of the data accumulated so far.
ASSERT_TRUE(kill(getpid(), SIGRTMAX - 18) == 0);
- sleep(1);
-
- // This triggers the dumping.
- debug_free(pointer);
- expected += android::base::StringPrintf("%d: free %p\n", getpid(), pointer);
ASSERT_TRUE(android::base::ReadFileToString(record_filename, &actual));
ASSERT_STREQ(expected.c_str(), actual.c_str());
@@ -2389,6 +2360,41 @@
ASSERT_STREQ(expected_log.c_str(), getFakeLogPrint().c_str());
}
+TEST_F(MallocDebugTest, record_allocs_no_entries_to_write) {
+ InitRecordAllocs("record_allocs=5");
+
+ kill(getpid(), SIGRTMAX - 18);
+
+ std::string actual;
+ ASSERT_FALSE(android::base::ReadFileToString(record_filename, &actual));
+
+ ASSERT_STREQ("", getFakeLogBuf().c_str());
+ ASSERT_STREQ("4 malloc_debug No alloc entries to write.\n", getFakeLogPrint().c_str());
+}
+
+TEST_F(MallocDebugTest, record_allocs_write_entries_does_not_allocate) {
+ InitRecordAllocs("record_allocs=5");
+
+ std::string expected;
+
+ void* pointer = debug_malloc(10);
+ ASSERT_TRUE(pointer != nullptr);
+ expected += android::base::StringPrintf("%d: malloc %p 10\n", getpid(), pointer);
+ debug_free(pointer);
+ expected += android::base::StringPrintf("%d: free %p\n", getpid(), pointer);
+
+ malloc_disable();
+ kill(getpid(), SIGRTMAX - 18);
+ malloc_enable();
+
+ std::string actual;
+ ASSERT_TRUE(android::base::ReadFileToString(record_filename, &actual));
+ ASSERT_STREQ(expected.c_str(), actual.c_str());
+
+ ASSERT_STREQ("", getFakeLogBuf().c_str());
+ ASSERT_STREQ("", getFakeLogPrint().c_str());
+}
+
TEST_F(MallocDebugTest, verify_pointers) {
Init("verify_pointers");