Modify symbol supplier interface to support an overload that takes a symbol data buffer, to get around an extraneous read/write of symbol data

R=doshimun



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

7 files changed, 166 insertions, 40 deletions

diff --git a/src/google_breakpad/processor/basic_source_line_resolver.h b/src/google_breakpad/processor/basic_source_line_resolver.h
index 814b01ae..38759579 100644
--- a/src/google_breakpad/processor/basic_source_line_resolver.h
+++ b/src/google_breakpad/processor/basic_source_line_resolver.h
@@ -69,6 +69,12 @@ class BasicSourceLineResolver : public SourceLineResolverInterface {
   // retained until the BasicSourceLineResolver is destroyed.
   virtual bool LoadModule(const string &module_name, const string &map_file);
 
+  // Exactly the same as above, except the given map_buffer is used
+  // for symbols. 
+  virtual bool LoadModuleUsingMapBuffer(const string &module_name,
+                                        const string &map_buffer);
+
+
   virtual bool HasModule(const string &module_name) const;
 
   virtual StackFrameInfo* FillSourceLineInfo(StackFrame *frame) const;
diff --git a/src/google_breakpad/processor/source_line_resolver_interface.h b/src/google_breakpad/processor/source_line_resolver_interface.h
index 192177b2..93ef85d2 100644
--- a/src/google_breakpad/processor/source_line_resolver_interface.h
+++ b/src/google_breakpad/processor/source_line_resolver_interface.h
@@ -56,6 +56,9 @@ class SourceLineResolverInterface {
   // map_file should contain line/address mappings for this module.
   virtual bool LoadModule(const string &module_name,
                           const string &map_file) = 0;
+  // Same as above, but takes the contents of a pre-read map buffer
+  virtual bool LoadModuleUsingMapBuffer(const string &module_name,
+                                        const string &map_buffer) = 0;
 
   // Returns true if a module with the given name has been loaded.
   virtual bool HasModule(const string &module_name) const = 0;
diff --git a/src/google_breakpad/processor/symbol_supplier.h b/src/google_breakpad/processor/symbol_supplier.h
index 0459c8aa..752bbbc3 100644
--- a/src/google_breakpad/processor/symbol_supplier.h
+++ b/src/google_breakpad/processor/symbol_supplier.h
@@ -59,12 +59,22 @@ class SymbolSupplier {
 
   // Retrieves the symbol file for the given CodeModule, placing the
   // path in symbol_file if successful.  system_info contains strings
-  // identifying the operating system and CPU; SymbolSupplier may use to help
-  // locate the symbol file.  system_info may be NULL or its fields may be
-  // empty if these values are unknown.
+  // identifying the operating system and CPU; SymbolSupplier may use
+  // to help locate the symbol file.  system_info may be NULL or its
+  // fields may be empty if these values are unknown.  symbol_file
+  // must be a pointer to a valid string
   virtual SymbolResult GetSymbolFile(const CodeModule *module,
                                      const SystemInfo *system_info,
                                      string *symbol_file) = 0;
+  // Same as above, except also places symbol data into symbol_data.
+  // If symbol_data is NULL, the data is not returned.  
+  // TODO(nealsid) Once we have symbol data caching behavior implemented
+  // investigate making all symbol suppliers implement all methods,
+  // and make this pure virtual
+  virtual SymbolResult GetSymbolFile(const CodeModule *module,
+                                     const SystemInfo *system_info,
+                                     string *symbol_file,
+                                     string *symbol_data) { assert(0); }
 };
 
 }  // namespace google_breakpad
diff --git a/src/processor/basic_source_line_resolver.cc b/src/processor/basic_source_line_resolver.cc
index 6baa1a8b..bba86599 100644
--- a/src/processor/basic_source_line_resolver.cc
+++ b/src/processor/basic_source_line_resolver.cc
@@ -29,6 +29,9 @@
 
 #include <stdio.h>
 #include <string.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
 
 #include <map>
 #include <utility>
@@ -106,9 +109,13 @@ class BasicSourceLineResolver::Module {
  public:
   Module(const string &name) : name_(name) { }
 
-  // Loads the given map file, returning true on success.
+  // Loads the given map file, returning true on success.  Reads the
+  // map file into memory and calls LoadMapFromBuffer
   bool LoadMap(const string &map_file);
 
+  // Loads a map from the given buffer, returning true on success
+  bool LoadMapFromBuffer(const string &map_buffer);
+
   // Looks up the given relative address, and fills the StackFrame struct
   // with the result.  Additional debugging information, if available, is
   // returned.  If no additional information is available, returns NULL.
@@ -211,6 +218,27 @@ bool BasicSourceLineResolver::LoadModule(const string &module_name,
   return true;
 }
 
+bool BasicSourceLineResolver::LoadModuleUsingMapBuffer(
+    const string &module_name,
+    const string &map_buffer) {
+  // 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 buffer";
+
+  Module *module = new Module(module_name);
+  if (!module->LoadMapFromBuffer(map_buffer)) {
+    delete module;
+    return false;
+  }
+
+  modules_->insert(make_pair(module_name, module));
+  return true;
+}
+
 bool BasicSourceLineResolver::HasModule(const string &module_name) const {
   return modules_->find(module_name) != modules_->end();
 }
@@ -238,44 +266,56 @@ class AutoFileCloser {
   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;
+bool BasicSourceLineResolver::Module::LoadMapFromBuffer(
+    const string &map_buffer) {
+  linked_ptr<Function> cur_func;
+  int line_number = 0;
+  const char *map_buffer_c_str = map_buffer.c_str();
+  char *save_ptr;
+
+  // set up our input buffer as a c-style string so we
+  // can we use strtok()
+  // have to copy because modifying the result of string::c_str is not
+  // permitted
+  size_t map_buffer_length = strlen(map_buffer_c_str);
+  char *map_buffer_chars = new char[map_buffer_length];
+  if (map_buffer_chars == NULL) {
+    BPLOG(ERROR) << "Memory allocation of " << map_buffer_length <<
+        " bytes failed";
     return false;
   }
 
-  AutoFileCloser closer(f);
+  strncpy(map_buffer_chars, map_buffer_c_str, map_buffer_length);
 
-  // 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;
+  if (map_buffer_chars[map_buffer_length - 1] == '\n') {
+    map_buffer_chars[map_buffer_length - 1] = '\0';
+  }
+  char *buffer;
+  buffer = strtok_r(map_buffer_chars, "\r\n", &save_ptr);
 
-  int line_number = 0;
-  while (fgets(buffer, sizeof(buffer), f)) {
+  while (buffer != NULL) {
     ++line_number;
+
     if (strncmp(buffer, "FILE ", 5) == 0) {
       if (!ParseFile(buffer)) {
-        BPLOG(ERROR) << "ParseFile failed at " <<
-                        map_file << ":" << line_number;
+        BPLOG(ERROR) << "ParseFile on buffer failed at " <<
+            ":" << line_number;
+        delete [] map_buffer_chars;
         return false;
       }
     } else if (strncmp(buffer, "STACK ", 6) == 0) {
       if (!ParseStackInfo(buffer)) {
         BPLOG(ERROR) << "ParseStackInfo failed at " <<
-                        map_file << ":" << line_number;
+            ":" << line_number;
+        delete [] map_buffer_chars;
         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;
+            ":" << line_number;
+        delete [] map_buffer_chars;
         return false;
       }
       // StoreRange will fail if the function has an invalid address or size.
@@ -288,7 +328,8 @@ bool BasicSourceLineResolver::Module::LoadMap(const string &map_file) {
 
       if (!ParsePublicSymbol(buffer)) {
         BPLOG(ERROR) << "ParsePublicSymbol failed at " <<
-                        map_file << ":" << line_number;
+            ":" << line_number;
+        delete [] map_buffer_chars;
         return false;
       }
     } else if (strncmp(buffer, "MODULE ", 7) == 0) {
@@ -301,23 +342,82 @@ bool BasicSourceLineResolver::Module::LoadMap(const string &map_file) {
     } else {
       if (!cur_func.get()) {
         BPLOG(ERROR) << "Found source line data without a function at " <<
-                        map_file << ":" << line_number;
+            ":" << line_number;
+        delete [] map_buffer_chars;
         return false;
       }
       Line *line = ParseLine(buffer);
       if (!line) {
-        BPLOG(ERROR) << "ParseLine failed at " <<
-                        map_file << ":" << line_number;
+        BPLOG(ERROR) << "ParseLine failed at " << line_number << " for " <<
+            buffer;
+        delete [] map_buffer_chars;
         return false;
       }
       cur_func->lines.StoreRange(line->address, line->size,
                                  linked_ptr<Line>(line));
     }
+
+    buffer = strtok_r(NULL, "\r\n", &save_ptr);
   }
 
   return true;
 }
 
+bool BasicSourceLineResolver::Module::LoadMap(const string &map_file) {
+  struct stat buf;
+  int error_code = stat(map_file.c_str(), &buf);
+  if (error_code == -1) {
+    string error_string;
+    int error_code = ErrnoString(&error_string);
+    BPLOG(ERROR) << "Could not open " << map_file <<
+        ", error " << error_code << ": " << error_string;
+    return false;
+  }
+
+  off_t file_size = buf.st_size;
+
+  // Allocate memory for file contents, plus a null terminator
+  // since we'll use strtok() on the contents.
+  char *file_buffer = new char[sizeof(char)*file_size + 1];
+
+  if (file_buffer == NULL) {
+    BPLOG(ERROR) << "Could not allocate memory for " << map_file;
+    return false;
+  }
+
+  BPLOG(ERROR) << "Opening " << map_file;
+
+  FILE *f = fopen(map_file.c_str(), "rt");
+  if (!f) {
+    string error_string;
+    int error_code = ErrnoString(&error_string);
+    BPLOG(ERROR) << "Could not open " << map_file <<
+        ", error " << error_code << ": " << error_string;
+    delete [] file_buffer;
+    return false;
+  }
+
+  AutoFileCloser closer(f);
+
+  int items_read = 0;
+
+  items_read = fread(file_buffer, 1, file_size, f);
+
+  if (items_read != file_size) {
+    string error_string;
+    int error_code = ErrnoString(&error_string);
+    BPLOG(ERROR) << "Could not slurp " << map_file <<
+        ", error " << error_code << ": " << error_string;
+    delete [] file_buffer;
+    return false;
+  }
+  file_buffer[file_size] = '\0';
+  string map_buffer(file_buffer);
+  delete [] file_buffer;
+
+  return LoadMapFromBuffer(map_buffer);
+}
+
 StackFrameInfo* BasicSourceLineResolver::Module::LookupAddress(
     StackFrame *frame) const {
   MemAddr address = frame->instruction - frame->module->base_address();
diff --git a/src/processor/simple_symbol_supplier.cc b/src/processor/simple_symbol_supplier.cc
index 291361cb..9c1aac4e 100644
--- a/src/processor/simple_symbol_supplier.cc
+++ b/src/processor/simple_symbol_supplier.cc
@@ -62,15 +62,16 @@ SymbolSupplier::SymbolResult SimpleSymbolSupplier::GetSymbolFile(
 
   for (unsigned int path_index = 0; path_index < paths_.size(); ++path_index) {
     SymbolResult result;
-    if ((result = GetSymbolFileAtPath(module, system_info, paths_[path_index],
-                                      symbol_file)) != NOT_FOUND) {
+    if ((result = GetSymbolFileAtPathFromRoot(module, system_info,
+                                              paths_[path_index],
+                                              symbol_file)) != NOT_FOUND) {
       return result;
     }
   }
   return NOT_FOUND;
 }
 
-SymbolSupplier::SymbolResult SimpleSymbolSupplier::GetSymbolFileAtPath(
+SymbolSupplier::SymbolResult SimpleSymbolSupplier::GetSymbolFileAtPathFromRoot(
     const CodeModule *module, const SystemInfo *system_info,
     const string &root_path, string *symbol_file) {
   BPLOG_IF(ERROR, !symbol_file) << "SimpleSymbolSupplier::GetSymbolFileAtPath "
diff --git a/src/processor/simple_symbol_supplier.h b/src/processor/simple_symbol_supplier.h
index 44e0d617..9343e448 100644
--- a/src/processor/simple_symbol_supplier.h
+++ b/src/processor/simple_symbol_supplier.h
@@ -102,15 +102,19 @@ class SimpleSymbolSupplier : public SymbolSupplier {
 
   // Returns the path to the symbol file for the given module.  See the
   // description above.
-  SymbolResult GetSymbolFile(const CodeModule *module,
-                             const SystemInfo *system_info,
-                             string *symbol_file);
+  virtual SymbolResult GetSymbolFile(const CodeModule *module,
+                                     const SystemInfo *system_info,
+                                     string *symbol_file);
 
+  virtual SymbolResult GetSymbolFile(const CodeModule *module,
+                             const SystemInfo *system_info,
+                             string *symbol_file,
+                             string *symbol_data) { assert(0); }
  protected:
-  SymbolResult GetSymbolFileAtPath(const CodeModule *module,
-                                   const SystemInfo *system_info,
-                                   const string &root_path,
-                                   string *symbol_file);
+  SymbolResult GetSymbolFileAtPathFromRoot(const CodeModule *module,
+                                           const SystemInfo *system_info,
+                                           const string &root_path,
+                                           string *symbol_file);
 
  private:
   vector<string> paths_;
diff --git a/src/processor/stackwalker.cc b/src/processor/stackwalker.cc
index cdb0ce52..de67bdaa 100644
--- a/src/processor/stackwalker.cc
+++ b/src/processor/stackwalker.cc
@@ -101,13 +101,15 @@ bool Stackwalker::Walk(CallStack *stack) {
         if (resolver_ &&
             !resolver_->HasModule(frame->module->code_file()) &&
             supplier_) {
-          string symbol_file;
+          string symbol_data, symbol_file;
           SymbolSupplier::SymbolResult symbol_result =
-              supplier_->GetSymbolFile(module, system_info_, &symbol_file);
+              supplier_->GetSymbolFile(module, system_info_,
+                                       &symbol_file, &symbol_data);
 
           switch (symbol_result) {
             case SymbolSupplier::FOUND:
-              resolver_->LoadModule(frame->module->code_file(), symbol_file);
+              resolver_->LoadModuleUsingMapBuffer(frame->module->code_file(),
+                                                  symbol_data);
               break;
             case SymbolSupplier::NOT_FOUND:
               break;  // nothing to do