Refactor code out of MinidumpModuleList::Read().
Add a StoreRange() helper method and an IsDevAshmem() helper function.
Change-Id: Iaec9dee1e08bd0155f1c33cfe9af722b0dcaef31
Reviewed-on: https://chromium-review.googlesource.com/1114188
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
diff --git a/src/google_breakpad/processor/minidump.h b/src/google_breakpad/processor/minidump.h
index 26b8b8d..785428b 100644
--- a/src/google_breakpad/processor/minidump.h
+++ b/src/google_breakpad/processor/minidump.h
@@ -541,6 +541,11 @@
bool Read(uint32_t expected_size);
+ bool StoreRange(const MinidumpModule& module,
+ uint64_t base_address,
+ uint32_t module_index,
+ uint32_t module_count);
+
// The largest number of modules that will be read from a minidump. The
// default is 1024.
static uint32_t max_modules_;
diff --git a/src/processor/minidump.cc b/src/processor/minidump.cc
index ea44c50..fa21f27 100644
--- a/src/processor/minidump.cc
+++ b/src/processor/minidump.cc
@@ -416,6 +416,11 @@
return std::string(buf);
}
+bool IsDevAshmem(const string& filename) {
+ const string kDevAshmem("/dev/ashmem/");
+ return filename.compare(0, kDevAshmem.length(), kDevAshmem) == 0;
+}
+
} // namespace
//
@@ -2674,8 +2679,7 @@
scoped_ptr<MinidumpModules> modules(
new MinidumpModules(module_count, MinidumpModule(minidump_)));
- for (unsigned int module_index = 0;
- module_index < module_count;
+ for (uint32_t module_index = 0; module_index < module_count;
++module_index) {
MinidumpModule* module = &(*modules)[module_index];
@@ -2693,17 +2697,16 @@
// included in the loop above, additional seeks would be needed where
// none are now to read contiguous data.
uint64_t last_end_address = 0;
- for (unsigned int module_index = 0;
- module_index < module_count;
+ for (uint32_t module_index = 0; module_index < module_count;
++module_index) {
- MinidumpModule* module = &(*modules)[module_index];
+ MinidumpModule& module = (*modules)[module_index];
// ReadAuxiliaryData fails if any data that the module indicates should
// exist is missing, but we treat some such cases as valid anyway. See
// issue #222: if a debugging record is of a format that's too large to
// handle, it shouldn't render the entire dump invalid. Check module
// validity before giving up.
- if (!module->ReadAuxiliaryData() && !module->valid()) {
+ if (!module.ReadAuxiliaryData() && !module.valid()) {
BPLOG(ERROR) << "MinidumpModuleList could not read required module "
"auxiliary data for module " <<
module_index << "/" << module_count;
@@ -2713,52 +2716,35 @@
// It is safe to use module->code_file() after successfully calling
// module->ReadAuxiliaryData or noting that the module is valid.
- uint64_t base_address = module->base_address();
- uint64_t module_size = module->size();
+ uint64_t base_address = module.base_address();
+ uint64_t module_size = module.size();
if (base_address == static_cast<uint64_t>(-1)) {
- BPLOG(ERROR) << "MinidumpModuleList found bad base address "
- "for module " << module_index << "/" << module_count <<
- ", " << module->code_file();
+ BPLOG(ERROR) << "MinidumpModuleList found bad base address for module "
+ << module_index << "/" << module_count << ", "
+ << module.code_file();
return false;
}
- if (!range_map_->StoreRange(base_address, module_size, module_index)) {
- // Android's shared memory implementation /dev/ashmem can contain
- // duplicate entries for JITted code, so ignore these.
- // TODO(wfh): Remove this code when Android is fixed.
- // See https://crbug.com/439531
- const string kDevAshmem("/dev/ashmem/");
- if (module->code_file().compare(
- 0, kDevAshmem.length(), kDevAshmem) != 0) {
- if (base_address < last_end_address) {
- // If failed due to apparent range overlap the cause may be
- // the client correction applied for Android packed relocations.
- // If this is the case, back out the client correction and retry.
- module_size -= last_end_address - base_address;
- base_address = last_end_address;
- if (!range_map_->StoreRange(base_address,
- module_size, module_index)) {
- BPLOG(ERROR) << "MinidumpModuleList could not store module " <<
- module_index << "/" << module_count << ", " <<
- module->code_file() << ", " <<
- HexString(base_address) << "+" <<
- HexString(module_size) << ", after adjusting";
- return false;
- }
- } else {
- BPLOG(ERROR) << "MinidumpModuleList could not store module " <<
- module_index << "/" << module_count << ", " <<
- module->code_file() << ", " <<
- HexString(base_address) << "+" <<
- HexString(module_size);
- return false;
- }
- } else {
- BPLOG(INFO) << "MinidumpModuleList ignoring overlapping module " <<
- module_index << "/" << module_count << ", " <<
- module->code_file() << ", " <<
- HexString(base_address) << "+" <<
- HexString(module_size);
+ if (!StoreRange(module, base_address, module_index, module_count)) {
+ if (base_address >= last_end_address) {
+ BPLOG(ERROR) << "MinidumpModuleList could not store module "
+ << module_index << "/" << module_count << ", "
+ << module.code_file() << ", " << HexString(base_address)
+ << "+" << HexString(module_size);
+ return false;
+ }
+
+ // If failed due to apparent range overlap the cause may be the client
+ // correction applied for Android packed relocations. If this is the
+ // case, back out the client correction and retry.
+ module_size -= last_end_address - base_address;
+ base_address = last_end_address;
+ if (!range_map_->StoreRange(base_address, module_size, module_index)) {
+ BPLOG(ERROR) << "MinidumpModuleList could not store module "
+ << module_index << "/" << module_count << ", "
+ << module.code_file() << ", " << HexString(base_address)
+ << "+" << HexString(module_size) << ", after adjusting";
+ return false;
}
}
last_end_address = base_address + module_size;
@@ -2773,6 +2759,27 @@
return true;
}
+bool MinidumpModuleList::StoreRange(const MinidumpModule& module,
+ uint64_t base_address,
+ uint32_t module_index,
+ uint32_t module_count) {
+ if (range_map_->StoreRange(base_address, module.size(), module_index))
+ return true;
+
+ // Android's shared memory implementation /dev/ashmem can contain duplicate
+ // entries for JITted code, so ignore these.
+ // TODO(wfh): Remove this code when Android is fixed.
+ // See https://crbug.com/439531
+ if (IsDevAshmem(module.code_file())) {
+ BPLOG(INFO) << "MinidumpModuleList ignoring overlapping module "
+ << module_index << "/" << module_count << ", "
+ << module.code_file() << ", " << HexString(base_address) << "+"
+ << HexString(module.size());
+ return true;
+ }
+
+ return false;
+}
const MinidumpModule* MinidumpModuleList::GetModuleForAddress(
uint64_t address) const {
