Issue 49013: Breakpad Processor: Use a separate API to retrieve Windows stack debugging info.

At the moment, FillSourceLineInfo returns Windows DIA-based stack
walking data. In addition to being ugly, this makes it difficult to
provide access to DWARF CFI-based stack walking data in a symmetrical
way.

This patch changes FillSourceLineInfo to do the single job its name
suggests, and adds a second member function to
SourceLineResolverInterface to retrieve Windows DIA stack walking
information. A sibling member function will provide access to DWARF
CFI stack walking data.

a=jimblandy, r=mmentovai


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

3 files changed, 89 insertions, 74 deletions

diff --git a/src/processor/basic_source_line_resolver.cc b/src/processor/basic_source_line_resolver.cc
index 94a53dbe..1802299b 100644
--- a/src/processor/basic_source_line_resolver.cc
+++ b/src/processor/basic_source_line_resolver.cc
@@ -114,11 +114,15 @@ class BasicSourceLineResolver::Module {
   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.
-  // A NULL return value is not an error.  The caller takes ownership of
-  // any returned WindowsFrameInfo object.
-  WindowsFrameInfo* LookupAddress(StackFrame *frame) const;
+  // with the result.
+  void LookupAddress(StackFrame *frame) const;
+
+  // If Windows stack walking information is available covering ADDRESS,
+  // return a WindowsFrameInfo structure describing it. If the information
+  // is not available, returns NULL. A NULL return value does not indicate
+  // an error. The caller takes ownership of any returned WindowsFrameInfo
+  // object.
+  WindowsFrameInfo *FindWindowsFrameInfo(const StackFrame *frame) const;
 
  private:
   friend class BasicSourceLineResolver;
@@ -236,12 +240,21 @@ bool BasicSourceLineResolver::HasModule(const string &module_name) const {
   return modules_->find(module_name) != modules_->end();
 }
 
-WindowsFrameInfo* BasicSourceLineResolver::FillSourceLineInfo(
-    StackFrame *frame) const {
+void BasicSourceLineResolver::FillSourceLineInfo(StackFrame *frame) const {
   if (frame->module) {
     ModuleMap::const_iterator it = modules_->find(frame->module->code_file());
     if (it != modules_->end()) {
-      return it->second->LookupAddress(frame);
+      it->second->LookupAddress(frame);
+    }
+  }
+}
+
+WindowsFrameInfo *BasicSourceLineResolver::FindWindowsFrameInfo(
+    const StackFrame *frame) const {
+  if (frame->module) {
+    ModuleMap::const_iterator it = modules_->find(frame->module->code_file());
+    if (it != modules_->end()) {
+      return it->second->FindWindowsFrameInfo(frame);
     }
   }
   return NULL;
@@ -413,42 +426,15 @@ bool BasicSourceLineResolver::Module::LoadMap(const string &map_file) {
   return LoadMapFromBuffer(map_buffer);
 }
 
-WindowsFrameInfo* BasicSourceLineResolver::Module::LookupAddress(
-    StackFrame *frame) const {
+void BasicSourceLineResolver::Module::LookupAddress(StackFrame *frame) const {
   MemAddr address = frame->instruction - frame->module->base_address();
 
-  linked_ptr<WindowsFrameInfo> retrieved_info;
-  // Check for debugging info first, before any possible early returns.
-  //
-  // We only know about STACK_INFO_FRAME_DATA and STACK_INFO_FPO.  Prefer
-  // them in this order.  STACK_INFO_FRAME_DATA is the newer type that
-  // includes its own program string.  STACK_INFO_FPO is the older type
-  // corresponding to the FPO_DATA struct.  See stackwalker_x86.cc.
-  if (!stack_info_[STACK_INFO_FRAME_DATA].RetrieveRange(address,
-                                                        &retrieved_info)) {
-    stack_info_[STACK_INFO_FPO].RetrieveRange(address, &retrieved_info);
-  }
-
-  scoped_ptr<WindowsFrameInfo> frame_info;
-  if (retrieved_info.get()) {
-    frame_info.reset(new WindowsFrameInfo());
-    frame_info->CopyFrom(*retrieved_info.get());
-  }
-
-  // First, look for a matching FUNC range.  Use RetrieveNearestRange instead
-  // of RetrieveRange so that the nearest function can be compared to the
-  // nearest PUBLIC symbol if the address does not lie within the function.
-  // Having access to the highest function below address, even when address
-  // is outside of the function, is useful: if the function is higher than
-  // the nearest PUBLIC symbol, then it means that the PUBLIC symbols is not
-  // valid for the address, and no function information should be filled in.
-  // Using RetrieveNearestRange instead of RetrieveRange means that we need
-  // to verify that address is within the range before using a FUNC.
-  //
-  // If no FUNC containing the address is found, look for the nearest PUBLIC
-  // symbol, being careful not to use a public symbol at a lower address than
-  // the nearest FUNC.
-  int parameter_size = 0;
+  // First, look for a FUNC record that covers address. Use
+  // RetrieveNearestRange instead of RetrieveRange so that, if there
+  // is no such function, we can use the next function to bound the
+  // extent of the PUBLIC symbol we find, below. This does mean we
+  // need to check that address indeed falls within the function we
+  // find; do the range comparison in an overflow-friendly way.
   linked_ptr<Function> func;
   linked_ptr<PublicSymbol> public_symbol;
   MemAddr function_base;
@@ -456,9 +442,7 @@ WindowsFrameInfo* BasicSourceLineResolver::Module::LookupAddress(
   MemAddr public_address;
   if (functions_.RetrieveNearestRange(address, &func,
                                       &function_base, &function_size) &&
-      address >= function_base && address < function_base + function_size) {
-    parameter_size = func->parameter_size;
-
+      address >= function_base && address - function_size < function_base) {
     frame->function_name = func->name;
     frame->function_base = frame->module->base_address() + function_base;
 
@@ -474,27 +458,55 @@ WindowsFrameInfo* BasicSourceLineResolver::Module::LookupAddress(
     }
   } else if (public_symbols_.Retrieve(address,
                                       &public_symbol, &public_address) &&
-             (!func.get() || public_address > function_base + function_size)) {
-    parameter_size = public_symbol->parameter_size;
-
+             (!func.get() || public_address - function_size > function_base)) {
     frame->function_name = public_symbol->name;
     frame->function_base = frame->module->base_address() + public_address;
-  } else {
-    // No FUNC or PUBLIC data available.
-    return frame_info.release();
   }
+}
 
-  if (!frame_info.get()) {
-    // Even without a relevant STACK line, many functions contain information
-    // about how much space their parameters consume on the stack.  Prefer
-    // the STACK stuff (above), but if it's not present, take the
-    // information from the FUNC or PUBLIC line.
-    frame_info.reset(new WindowsFrameInfo());
-    frame_info->parameter_size = parameter_size;
-    frame_info->valid |= WindowsFrameInfo::VALID_PARAMETER_SIZE;
+WindowsFrameInfo *BasicSourceLineResolver::Module::FindWindowsFrameInfo(
+    const StackFrame *frame) const {
+  MemAddr address = frame->instruction - frame->module->base_address();
+  scoped_ptr<WindowsFrameInfo> result(new WindowsFrameInfo());
+
+  // We only know about STACK_INFO_FRAME_DATA and STACK_INFO_FPO.  Prefer
+  // them in this order.  STACK_INFO_FRAME_DATA is the newer type that
+  // includes its own program string.  STACK_INFO_FPO is the older type
+  // corresponding to the FPO_DATA struct.  See stackwalker_x86.cc.
+  linked_ptr<WindowsFrameInfo> frame_info;
+  if ((stack_info_[STACK_INFO_FRAME_DATA].RetrieveRange(address, &frame_info))
+      || (stack_info_[STACK_INFO_FPO].RetrieveRange(address, &frame_info))) {
+    result->CopyFrom(*frame_info.get());
+    return result.release();
+  }
+
+  // Even without a relevant STACK line, many functions contain
+  // information about how much space their parameters consume on the
+  // stack. Use RetrieveNearestRange instead of RetrieveRange, so that
+  // we can use the function to bound the extent of the PUBLIC symbol,
+  // below. However, this does mean we need to check that ADDRESS
+  // falls within the retrieved function's range; do the range
+  // comparison in an overflow-friendly way.
+  linked_ptr<Function> function;
+  MemAddr function_base, function_size;
+  if (functions_.RetrieveNearestRange(address, &function,
+                                      &function_base, &function_size) && 
+      address >= function_base && address - function_base < function_size) {
+    result->parameter_size = function->parameter_size;
+    result->valid |= WindowsFrameInfo::VALID_PARAMETER_SIZE;
+    return result.release();
   }
 
-  return frame_info.release();
+  // PUBLIC symbols might have a parameter size. Use the function we
+  // found above to limit the range the public symbol covers.
+  linked_ptr<PublicSymbol> public_symbol;
+  MemAddr public_address;
+  if (public_symbols_.Retrieve(address, &public_symbol, &public_address) &&
+      (!function.get() || public_address - function_size > function_base)) {
+    result->parameter_size = public_symbol->parameter_size;
+  }
+  
+  return NULL;
 }
 
 // static
diff --git a/src/processor/basic_source_line_resolver_unittest.cc b/src/processor/basic_source_line_resolver_unittest.cc
index 688b0dfb..2940831f 100644
--- a/src/processor/basic_source_line_resolver_unittest.cc
+++ b/src/processor/basic_source_line_resolver_unittest.cc
@@ -104,9 +104,10 @@ static bool RunTests() {
   TestCodeModule module1("module1");
 
   StackFrame frame;
+  scoped_ptr<WindowsFrameInfo> frame_info;
   frame.instruction = 0x1000;
   frame.module = NULL;
-  scoped_ptr<WindowsFrameInfo> frame_info(resolver.FillSourceLineInfo(&frame));
+  resolver.FillSourceLineInfo(&frame);
   ASSERT_FALSE(frame.module);
   ASSERT_TRUE(frame.function_name.empty());
   ASSERT_EQ(frame.function_base, 0);
@@ -115,7 +116,7 @@ static bool RunTests() {
   ASSERT_EQ(frame.source_line_base, 0);
 
   frame.module = &module1;
-  frame_info.reset(resolver.FillSourceLineInfo(&frame));
+  resolver.FillSourceLineInfo(&frame);
   ASSERT_EQ(frame.function_name, "Function1_1");
   ASSERT_TRUE(frame.module);
   ASSERT_EQ(frame.module->code_file(), "module1");
@@ -123,6 +124,7 @@ static bool RunTests() {
   ASSERT_EQ(frame.source_file_name, "file1_1.cc");
   ASSERT_EQ(frame.source_line, 44);
   ASSERT_EQ(frame.source_line_base, 0x1000);
+  frame_info.reset(resolver.FindWindowsFrameInfo(&frame));
   ASSERT_TRUE(frame_info.get());
   ASSERT_FALSE(frame_info->allocates_base_pointer);
   ASSERT_EQ(frame_info->program_string,
@@ -131,37 +133,40 @@ static bool RunTests() {
   ClearSourceLineInfo(&frame);
   frame.instruction = 0x800;
   frame.module = &module1;
-  frame_info.reset(resolver.FillSourceLineInfo(&frame));
+  resolver.FillSourceLineInfo(&frame);
   ASSERT_TRUE(VerifyEmpty(frame));
+  frame_info.reset(resolver.FindWindowsFrameInfo(&frame));
   ASSERT_FALSE(frame_info.get());
 
   frame.instruction = 0x1280;
-  frame_info.reset(resolver.FillSourceLineInfo(&frame));
+  resolver.FillSourceLineInfo(&frame);
   ASSERT_EQ(frame.function_name, "Function1_3");
   ASSERT_TRUE(frame.source_file_name.empty());
   ASSERT_EQ(frame.source_line, 0);
+  frame_info.reset(resolver.FindWindowsFrameInfo(&frame));
   ASSERT_TRUE(frame_info.get());
   ASSERT_FALSE(frame_info->allocates_base_pointer);
   ASSERT_TRUE(frame_info->program_string.empty());
 
   frame.instruction = 0x1380;
-  frame_info.reset(resolver.FillSourceLineInfo(&frame));
+  resolver.FillSourceLineInfo(&frame);
   ASSERT_EQ(frame.function_name, "Function1_4");
   ASSERT_TRUE(frame.source_file_name.empty());
   ASSERT_EQ(frame.source_line, 0);
+  frame_info.reset(resolver.FindWindowsFrameInfo(&frame));
   ASSERT_TRUE(frame_info.get());
   ASSERT_FALSE(frame_info->allocates_base_pointer);
   ASSERT_FALSE(frame_info->program_string.empty());
 
   frame.instruction = 0x2000;
-  frame_info.reset(resolver.FillSourceLineInfo(&frame));
+  frame_info.reset(resolver.FindWindowsFrameInfo(&frame));
   ASSERT_FALSE(frame_info.get());
 
   TestCodeModule module2("module2");
 
   frame.instruction = 0x2181;
   frame.module = &module2;
-  frame_info.reset(resolver.FillSourceLineInfo(&frame));
+  resolver.FillSourceLineInfo(&frame);
   ASSERT_EQ(frame.function_name, "Function2_2");
   ASSERT_EQ(frame.function_base, 0x2170);
   ASSERT_TRUE(frame.module);
@@ -169,14 +174,13 @@ static bool RunTests() {
   ASSERT_EQ(frame.source_file_name, "file2_2.cc");
   ASSERT_EQ(frame.source_line, 21);
   ASSERT_EQ(frame.source_line_base, 0x2180);
+  frame_info.reset(resolver.FindWindowsFrameInfo(&frame));
   ASSERT_TRUE(frame_info.get());
   ASSERT_EQ(frame_info->prolog_size, 1);
 
   frame.instruction = 0x216f;
-  WindowsFrameInfo *s;
-  s = resolver.FillSourceLineInfo(&frame);
+  resolver.FillSourceLineInfo(&frame);
   ASSERT_EQ(frame.function_name, "Public2_1");
-  delete s;
 
   ClearSourceLineInfo(&frame);
   frame.instruction = 0x219f;
@@ -186,11 +190,9 @@ static bool RunTests() {
 
   frame.instruction = 0x21a0;
   frame.module = &module2;
-  s = resolver.FillSourceLineInfo(&frame);
+  resolver.FillSourceLineInfo(&frame);
   ASSERT_EQ(frame.function_name, "Public2_2");
 
-  delete s;
-
   ASSERT_FALSE(resolver.LoadModule("module3",
                                    testdata_dir + "/module3_bad.out"));
   ASSERT_FALSE(resolver.HasModule("module3"));
diff --git a/src/processor/stackwalker.cc b/src/processor/stackwalker.cc
index 63fc1437..b40ed0c6 100644
--- a/src/processor/stackwalker.cc
+++ b/src/processor/stackwalker.cc
@@ -118,7 +118,8 @@ bool Stackwalker::Walk(CallStack *stack) {
               return false;
           }
         }
-        frame_info.reset(resolver_->FillSourceLineInfo(frame.get()));
+        resolver_->FillSourceLineInfo(frame.get());
+        frame_info.reset(resolver_->FindWindowsFrameInfo(frame.get()));
       }
     }