Add logging to minidump processor (#82). Part 2: add messages to the rest of

the processor.  r=ted.mielczarek

http://groups.google.com/group/google-breakpad-dev/browse_thread/thread/cf56b767383a5d4b


git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@172 4c0a9323-5329-0410-9bdc-e9ce6186880e

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

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
 

src/processor/address_map-inl.h

@@ -36,7 +36,10 @@
 #ifndef PROCESSOR_ADDRESS_MAP_INL_H__
 #define PROCESSOR_ADDRESS_MAP_INL_H__
 
+#include <cassert>
+
 #include "processor/address_map.h"
+#include "processor/logging.h"
 
 namespace google_breakpad {
 
@@ -45,8 +48,11 @@ bool AddressMap<AddressType, EntryType>::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<typename AddressType, typename EntryType>
 bool AddressMap<AddressType, EntryType>::Retrieve(
     const AddressType &address,
     EntryType *entry, AddressType *entry_address) const {
-  if (!entry)
+  BPLOG_IF(ERROR, !entry) << "AddressMap::Retrieve requires |entry|";
-    return false;
+  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.

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
+  // the address argument.  If there is no such range, returns false.  The
-  // parameter error, returns false.  The entry is returned in entry.  If
+  // 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,

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.

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<u_int64_t, linked_ptr<const CodeModule> >()) {
+  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(),
+    if (!map_->StoreRange(module->base_address(), module->size(),
-                     linked_ptr<const CodeModule>(module));
+                          linked_ptr<const CodeModule>(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<const CodeModule> 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<const CodeModule> 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();
 }

src/processor/basic_source_line_resolver.cc

@@ -145,7 +145,7 @@ class BasicSourceLineResolver::Module {
   static bool Tokenize(char *line, int max_tokens, vector<char*> *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<Function> 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 <guid> <age> <filename>
     } 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<unsigned int>(max_tokens);
 }
 
-void BasicSourceLineResolver::Module::ParseFile(char *file_line) {
+bool BasicSourceLineResolver::Module::ParseFile(char *file_line) {
   // FILE <id> <filename>
   file_line += 5;  // skip prefix
 
   vector<char*> 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) {
+  if (!filename) {
-    files_.insert(make_pair(index, string(filename)));
+    return false;
   }
+
+  files_.insert(make_pair(index, string(filename)));
+  return true;
 }
 
 BasicSourceLineResolver::Function*

src/processor/contained_range_map-inl.h

@@ -37,7 +37,10 @@
 #define PROCESSOR_CONTAINED_RANGE_MAP_INL_H__
 
 
+#include <cassert>
+
 #include "processor/contained_range_map.h"
+#include "processor/logging.h"
 
 
 namespace google_breakpad {
@@ -56,8 +59,11 @@ bool ContainedRangeMap<AddressType, EntryType>::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<AddressType, EntryType>::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<AddressType, EntryType>::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<AddressType, EntryType>::StoreRange(
 template<typename AddressType, typename EntryType>
 bool ContainedRangeMap<AddressType, EntryType>::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

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
+  // encompasses the address, returns false.
-  // false.
   bool RetrieveRange(const AddressType &address, EntryType *entry) const;
 
   // Removes all children.  Note that Clear only removes descendants,

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;
   }
 

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<ValueType>::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<ValueType>::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<ValueType>::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<ValueType>::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<ValueType>::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;
     }
 

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<AddressType, EntryType>::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,6 +70,12 @@ bool RangeMap<AddressType, EntryType>::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;
   }
 
@@ -70,6 +84,13 @@ bool RangeMap<AddressType, EntryType>::StoreRange(const AddressType &base,
       // The range above this one overlaps with this one.  It may fully
       // 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<typename AddressType, typename EntryType>
 bool RangeMap<AddressType, EntryType>::RetrieveRange(
     const AddressType &address, EntryType *entry,
     AddressType *entry_base, AddressType *entry_size) const {
-  if (!entry)
+  BPLOG_IF(ERROR, !entry) << "RangeMap::RetrieveRange requires |entry|";
-    return false;
+  assert(entry);
 
   MapConstIterator iterator = map_.lower_bound(address);
   if (iterator == map_.end())
@@ -114,8 +135,8 @@ template<typename AddressType, typename EntryType>
 bool RangeMap<AddressType, EntryType>::RetrieveNearestRange(
     const AddressType &address, EntryType *entry,
     AddressType *entry_base, AddressType *entry_size) const {
-  if (!entry)
+  BPLOG_IF(ERROR, !entry) << "RangeMap::RetrieveNearestRange requires |entry|";
-    return false;
+  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<typename AddressType, typename EntryType>
 bool RangeMap<AddressType, EntryType>::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.

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.
+  // no such range, returns false.  entry_base and entry_size, if non-NULL,
-  // entry_base and entry_size, if non-NULL, are set to the base and size
+  // are set to the base and size of the entry's range.
-  // 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
+  // false if index is larger than the number of ranges stored.  entry_base
-  // parameter error occurs.  entry_base and entry_size, if non-NULL, are set
+  // and entry_size, if non-NULL, are set to the base and size of the entry's
-  // to the base and size of the entry's range.
+  // range.
   //
   // RetrieveRangeAtIndex is not optimized for speedy operation.
   bool RetrieveRangeAtIndex(int index, EntryType *entry,

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;

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;
 }
 

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<StackFrameInfo> > &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

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<StackFrameInfo> > &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<StackFrameX86*>(
       stack->frames()->back());