https://github.com/Jlalond updated
https://github.com/llvm/llvm-project/pull/129307
>From 2f77beefb752ffdae908101d75381268203311d6 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalo...@fb.com>
Date: Fri, 28 Feb 2025 11:38:35 -0800
Subject: [PATCH 1/6] Update MinidumpFileBuilder to read and write in chunks
---
.../Minidump/MinidumpFileBuilder.cpp | 130 ++++++++++++------
.../ObjectFile/Minidump/MinidumpFileBuilder.h | 7 +
2 files changed, 97 insertions(+), 40 deletions(-)
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index c5013ea5e3be4..e88b606f279cd 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -969,12 +969,82 @@ Status MinidumpFileBuilder::DumpDirectories() const {
return error;
}
-static uint64_t
-GetLargestRangeSize(const std::vector<CoreFileMemoryRange> &ranges) {
- uint64_t max_size = 0;
- for (const auto &core_range : ranges)
- max_size = std::max(max_size, core_range.range.size());
- return max_size;
+Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
+ const lldb_private::CoreFileMemoryRange &range, uint64_t *bytes_read) {
+ Log *log = GetLog(LLDBLog::Object);
+ lldb::addr_t addr = range.range.start();
+ lldb::addr_t size = range.range.size();
+ // First we set the byte tally to 0, so if we do exit gracefully
+ // the caller doesn't think the random garbage on the stack is a
+ // success.
+ if (bytes_read)
+ *bytes_read = 0;
+
+ uint64_t bytes_remaining = size;
+ uint64_t total_bytes_read = 0;
+ auto data_up = std::make_unique<DataBufferHeap>(
+ std::min(bytes_remaining, MAX_WRITE_CHUNK_SIZE), 0);
+ Status error;
+ while (bytes_remaining > 0) {
+ // Get the next read chunk size as the minimum of the remaining bytes and
+ // the write chunk max size.
+ const size_t bytes_to_read =
+ std::min(bytes_remaining, MAX_WRITE_CHUNK_SIZE);
+ const size_t bytes_read_for_chunk =
+ m_process_sp->ReadMemory(range.range.start() + total_bytes_read,
+ data_up->GetBytes(), bytes_to_read, error);
+ if (error.Fail() || bytes_read_for_chunk == 0) {
+ LLDB_LOGF(log,
+ "Failed to read memory region at: %" PRIx64
+ ". Bytes read: %zu, error: %s",
+ addr, bytes_read_for_chunk, error.AsCString());
+ // If we've only read one byte we can just give up and return
+ if (total_bytes_read == 0)
+ return Status();
+
+ // If we've read some bytes, we stop trying to read more and return
+ // this best effort attempt
+ bytes_remaining = 0;
+ } else if (bytes_read_for_chunk != bytes_to_read) {
+ LLDB_LOGF(
+ log, "Memory region at: %" PRIx64 " failed to read %" PRIx64 "
bytes",
+ addr, size);
+
+ // If we've read some bytes, we stop trying to read more and return
+ // this best effort attempt
+ bytes_remaining = 0;
+ }
+
+ // Write to the minidump file with the chunk potentially flushing to disk.
+ // this is the only place we want to return a true error, so that we can
+ // fail. If we get an error writing to disk we can't easily gaurauntee
+ // that we won't corrupt the minidump.
+ error = AddData(data_up->GetBytes(), bytes_read_for_chunk);
+ if (error.Fail())
+ return error;
+
+ // This check is so we don't overflow when the error code above sets the
+ // bytes to read to 0 (the graceful exit condition).
+ if (bytes_remaining > 0)
+ bytes_remaining -= bytes_read_for_chunk;
+
+ total_bytes_read += bytes_read_for_chunk;
+ // If the caller wants a tally back of the bytes_read, update it as we
+ // write. We do this in the loop so if we encounter an error we can
+ // report the accurate total.
+ if (bytes_read)
+ *bytes_read += bytes_read_for_chunk;
+
+ // We clear the heap per loop, without checking if we
+ // read the expected bytes this is so we don't allocate
+ // more than the MAX_WRITE_CHUNK_SIZE. But we do check if
+ // this is even worth clearing before we return and
+ // destruct the heap.
+ if (bytes_remaining > 0)
+ data_up->Clear();
+ }
+
+ return error;
}
Status
@@ -987,8 +1057,6 @@
MinidumpFileBuilder::AddMemoryList_32(std::vector<CoreFileMemoryRange> &ranges,
Log *log = GetLog(LLDBLog::Object);
size_t region_index = 0;
- auto data_up =
- std::make_unique<DataBufferHeap>(GetLargestRangeSize(ranges), 0);
for (const auto &core_range : ranges) {
// Take the offset before we write.
const offset_t offset_for_data = GetCurrentDataEndOffset();
@@ -1003,18 +1071,15 @@
MinidumpFileBuilder::AddMemoryList_32(std::vector<CoreFileMemoryRange> &ranges,
++region_index;
progress.Increment(1, "Adding Memory Range " + core_range.Dump());
- const size_t bytes_read =
- m_process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
- if (error.Fail() || bytes_read == 0) {
- LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error:
%s",
- bytes_read, error.AsCString());
- // Just skip sections with errors or zero bytes in 32b mode
+ uint64_t bytes_read;
+ error = ReadWriteMemoryInChunks(core_range, &bytes_read);
+ if (error.Fail())
+ return error;
+
+ // If we completely failed to read this range
+ // we can just omit any of the book keeping.
+ if (bytes_read == 0)
continue;
- } else if (bytes_read != size) {
- LLDB_LOGF(
- log, "Memory region at: %" PRIx64 " failed to read %" PRIx64 "
bytes",
- addr, size);
- }
MemoryDescriptor descriptor;
descriptor.StartOfMemoryRange =
@@ -1026,11 +1091,6 @@
MinidumpFileBuilder::AddMemoryList_32(std::vector<CoreFileMemoryRange> &ranges,
descriptors.push_back(descriptor);
if (m_thread_by_range_end.count(end) > 0)
m_thread_by_range_end[end].Stack = descriptor;
-
- // Add the data to the buffer, flush as needed.
- error = AddData(data_up->GetBytes(), bytes_read);
- if (error.Fail())
- return error;
}
// Add a directory that references this list
@@ -1106,8 +1166,6 @@
MinidumpFileBuilder::AddMemoryList_64(std::vector<CoreFileMemoryRange> &ranges,
Log *log = GetLog(LLDBLog::Object);
size_t region_index = 0;
- auto data_up =
- std::make_unique<DataBufferHeap>(GetLargestRangeSize(ranges), 0);
for (const auto &core_range : ranges) {
const addr_t addr = core_range.range.start();
const addr_t size = core_range.range.size();
@@ -1120,27 +1178,19 @@
MinidumpFileBuilder::AddMemoryList_64(std::vector<CoreFileMemoryRange> &ranges,
++region_index;
progress.Increment(1, "Adding Memory Range " + core_range.Dump());
- const size_t bytes_read =
- m_process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
- if (error.Fail()) {
- LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error:
%s",
- bytes_read, error.AsCString());
- error.Clear();
+ uint64_t bytes_read;
+ error = ReadWriteMemoryInChunks(core_range, &bytes_read);
+ if (error.Fail())
+ return error;
+
+ if (bytes_read == 0) {
cleanup_required = true;
descriptors[region_index].DataSize = 0;
}
if (bytes_read != size) {
- LLDB_LOGF(
- log, "Memory region at: %" PRIx64 " failed to read %" PRIx64 "
bytes",
- addr, size);
cleanup_required = true;
descriptors[region_index].DataSize = bytes_read;
}
-
- // Add the data to the buffer, flush as needed.
- error = AddData(data_up->GetBytes(), bytes_read);
- if (error.Fail())
- return error;
}
// Early return if there is no cleanup needed.
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index 48293ee1bf5e5..b9ebb0d9a28b7 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -142,6 +142,13 @@ class MinidumpFileBuilder {
lldb_private::Status AddDirectory(llvm::minidump::StreamType type,
uint64_t stream_size);
lldb::offset_t GetCurrentDataEndOffset() const;
+
+ // Read a memory region from the process and write it to the file
+ // in fixed size chunks.
+ lldb_private::Status
+ ReadWriteMemoryInChunks(const lldb_private::CoreFileMemoryRange &range,
+ uint64_t *bytes_read);
+
// Stores directories to fill in later
std::vector<llvm::minidump::Directory> m_directories;
// When we write off the threads for the first time, we need to clean them up
>From 58501dbdbbbad3438a9b4e42ea00b56d552b1806 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalo...@fb.com>
Date: Fri, 28 Feb 2025 13:43:16 -0800
Subject: [PATCH 2/6] Comment cleanup on the 0 bytes read error case
---
.../source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index e88b606f279cd..969155f52c4b2 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -998,7 +998,8 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
"Failed to read memory region at: %" PRIx64
". Bytes read: %zu, error: %s",
addr, bytes_read_for_chunk, error.AsCString());
- // If we've only read one byte we can just give up and return
+ // If we've read nothing, and get an error or fail to read
+ // we can just give up early.
if (total_bytes_read == 0)
return Status();
>From 4d55b066ded5474d8648e0972b5668c5e474bb80 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalo...@fb.com>
Date: Mon, 3 Mar 2025 09:37:52 -0800
Subject: [PATCH 3/6] Add one more overflow safety check, const some stuff
---
.../ObjectFile/Minidump/MinidumpFileBuilder.cpp | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index 969155f52c4b2..35a6ad6eb4bf4 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -972,8 +972,8 @@ Status MinidumpFileBuilder::DumpDirectories() const {
Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
const lldb_private::CoreFileMemoryRange &range, uint64_t *bytes_read) {
Log *log = GetLog(LLDBLog::Object);
- lldb::addr_t addr = range.range.start();
- lldb::addr_t size = range.range.size();
+ const lldb::addr_t addr = range.range.start();
+ const lldb::addr_t size = range.range.size();
// First we set the byte tally to 0, so if we do exit gracefully
// the caller doesn't think the random garbage on the stack is a
// success.
@@ -1024,7 +1024,13 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
if (error.Fail())
return error;
- // This check is so we don't overflow when the error code above sets the
+ // If the bytes read in this chunk would cause us to overflow, set the
+ // remaining bytes to 0 so we exit. This is a safety check so we don't
+ // get stuck building a bigger file forever.
+ if (bytes_read_for_chunk > bytes_remaining)
+ bytes_remaining = 0;
+
+ // This check is so we don't overflow when the error above sets the
// bytes to read to 0 (the graceful exit condition).
if (bytes_remaining > 0)
bytes_remaining -= bytes_read_for_chunk;
>From f73943baf1ea30980dccac926b10b30f694d9b55 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalo...@fb.com>
Date: Tue, 4 Mar 2025 21:48:13 -0800
Subject: [PATCH 4/6] Discussed with Jeffrey offline, fix clear causing buffer
overflow. Move data_up memory allocation back to the add memory functions and
more intelligently allocate the buffer
---
.../Minidump/MinidumpFileBuilder.cpp | 29 +++++++++++--------
.../ObjectFile/Minidump/MinidumpFileBuilder.h | 6 ++--
2 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index 35a6ad6eb4bf4..d6a0a7a6369b5 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -970,7 +970,10 @@ Status MinidumpFileBuilder::DumpDirectories() const {
}
Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
+ const std::unique_ptr<lldb_private::DataBufferHeap> &data_up,
const lldb_private::CoreFileMemoryRange &range, uint64_t *bytes_read) {
+ if (!data_up)
+ return Status::FromErrorString("No buffer supplied to read memory.");
Log *log = GetLog(LLDBLog::Object);
const lldb::addr_t addr = range.range.start();
const lldb::addr_t size = range.range.size();
@@ -982,8 +985,6 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
uint64_t bytes_remaining = size;
uint64_t total_bytes_read = 0;
- auto data_up = std::make_unique<DataBufferHeap>(
- std::min(bytes_remaining, MAX_WRITE_CHUNK_SIZE), 0);
Status error;
while (bytes_remaining > 0) {
// Get the next read chunk size as the minimum of the remaining bytes and
@@ -1041,19 +1042,19 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
// report the accurate total.
if (bytes_read)
*bytes_read += bytes_read_for_chunk;
-
- // We clear the heap per loop, without checking if we
- // read the expected bytes this is so we don't allocate
- // more than the MAX_WRITE_CHUNK_SIZE. But we do check if
- // this is even worth clearing before we return and
- // destruct the heap.
- if (bytes_remaining > 0)
- data_up->Clear();
}
return error;
}
+static uint64_t
+GetLargestRangeSize(const std::vector<CoreFileMemoryRange> &ranges) {
+ uint64_t max_size = 0;
+ for (const auto &core_range : ranges)
+ max_size = std::max(max_size, core_range.range.size());
+ return max_size;
+}
+
Status
MinidumpFileBuilder::AddMemoryList_32(std::vector<CoreFileMemoryRange> &ranges,
Progress &progress) {
@@ -1064,6 +1065,8 @@
MinidumpFileBuilder::AddMemoryList_32(std::vector<CoreFileMemoryRange> &ranges,
Log *log = GetLog(LLDBLog::Object);
size_t region_index = 0;
+ auto data_up = std::make_unique<DataBufferHeap>(
+ std::min(GetLargestRangeSize(ranges), MAX_WRITE_CHUNK_SIZE), 0);
for (const auto &core_range : ranges) {
// Take the offset before we write.
const offset_t offset_for_data = GetCurrentDataEndOffset();
@@ -1079,7 +1082,7 @@
MinidumpFileBuilder::AddMemoryList_32(std::vector<CoreFileMemoryRange> &ranges,
progress.Increment(1, "Adding Memory Range " + core_range.Dump());
uint64_t bytes_read;
- error = ReadWriteMemoryInChunks(core_range, &bytes_read);
+ error = ReadWriteMemoryInChunks(data_up, core_range, &bytes_read);
if (error.Fail())
return error;
@@ -1155,6 +1158,8 @@
MinidumpFileBuilder::AddMemoryList_64(std::vector<CoreFileMemoryRange> &ranges,
list_header.BaseRVA = memory_ranges_base_rva;
m_data.AppendData(&list_header, sizeof(llvm::minidump::Memory64ListHeader));
+ auto data_up = std::make_unique<DataBufferHeap>(
+ std::min(GetLargestRangeSize(ranges), MAX_WRITE_CHUNK_SIZE), 0);
bool cleanup_required = false;
std::vector<MemoryDescriptor_64> descriptors;
// Enumerate the ranges and create the memory descriptors so we can append
@@ -1186,7 +1191,7 @@
MinidumpFileBuilder::AddMemoryList_64(std::vector<CoreFileMemoryRange> &ranges,
progress.Increment(1, "Adding Memory Range " + core_range.Dump());
uint64_t bytes_read;
- error = ReadWriteMemoryInChunks(core_range, &bytes_read);
+ error = ReadWriteMemoryInChunks(data_up, core_range, &bytes_read);
if (error.Fail())
return error;
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index b9ebb0d9a28b7..7840d68797245 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -145,9 +145,9 @@ class MinidumpFileBuilder {
// Read a memory region from the process and write it to the file
// in fixed size chunks.
- lldb_private::Status
- ReadWriteMemoryInChunks(const lldb_private::CoreFileMemoryRange &range,
- uint64_t *bytes_read);
+ lldb_private::Status ReadWriteMemoryInChunks(
+ const std::unique_ptr<lldb_private::DataBufferHeap> &data_up,
+ const lldb_private::CoreFileMemoryRange &range, uint64_t *bytes_read);
// Stores directories to fill in later
std::vector<llvm::minidump::Directory> m_directories;
>From dc67f7d7361d4c9ee164b90086f31800b75f9324 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalo...@fb.com>
Date: Mon, 10 Mar 2025 15:18:14 -0700
Subject: [PATCH 5/6] Finish refactor of data_up being the max buffer size, and
enforce bytes_read* is not null
---
.../ObjectFile/Minidump/MinidumpFileBuilder.cpp | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index d6a0a7a6369b5..6249d4c0bb917 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -974,25 +974,26 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
const lldb_private::CoreFileMemoryRange &range, uint64_t *bytes_read) {
if (!data_up)
return Status::FromErrorString("No buffer supplied to read memory.");
+
+ if (!bytes_read)
+ return Status::FromErrorString("Bytes read pointer cannot be null.");
Log *log = GetLog(LLDBLog::Object);
const lldb::addr_t addr = range.range.start();
const lldb::addr_t size = range.range.size();
// First we set the byte tally to 0, so if we do exit gracefully
// the caller doesn't think the random garbage on the stack is a
// success.
- if (bytes_read)
- *bytes_read = 0;
+ *bytes_read = 0;
uint64_t bytes_remaining = size;
- uint64_t total_bytes_read = 0;
Status error;
while (bytes_remaining > 0) {
// Get the next read chunk size as the minimum of the remaining bytes and
// the write chunk max size.
const size_t bytes_to_read =
- std::min(bytes_remaining, MAX_WRITE_CHUNK_SIZE);
+ std::min(bytes_remaining, data_up->GetByteSize());
const size_t bytes_read_for_chunk =
- m_process_sp->ReadMemory(range.range.start() + total_bytes_read,
+ m_process_sp->ReadMemory(range.range.start() + *bytes_read,
data_up->GetBytes(), bytes_to_read, error);
if (error.Fail() || bytes_read_for_chunk == 0) {
LLDB_LOGF(log,
@@ -1001,7 +1002,7 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
addr, bytes_read_for_chunk, error.AsCString());
// If we've read nothing, and get an error or fail to read
// we can just give up early.
- if (total_bytes_read == 0)
+ if (*bytes_read == 0)
return Status();
// If we've read some bytes, we stop trying to read more and return
@@ -1036,12 +1037,10 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
if (bytes_remaining > 0)
bytes_remaining -= bytes_read_for_chunk;
- total_bytes_read += bytes_read_for_chunk;
// If the caller wants a tally back of the bytes_read, update it as we
// write. We do this in the loop so if we encounter an error we can
// report the accurate total.
- if (bytes_read)
- *bytes_read += bytes_read_for_chunk;
+ *bytes_read += bytes_read_for_chunk;
}
return error;
>From a3ef677704d5fe1730e563152eeecbfc02fa8536 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalo...@fb.com>
Date: Fri, 14 Mar 2025 11:33:03 -0700
Subject: [PATCH 6/6] Convert the soft exit cases into hard exit conditions and
add a flag for the partial read exit case.
Additionally add an expanded comment of how we handle errors encountered during
reading, and why we don't set the *bytes_read to 0.
---
.../Minidump/MinidumpFileBuilder.cpp | 46 +++++++++----------
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index 6249d4c0bb917..41ccee338ae92 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -986,8 +986,13 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
*bytes_read = 0;
uint64_t bytes_remaining = size;
+ // This is our flag to control if we had a partial read
+ // if we read less than our expected number of bytes without
+ // getting an error, we should add those bytes and discountine
+ // trying to read.
+ bool partialReadEncountered = false;
Status error;
- while (bytes_remaining > 0) {
+ while (bytes_remaining > 0 && !partialReadEncountered) {
// Get the next read chunk size as the minimum of the remaining bytes and
// the write chunk max size.
const size_t bytes_to_read =
@@ -1000,22 +1005,22 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
"Failed to read memory region at: %" PRIx64
". Bytes read: %zu, error: %s",
addr, bytes_read_for_chunk, error.AsCString());
- // If we've read nothing, and get an error or fail to read
- // we can just give up early.
- if (*bytes_read == 0)
- return Status();
- // If we've read some bytes, we stop trying to read more and return
- // this best effort attempt
- bytes_remaining = 0;
+ // If we failed to read memory and got an error, we return and skip
+ // the rest of the region. We need to return a non-error status here
+ // because the caller can't differentiate between this skippable
+ // error, and an error appending data to the file, which is fatal.
+ return Status();
} else if (bytes_read_for_chunk != bytes_to_read) {
- LLDB_LOGF(
- log, "Memory region at: %" PRIx64 " failed to read %" PRIx64 "
bytes",
- addr, size);
+ LLDB_LOGF(log,
+ "Memory region at: %" PRIx64 " partiall read %" PRIx64
+ " bytes out of %" PRIx64 " bytes.",
+ addr, bytes_read_for_chunk,
+ bytes_to_read - bytes_read_for_chunk);
// If we've read some bytes, we stop trying to read more and return
// this best effort attempt
- bytes_remaining = 0;
+ partialReadEncountered = true;
}
// Write to the minidump file with the chunk potentially flushing to disk.
@@ -1026,20 +1031,15 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
if (error.Fail())
return error;
- // If the bytes read in this chunk would cause us to overflow, set the
- // remaining bytes to 0 so we exit. This is a safety check so we don't
- // get stuck building a bigger file forever.
+ // If the bytes read in this chunk would cause us to overflow, something
+ // went wrong and we should fail out of creating the Minidump.
if (bytes_read_for_chunk > bytes_remaining)
- bytes_remaining = 0;
-
- // This check is so we don't overflow when the error above sets the
- // bytes to read to 0 (the graceful exit condition).
- if (bytes_remaining > 0)
+ return Status::FromErrorString("Unexpected number of bytes read.");
+ else
bytes_remaining -= bytes_read_for_chunk;
- // If the caller wants a tally back of the bytes_read, update it as we
- // write. We do this in the loop so if we encounter an error we can
- // report the accurate total.
+ // Update the caller with the number of bytes read, but also written to the
+ // underlying buffer.
*bytes_read += bytes_read_for_chunk;
}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
