diff --git a/Makefile.am b/Makefile.am index e6fd0a4c..c78c1293 100644 --- a/Makefile.am +++ b/Makefile.am @@ -128,14 +128,22 @@ TESTS_ENVIRONMENT = src_processor_address_map_unittest_SOURCES = \ src/processor/address_map_unittest.cc +src_processor_address_map_unittest_LDADD = \ + src/processor/logging.lo \ + src/processor/pathname_stripper.lo src_processor_basic_source_line_resolver_unittest_SOURCES = \ src/processor/basic_source_line_resolver_unittest.cc src_processor_basic_source_line_resolver_unittest_LDADD = \ - src/processor/basic_source_line_resolver.lo + src/processor/basic_source_line_resolver.lo \ + src/processor/pathname_stripper.lo \ + src/processor/logging.lo src_processor_contained_range_map_unittest_SOURCES = \ src/processor/contained_range_map_unittest.cc +src_processor_contained_range_map_unittest_LDADD = \ + src/processor/logging.lo \ + src/processor/pathname_stripper.lo src_processor_minidump_processor_unittest_SOURCES = \ src/processor/minidump_processor_unittest.cc @@ -159,9 +167,15 @@ src_processor_pathname_stripper_unittest_LDADD = \ src_processor_postfix_evaluator_unittest_SOURCES = \ src/processor/postfix_evaluator_unittest.cc +src_processor_postfix_evaluator_unittest_LDADD = \ + src/processor/logging.lo \ + src/processor/pathname_stripper.lo src_processor_range_map_unittest_SOURCES = \ src/processor/range_map_unittest.cc +src_processor_range_map_unittest_LDADD = \ + src/processor/logging.lo \ + src/processor/pathname_stripper.lo src_processor_stackwalker_selftest_SOURCES = \ src/processor/stackwalker_selftest.cc diff --git a/Makefile.in b/Makefile.in index d2bc94a7..c6283f55 100644 --- a/Makefile.in +++ b/Makefile.in @@ -123,17 +123,20 @@ am_src_processor_address_map_unittest_OBJECTS = \ src/processor/address_map_unittest.$(OBJEXT) src_processor_address_map_unittest_OBJECTS = \ $(am_src_processor_address_map_unittest_OBJECTS) -src_processor_address_map_unittest_LDADD = $(LDADD) +src_processor_address_map_unittest_DEPENDENCIES = \ + src/processor/logging.lo src/processor/pathname_stripper.lo am_src_processor_basic_source_line_resolver_unittest_OBJECTS = \ src/processor/basic_source_line_resolver_unittest.$(OBJEXT) src_processor_basic_source_line_resolver_unittest_OBJECTS = $(am_src_processor_basic_source_line_resolver_unittest_OBJECTS) src_processor_basic_source_line_resolver_unittest_DEPENDENCIES = \ - src/processor/basic_source_line_resolver.lo + src/processor/basic_source_line_resolver.lo \ + src/processor/pathname_stripper.lo src/processor/logging.lo am_src_processor_contained_range_map_unittest_OBJECTS = \ src/processor/contained_range_map_unittest.$(OBJEXT) src_processor_contained_range_map_unittest_OBJECTS = \ $(am_src_processor_contained_range_map_unittest_OBJECTS) -src_processor_contained_range_map_unittest_LDADD = $(LDADD) +src_processor_contained_range_map_unittest_DEPENDENCIES = \ + src/processor/logging.lo src/processor/pathname_stripper.lo am_src_processor_minidump_dump_OBJECTS = \ src/processor/minidump_dump.$(OBJEXT) src_processor_minidump_dump_OBJECTS = \ @@ -178,12 +181,14 @@ am_src_processor_postfix_evaluator_unittest_OBJECTS = \ src/processor/postfix_evaluator_unittest.$(OBJEXT) src_processor_postfix_evaluator_unittest_OBJECTS = \ $(am_src_processor_postfix_evaluator_unittest_OBJECTS) -src_processor_postfix_evaluator_unittest_LDADD = $(LDADD) +src_processor_postfix_evaluator_unittest_DEPENDENCIES = \ + src/processor/logging.lo src/processor/pathname_stripper.lo am_src_processor_range_map_unittest_OBJECTS = \ src/processor/range_map_unittest.$(OBJEXT) src_processor_range_map_unittest_OBJECTS = \ $(am_src_processor_range_map_unittest_OBJECTS) -src_processor_range_map_unittest_LDADD = $(LDADD) +src_processor_range_map_unittest_DEPENDENCIES = \ + src/processor/logging.lo src/processor/pathname_stripper.lo am_src_processor_stackwalker_selftest_OBJECTS = \ src/processor/stackwalker_selftest.$(OBJEXT) src_processor_stackwalker_selftest_OBJECTS = \ @@ -429,15 +434,25 @@ TESTS_ENVIRONMENT = src_processor_address_map_unittest_SOURCES = \ src/processor/address_map_unittest.cc +src_processor_address_map_unittest_LDADD = \ + src/processor/logging.lo \ + src/processor/pathname_stripper.lo + src_processor_basic_source_line_resolver_unittest_SOURCES = \ src/processor/basic_source_line_resolver_unittest.cc src_processor_basic_source_line_resolver_unittest_LDADD = \ - src/processor/basic_source_line_resolver.lo + src/processor/basic_source_line_resolver.lo \ + src/processor/pathname_stripper.lo \ + src/processor/logging.lo src_processor_contained_range_map_unittest_SOURCES = \ src/processor/contained_range_map_unittest.cc +src_processor_contained_range_map_unittest_LDADD = \ + src/processor/logging.lo \ + src/processor/pathname_stripper.lo + src_processor_minidump_processor_unittest_SOURCES = \ src/processor/minidump_processor_unittest.cc @@ -463,9 +478,17 @@ src_processor_pathname_stripper_unittest_LDADD = \ src_processor_postfix_evaluator_unittest_SOURCES = \ src/processor/postfix_evaluator_unittest.cc +src_processor_postfix_evaluator_unittest_LDADD = \ + src/processor/logging.lo \ + src/processor/pathname_stripper.lo + src_processor_range_map_unittest_SOURCES = \ src/processor/range_map_unittest.cc +src_processor_range_map_unittest_LDADD = \ + src/processor/logging.lo \ + src/processor/pathname_stripper.lo + src_processor_stackwalker_selftest_SOURCES = \ src/processor/stackwalker_selftest.cc diff --git a/src/processor/address_map-inl.h b/src/processor/address_map-inl.h index e09dcae5..d88b4fcc 100644 --- a/src/processor/address_map-inl.h +++ b/src/processor/address_map-inl.h @@ -36,7 +36,10 @@ #ifndef PROCESSOR_ADDRESS_MAP_INL_H__ #define PROCESSOR_ADDRESS_MAP_INL_H__ +#include + #include "processor/address_map.h" +#include "processor/logging.h" namespace google_breakpad { @@ -45,8 +48,11 @@ bool AddressMap::Store(const AddressType &address, const EntryType &entry) { // Ensure that the specified address doesn't conflict with something already // in the map. - if (map_.find(address) != map_.end()) + if (map_.find(address) != map_.end()) { + BPLOG(INFO) << "Store failed, address " << HexString(address) << + " is already present"; return false; + } map_.insert(MapValue(address, entry)); return true; @@ -56,8 +62,8 @@ template bool AddressMap::Retrieve( const AddressType &address, EntryType *entry, AddressType *entry_address) const { - if (!entry) - return false; + BPLOG_IF(ERROR, !entry) << "AddressMap::Retrieve requires |entry|"; + assert(entry); // upper_bound gives the first element whose key is greater than address, // but we want the first element whose key is less than or equal to address. diff --git a/src/processor/address_map.h b/src/processor/address_map.h index 44632323..14139e7a 100644 --- a/src/processor/address_map.h +++ b/src/processor/address_map.h @@ -53,8 +53,8 @@ class AddressMap { bool Store(const AddressType &address, const EntryType &entry); // Locates the entry stored at the highest address less than or equal to - // the address argument. If there is no such range, or if there is a - // parameter error, returns false. The entry is returned in entry. If + // the address argument. If there is no such range, returns false. The + // entry is returned in entry, which is a required argument. If // entry_address is not NULL, it will be set to the address that the entry // was stored at. bool Retrieve(const AddressType &address, diff --git a/src/processor/address_map_unittest.cc b/src/processor/address_map_unittest.cc index cd6b5a79..3c865895 100644 --- a/src/processor/address_map_unittest.cc +++ b/src/processor/address_map_unittest.cc @@ -107,7 +107,6 @@ static bool DoAddressMapTest() { ASSERT_EQ(entry->id(), 1); ASSERT_EQ(address, 10); ASSERT_TRUE(test_map.Retrieve(11, &entry, &address)); - ASSERT_FALSE(test_map.Retrieve(11, NULL, &address)); // parameter error ASSERT_TRUE(test_map.Retrieve(11, &entry, NULL)); // NULL ok here // Add some more elements. diff --git a/src/processor/basic_code_modules.cc b/src/processor/basic_code_modules.cc index 1e3ba99a..a21491c4 100644 --- a/src/processor/basic_code_modules.cc +++ b/src/processor/basic_code_modules.cc @@ -39,6 +39,7 @@ #include "processor/basic_code_modules.h" #include "google_breakpad/processor/code_module.h" #include "processor/linked_ptr.h" +#include "processor/logging.h" #include "processor/range_map-inl.h" namespace google_breakpad { @@ -46,6 +47,8 @@ namespace google_breakpad { BasicCodeModules::BasicCodeModules(const CodeModules *that) : main_address_(0), map_(new RangeMap >()) { + BPLOG_IF(ERROR, !that) << "BasicCodeModules::BasicCodeModules requires " + "|that|"; assert(that); const CodeModule *main_module = that->GetMainModule(); @@ -61,8 +64,11 @@ BasicCodeModules::BasicCodeModules(const CodeModules *that) // entire list, and GetModuleAtIndex may be faster than // GetModuleAtSequence. const CodeModule *module = that->GetModuleAtIndex(module_sequence)->Copy(); - map_->StoreRange(module->base_address(), module->size(), - linked_ptr(module)); + if (!map_->StoreRange(module->base_address(), module->size(), + linked_ptr(module))) { + BPLOG(ERROR) << "Module " << module->code_file() << + " could not be stored"; + } } } @@ -77,8 +83,10 @@ unsigned int BasicCodeModules::module_count() const { const CodeModule* BasicCodeModules::GetModuleForAddress( u_int64_t address) const { linked_ptr module; - if (!map_->RetrieveRange(address, &module, NULL, NULL)) + if (!map_->RetrieveRange(address, &module, NULL, NULL)) { + BPLOG(INFO) << "No module at " << HexString(address); return NULL; + } return module.get(); } @@ -90,8 +98,10 @@ const CodeModule* BasicCodeModules::GetMainModule() const { const CodeModule* BasicCodeModules::GetModuleAtSequence( unsigned int sequence) const { linked_ptr module; - if (!map_->RetrieveRangeAtIndex(sequence, &module, NULL, NULL)) + if (!map_->RetrieveRangeAtIndex(sequence, &module, NULL, NULL)) { + BPLOG(ERROR) << "RetrieveRangeAtIndex failed for sequence " << sequence; return NULL; + } return module.get(); } diff --git a/src/processor/basic_source_line_resolver.cc b/src/processor/basic_source_line_resolver.cc index a66a4700..e5d1bd7f 100644 --- a/src/processor/basic_source_line_resolver.cc +++ b/src/processor/basic_source_line_resolver.cc @@ -145,7 +145,7 @@ class BasicSourceLineResolver::Module { static bool Tokenize(char *line, int max_tokens, vector *tokens); // Parses a file declaration - void ParseFile(char *file_line); + bool ParseFile(char *file_line); // Parses a function declaration, returning a new Function object. Function* ParseFunction(char *function_line); @@ -188,9 +188,13 @@ bool BasicSourceLineResolver::LoadModule(const string &module_name, const string &map_file) { // Make sure we don't already have a module with the given name. if (modules_->find(module_name) != modules_->end()) { + BPLOG(INFO) << "Symbols for module " << module_name << " already loaded"; return false; } + BPLOG(INFO) << "Loading symbols for module " << module_name << " from " << + map_file; + Module *module = new Module(module_name); if (!module->LoadMap(map_file)) { delete module; @@ -216,28 +220,56 @@ StackFrameInfo* BasicSourceLineResolver::FillSourceLineInfo( return NULL; } +class AutoFileCloser { + public: + AutoFileCloser(FILE *file) : file_(file) {} + ~AutoFileCloser() { + if (file_) + fclose(file_); + } + + private: + FILE *file_; +}; + bool BasicSourceLineResolver::Module::LoadMap(const string &map_file) { FILE *f = fopen(map_file.c_str(), "r"); if (!f) { + string error_string; + int error_code = ErrnoString(&error_string); + BPLOG(ERROR) << "Could not open " << map_file << + ", error " << error_code << ": " << error_string; return false; } + AutoFileCloser closer(f); + // TODO(mmentovai): this might not be large enough to handle really long // lines, which might be present for FUNC lines of highly-templatized // code. char buffer[8192]; linked_ptr cur_func; + int line_number = 0; while (fgets(buffer, sizeof(buffer), f)) { + ++line_number; if (strncmp(buffer, "FILE ", 5) == 0) { - ParseFile(buffer); + if (!ParseFile(buffer)) { + BPLOG(ERROR) << "ParseFile failed at " << + map_file << ":" << line_number; + return false; + } } else if (strncmp(buffer, "STACK ", 6) == 0) { if (!ParseStackInfo(buffer)) { + BPLOG(ERROR) << "ParseStackInfo failed at " << + map_file << ":" << line_number; return false; } } else if (strncmp(buffer, "FUNC ", 5) == 0) { cur_func.reset(ParseFunction(buffer)); if (!cur_func.get()) { + BPLOG(ERROR) << "ParseFunction failed at " << + map_file << ":" << line_number; return false; } // StoreRange will fail if the function has an invalid address or size. @@ -249,6 +281,8 @@ bool BasicSourceLineResolver::Module::LoadMap(const string &map_file) { cur_func.reset(); if (!ParsePublicSymbol(buffer)) { + BPLOG(ERROR) << "ParsePublicSymbol failed at " << + map_file << ":" << line_number; return false; } } else if (strncmp(buffer, "MODULE ", 7) == 0) { @@ -260,10 +294,14 @@ bool BasicSourceLineResolver::Module::LoadMap(const string &map_file) { // MODULE } else { if (!cur_func.get()) { + BPLOG(ERROR) << "Found source line data without a function at " << + map_file << ":" << line_number; return false; } Line *line = ParseLine(buffer); if (!line) { + BPLOG(ERROR) << "ParseLine failed at " << + map_file << ":" << line_number; return false; } cur_func->lines.StoreRange(line->address, line->size, @@ -271,7 +309,6 @@ bool BasicSourceLineResolver::Module::LoadMap(const string &map_file) { } } - fclose(f); return true; } @@ -387,24 +424,27 @@ bool BasicSourceLineResolver::Module::Tokenize(char *line, int max_tokens, return tokens->size() == static_cast(max_tokens); } -void BasicSourceLineResolver::Module::ParseFile(char *file_line) { +bool BasicSourceLineResolver::Module::ParseFile(char *file_line) { // FILE file_line += 5; // skip prefix vector tokens; if (!Tokenize(file_line, 2, &tokens)) { - return; + return false; } int index = atoi(tokens[0]); if (index < 0) { - return; + return false; } char *filename = tokens[1]; - if (filename) { - files_.insert(make_pair(index, string(filename))); + if (!filename) { + return false; } + + files_.insert(make_pair(index, string(filename))); + return true; } BasicSourceLineResolver::Function* diff --git a/src/processor/contained_range_map-inl.h b/src/processor/contained_range_map-inl.h index 5f029712..97f9fdbe 100644 --- a/src/processor/contained_range_map-inl.h +++ b/src/processor/contained_range_map-inl.h @@ -37,7 +37,10 @@ #define PROCESSOR_CONTAINED_RANGE_MAP_INL_H__ +#include + #include "processor/contained_range_map.h" +#include "processor/logging.h" namespace google_breakpad { @@ -56,8 +59,11 @@ bool ContainedRangeMap::StoreRange( AddressType high = base + size - 1; // Check for undersize or overflow. - if (size <= 0 || high < base) + if (size <= 0 || high < base) { + BPLOG(INFO) << "StoreRange failed, " << HexString(base) << "+" << + HexString(size) << ", " << HexString(high); return false; + } if (!map_) map_ = new AddressToRangeMap(); @@ -74,8 +80,11 @@ bool ContainedRangeMap::StoreRange( // range's, it violates the containment rules, and an attempt to store // it must fail. iterator_base->first contains the key, which was the // containing child's high address. - if (iterator_base->second->base_ == base && iterator_base->first == high) + if (iterator_base->second->base_ == base && iterator_base->first == high) { + BPLOG(INFO) << "StoreRange failed, identical range is already " + "present: " << HexString(base) << "+" << HexString(size); return false; + } // Pass the new range on to the child to attempt to store. return iterator_base->second->StoreRange(base, size, entry); @@ -92,6 +101,12 @@ bool ContainedRangeMap::StoreRange( // fully. Partial containment isn't allowed. if ((iterator_base != iterator_end && base > iterator_base->second->base_) || (contains_high && high < iterator_high->first)) { + // TODO(mmentovai): Some symbol files will trip this check frequently + // on STACK lines. Too many messages will be produced. These are more + // suitable for a DEBUG channel than an INFO channel. + // BPLOG(INFO) << "StoreRange failed, new range partially contains " + // "existing range: " << HexString(base) << "+" << + // HexString(size); return false; } @@ -130,7 +145,12 @@ bool ContainedRangeMap::StoreRange( template bool ContainedRangeMap::RetrieveRange( const AddressType &address, EntryType *entry) const { - if (!entry || !map_) + BPLOG_IF(ERROR, !entry) << "ContainedRangeMap::RetrieveRange requires " + "|entry|"; + assert(entry); + + // If nothing was ever stored, then there's nothing to retrieve. + if (!map_) return false; // Get an iterator to the child range whose high address is equal to or diff --git a/src/processor/contained_range_map.h b/src/processor/contained_range_map.h index deb1e061..f30016f3 100644 --- a/src/processor/contained_range_map.h +++ b/src/processor/contained_range_map.h @@ -92,8 +92,7 @@ class ContainedRangeMap { // the specified address. This method will only return entries held by // child ranges, and not the entry contained by |this|. This is necessary // to support a sparsely-populated root range. If no descendant range - // encompasses the address, or if there is a parameter error, returns - // false. + // encompasses the address, returns false. bool RetrieveRange(const AddressType &address, EntryType *entry) const; // Removes all children. Note that Clear only removes descendants, diff --git a/src/processor/minidump.cc b/src/processor/minidump.cc index 63a36037..28cc22ed 100644 --- a/src/processor/minidump.cc +++ b/src/processor/minidump.cc @@ -2650,9 +2650,9 @@ bool MinidumpMiscInfo::Read(u_int32_t expected_size) { } } - if (misc_info_.size_of_info != expected_size) { + if (expected_size != misc_info_.size_of_info) { BPLOG(ERROR) << "MinidumpMiscInfo size mismatch, " << - misc_info_.size_of_info << " != " << expected_size; + expected_size << " != " << misc_info_.size_of_info; return false; } diff --git a/src/processor/postfix_evaluator-inl.h b/src/processor/postfix_evaluator-inl.h index 54796df1..0b5f9c9b 100644 --- a/src/processor/postfix_evaluator-inl.h +++ b/src/processor/postfix_evaluator-inl.h @@ -27,6 +27,7 @@ #include "processor/postfix_evaluator.h" #include "google_breakpad/processor/memory_region.h" +#include "processor/logging.h" namespace google_breakpad { @@ -83,8 +84,11 @@ bool PostfixEvaluator::Evaluate(const string &expression, if (operation != BINARY_OP_NONE) { // Get the operands. ValueType operand1, operand2; - if (!PopValues(&operand1, &operand2)) + if (!PopValues(&operand1, &operand2)) { + BPLOG(ERROR) << "Could not PopValues to get two values for binary " + "operation " << token << ": " << expression; return false; + } // Perform the operation. ValueType result; @@ -107,6 +111,7 @@ bool PostfixEvaluator::Evaluate(const string &expression, case BINARY_OP_NONE: // This will not happen, but compilers will want a default or // BINARY_OP_NONE case. + BPLOG(ERROR) << "Not reached!"; return false; break; } @@ -115,32 +120,51 @@ bool PostfixEvaluator::Evaluate(const string &expression, PushValue(result); } else if (token == "^") { // ^ for unary dereference. Can't dereference without memory. - if (!memory_) + if (!memory_) { + BPLOG(ERROR) << "Attempt to dereference without memory: " << + expression; return false; + } ValueType address; - if (!PopValue(&address)) + if (!PopValue(&address)) { + BPLOG(ERROR) << "Could not PopValue to get value to derefence: " << + expression; return false; + } ValueType value; - if (!memory_->GetMemoryAtAddress(address, &value)) + if (!memory_->GetMemoryAtAddress(address, &value)) { + BPLOG(ERROR) << "Could not dereference memory at address " << + HexString(address) << ": " << expression; return false; + } PushValue(value); } else if (token == "=") { // = for assignment. ValueType value; - if (!PopValue(&value)) + if (!PopValue(&value)) { + BPLOG(ERROR) << "Could not PopValue to get value to assign: " << + expression; return false; + } // Assignment is only meaningful when assigning into an identifier. // The identifier must name a variable, not a constant. Variables // begin with '$'. string identifier; - if (PopValueOrIdentifier(NULL, &identifier) != POP_RESULT_IDENTIFIER) + if (PopValueOrIdentifier(NULL, &identifier) != POP_RESULT_IDENTIFIER) { + BPLOG(ERROR) << "PopValueOrIdentifier returned a value, but an " + "identifier is needed to assign " << + HexString(value) << ": " << expression; return false; - if (identifier.empty() || identifier[0] != '$') + } + if (identifier.empty() || identifier[0] != '$') { + BPLOG(ERROR) << "Can't assign " << HexString(value) << " to " << + identifier << ": " << expression; return false; + } (*dictionary_)[identifier] = value; if (assigned) @@ -157,6 +181,7 @@ bool PostfixEvaluator::Evaluate(const string &expression, // If there's anything left on the stack, it indicates incomplete execution. // This is a failure case. If the stack is empty, evalution was complete // and successful. + BPLOG_IF(ERROR, !stack_.empty()) << "Incomplete execution: " << expression; return stack_.empty(); } @@ -210,6 +235,7 @@ bool PostfixEvaluator::PopValue(ValueType *value) { if (iterator == dictionary_->end()) { // The identifier wasn't found in the dictionary. Don't imply any // default value, just fail. + BPLOG(ERROR) << "Identifier " << token << " not in dictionary"; return false; } diff --git a/src/processor/range_map-inl.h b/src/processor/range_map-inl.h index 3b01f9e3..17169ea4 100644 --- a/src/processor/range_map-inl.h +++ b/src/processor/range_map-inl.h @@ -38,6 +38,7 @@ #include "processor/range_map.h" +#include "processor/logging.h" namespace google_breakpad { @@ -50,8 +51,15 @@ bool RangeMap::StoreRange(const AddressType &base, AddressType high = base + size - 1; // Check for undersize or overflow. - if (size <= 0 || high < base) + if (size <= 0 || high < base) { + // The processor will hit this case too frequently with common symbol + // files in the size == 0 case, which is more suited to a DEBUG channel. + // Filter those out since there's no DEBUG channel at the moment. + BPLOG_IF(INFO, size != 0) << "StoreRange failed, " << HexString(base) << + "+" << HexString(size) << ", " << + HexString(high); return false; + } // Ensure that this range does not overlap with another one already in the // map. @@ -62,14 +70,27 @@ bool RangeMap::StoreRange(const AddressType &base, // Some other range begins in the space used by this range. It may be // contained within the space used by this range, or it may extend lower. // Regardless, it is an error. + AddressType other_base = iterator_base->second.base(); + AddressType other_size = iterator_base->first - other_base + 1; + BPLOG(INFO) << "StoreRange failed, an existing range is contained by or " + "extends lower than the new range: new " << + HexString(base) << "+" << HexString(size) << ", existing " << + HexString(other_base) << "+" << HexString(other_size); return false; } if (iterator_high != map_.end()) { if (iterator_high->second.base() <= high) { // The range above this one overlaps with this one. It may fully - // contain this range, or it may begin within this range and extend + // contain this range, or it may begin within this range and extend // higher. Regardless, it's an error. + AddressType other_base = iterator_high->second.base(); + AddressType other_size = iterator_high->first - other_base + 1; + BPLOG(INFO) << "StoreRange failed, an existing range contains or " + "extends higher than the new range: new " << + HexString(base) << "+" << HexString(size) << + ", existing " << + HexString(other_base) << "+" << HexString(other_size); return false; } } @@ -85,8 +106,8 @@ template bool RangeMap::RetrieveRange( const AddressType &address, EntryType *entry, AddressType *entry_base, AddressType *entry_size) const { - if (!entry) - return false; + BPLOG_IF(ERROR, !entry) << "RangeMap::RetrieveRange requires |entry|"; + assert(entry); MapConstIterator iterator = map_.lower_bound(address); if (iterator == map_.end()) @@ -114,8 +135,8 @@ template bool RangeMap::RetrieveNearestRange( const AddressType &address, EntryType *entry, AddressType *entry_base, AddressType *entry_size) const { - if (!entry) - return false; + BPLOG_IF(ERROR, !entry) << "RangeMap::RetrieveNearestRange requires |entry|"; + assert(entry); // If address is within a range, RetrieveRange can handle it. if (RetrieveRange(address, entry, entry_base, entry_size)) @@ -145,8 +166,13 @@ template bool RangeMap::RetrieveRangeAtIndex( int index, EntryType *entry, AddressType *entry_base, AddressType *entry_size) const { - if (!entry || index >= GetCount()) + BPLOG_IF(ERROR, !entry) << "RangeMap::RetrieveRangeAtIndex requires |entry|"; + assert(entry); + + if (index >= GetCount()) { + BPLOG(ERROR) << "Index out of range: " << index << "/" << GetCount(); return false; + } // Walk through the map. Although it's ordered, it's not a vector, so it // can't be addressed directly by index. diff --git a/src/processor/range_map.h b/src/processor/range_map.h index 9366563d..a7b67412 100644 --- a/src/processor/range_map.h +++ b/src/processor/range_map.h @@ -60,9 +60,8 @@ class RangeMap { const EntryType &entry); // Locates the range encompassing the supplied address. If there is - // no such range, or if there is a parameter error, returns false. - // entry_base and entry_size, if non-NULL, are set to the base and size - // of the entry's range. + // no such range, returns false. entry_base and entry_size, if non-NULL, + // are set to the base and size of the entry's range. bool RetrieveRange(const AddressType &address, EntryType *entry, AddressType *entry_base, AddressType *entry_size) const; @@ -77,9 +76,9 @@ class RangeMap { // Treating all ranges as a list ordered by the address spaces that they // occupy, locates the range at the index specified by index. Returns - // false if index is larger than the number of ranges stored, or if another - // parameter error occurs. entry_base and entry_size, if non-NULL, are set - // to the base and size of the entry's range. + // false if index is larger than the number of ranges stored. entry_base + // and entry_size, if non-NULL, are set to the base and size of the entry's + // range. // // RetrieveRangeAtIndex is not optimized for speedy operation. bool RetrieveRangeAtIndex(int index, EntryType *entry, diff --git a/src/processor/simple_symbol_supplier.cc b/src/processor/simple_symbol_supplier.cc index 734a7f77..62800013 100644 --- a/src/processor/simple_symbol_supplier.cc +++ b/src/processor/simple_symbol_supplier.cc @@ -41,6 +41,7 @@ #include "processor/simple_symbol_supplier.h" #include "google_breakpad/processor/code_module.h" #include "google_breakpad/processor/system_info.h" +#include "processor/logging.h" #include "processor/pathname_stripper.h" namespace google_breakpad { @@ -53,7 +54,11 @@ static bool file_exists(const string &file_name) { SymbolSupplier::SymbolResult SimpleSymbolSupplier::GetSymbolFile( const CodeModule *module, const SystemInfo *system_info, string *symbol_file) { + BPLOG_IF(ERROR, !symbol_file) << "SimpleSymbolSupplier::GetSymbolFile " + "requires |symbol_file|"; assert(symbol_file); + symbol_file->clear(); + for (unsigned int path_index = 0; path_index < paths_.size(); ++path_index) { SymbolResult result; if ((result = GetSymbolFileAtPath(module, system_info, paths_[path_index], @@ -67,7 +72,11 @@ SymbolSupplier::SymbolResult SimpleSymbolSupplier::GetSymbolFile( SymbolSupplier::SymbolResult SimpleSymbolSupplier::GetSymbolFileAtPath( const CodeModule *module, const SystemInfo *system_info, const string &root_path, string *symbol_file) { + BPLOG_IF(ERROR, !symbol_file) << "SimpleSymbolSupplier::GetSymbolFileAtPath " + "requires |symbol_file|"; assert(symbol_file); + symbol_file->clear(); + if (!module) return NOT_FOUND; @@ -77,15 +86,24 @@ SymbolSupplier::SymbolResult SimpleSymbolSupplier::GetSymbolFileAtPath( // Append the debug (pdb) file name as a directory name. path.append("/"); string debug_file_name = PathnameStripper::File(module->debug_file()); - if (debug_file_name.empty()) + if (debug_file_name.empty()) { + BPLOG(ERROR) << "Can't construct symbol file path without debug_file " + "(code_file = " << + PathnameStripper::File(module->code_file()) << ")"; return NOT_FOUND; + } path.append(debug_file_name); // Append the identifier as a directory name. path.append("/"); string identifier = module->debug_identifier(); - if (identifier.empty()) + if (identifier.empty()) { + BPLOG(ERROR) << "Can't construct symbol file path without debug_identifier " + "(code_file = " << + PathnameStripper::File(module->code_file()) << + ", debug_file = " << debug_file_name << ")"; return NOT_FOUND; + } path.append(identifier); // Transform the debug file name into one ending in .sym. If the existing @@ -103,8 +121,10 @@ SymbolSupplier::SymbolResult SimpleSymbolSupplier::GetSymbolFileAtPath( } path.append(".sym"); - if (!file_exists(path)) + if (!file_exists(path)) { + BPLOG(INFO) << "No symbol file at " << path; return NOT_FOUND; + } *symbol_file = path; return FOUND; diff --git a/src/processor/stackwalker.cc b/src/processor/stackwalker.cc index 00eaa6af..46d8384b 100644 --- a/src/processor/stackwalker.cc +++ b/src/processor/stackwalker.cc @@ -45,6 +45,7 @@ #include "google_breakpad/processor/stack_frame.h" #include "google_breakpad/processor/symbol_supplier.h" #include "processor/linked_ptr.h" +#include "processor/logging.h" #include "processor/scoped_ptr.h" #include "processor/stack_frame_info.h" #include "processor/stackwalker_ppc.h" @@ -67,6 +68,7 @@ Stackwalker::Stackwalker(const SystemInfo *system_info, bool Stackwalker::Walk(CallStack *stack) { + BPLOG_IF(ERROR, !stack) << "Stackwalker::Walk requires |stack|"; assert(stack); stack->Clear(); @@ -139,8 +141,10 @@ Stackwalker* Stackwalker::StackwalkerForCPU( const CodeModules *modules, SymbolSupplier *supplier, SourceLineResolverInterface *resolver) { - if (!context) + if (!context) { + BPLOG(ERROR) << "Can't choose a stackwalker implementation without context"; return NULL; + } Stackwalker *cpu_stackwalker = NULL; @@ -161,6 +165,9 @@ Stackwalker* Stackwalker::StackwalkerForCPU( break; } + BPLOG_IF(ERROR, !cpu_stackwalker) << "Unknown CPU type " << HexString(cpu) << + ", can't choose a stackwalker " + "implementation"; return cpu_stackwalker; } diff --git a/src/processor/stackwalker_ppc.cc b/src/processor/stackwalker_ppc.cc index 044ae99b..02a49b21 100644 --- a/src/processor/stackwalker_ppc.cc +++ b/src/processor/stackwalker_ppc.cc @@ -38,6 +38,7 @@ #include "google_breakpad/processor/call_stack.h" #include "google_breakpad/processor/memory_region.h" #include "google_breakpad/processor/stack_frame_cpu.h" +#include "processor/logging.h" namespace google_breakpad { @@ -54,14 +55,19 @@ StackwalkerPPC::StackwalkerPPC(const SystemInfo *system_info, // This implementation only covers 32-bit ppc CPUs. The limits of the // supplied stack are invalid. Mark memory_ = NULL, which will cause // stackwalking to fail. + BPLOG(ERROR) << "Memory out of range for stackwalking: " << + HexString(memory_->GetBase()) << "+" << + HexString(memory_->GetSize()); memory_ = NULL; } } StackFrame* StackwalkerPPC::GetContextFrame() { - if (!context_ || !memory_) + if (!context_ || !memory_) { + BPLOG(ERROR) << "Can't get context frame without context or memory"; return NULL; + } StackFramePPC *frame = new StackFramePPC(); @@ -78,8 +84,10 @@ StackFrame* StackwalkerPPC::GetContextFrame() { StackFrame* StackwalkerPPC::GetCallerFrame( const CallStack *stack, const vector< linked_ptr > &stack_frame_info) { - if (!memory_ || !stack) + if (!memory_ || !stack) { + BPLOG(ERROR) << "Can't get caller frame without memory or stack"; return NULL; + } // The instruction pointers for previous frames are saved on the stack. // The typical ppc calling convention is for the called procedure to store diff --git a/src/processor/stackwalker_x86.cc b/src/processor/stackwalker_x86.cc index 59662cdf..e39df17f 100644 --- a/src/processor/stackwalker_x86.cc +++ b/src/processor/stackwalker_x86.cc @@ -42,6 +42,7 @@ #include "google_breakpad/processor/memory_region.h" #include "google_breakpad/processor/stack_frame_cpu.h" #include "processor/linked_ptr.h" +#include "processor/logging.h" #include "processor/stack_frame_info.h" namespace google_breakpad { @@ -58,14 +59,19 @@ StackwalkerX86::StackwalkerX86(const SystemInfo *system_info, if (memory_->GetBase() + memory_->GetSize() - 1 > 0xffffffff) { // The x86 is a 32-bit CPU, the limits of the supplied stack are invalid. // Mark memory_ = NULL, which will cause stackwalking to fail. + BPLOG(ERROR) << "Memory out of range for stackwalking: " << + HexString(memory_->GetBase()) << "+" << + HexString(memory_->GetSize()); memory_ = NULL; } } StackFrame* StackwalkerX86::GetContextFrame() { - if (!context_ || !memory_) + if (!context_ || !memory_) { + BPLOG(ERROR) << "Can't get context frame without context or memory"; return NULL; + } StackFrameX86 *frame = new StackFrameX86(); @@ -82,8 +88,10 @@ StackFrame* StackwalkerX86::GetContextFrame() { StackFrame* StackwalkerX86::GetCallerFrame( const CallStack *stack, const vector< linked_ptr > &stack_frame_info) { - if (!memory_ || !stack) + if (!memory_ || !stack) { + BPLOG(ERROR) << "Can't get caller frame without memory or stack"; return NULL; + } StackFrameX86 *last_frame = static_cast( stack->frames()->back());