Fix incorrect source file name for inlined frames

Processor shows incorrect source file name if a frame have an inlined
frame and their source files are different.
Consider this example:
FILE 0 /tmp/a.h
FILE 1 /tmp/a.cpp
INLINE_ORIGIN 0 0 foo()
FUNC 1110 a 0 main
INLINE 0 22 0 1110 7
1110 7 3 0
1117 3 23 1

When querying the address 0x1110, we know this line 0x1110 corresponds
to /tmp/a.h line 3 and it's inside a inlined function foo() which is
defined at /tmp/a.h and called at line 22. But we don't know at which
file it's being called at line 22. So, we will get stacks like this:
void foo() /tmp/a.h:3
int main() /tmp/a.h:22

The correct stacks should be this:
void foo() /tmp/a.h:3
int main() /tmp/a.cpp:22

In this change:
1. Remove file_id field for INLINE_ORIGIN record.
2. Add call_site_file_id for INLINE record to represents the file where
this call being inlined.

After adding call_site_file_id to it (as third field), it looks like
this:
FILE 0 /tmp/a.h
FILE 1 /tmp/a.cpp
INLINE_ORIGIN 0 foo()
FUNC 1110 a 0 main
INLINE 0 22 1 0 1110 7
1110 7 3 0
1117 3 23 1

Bug: 1190878
Change-Id: Ibbb697d2f7e1b6ac3208cac6fae4353c8743198d
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3232838
Reviewed-by: Joshua Peraza <jperaza@chromium.org>

src/common/dwarf_cu_to_module.cc

@@ -254,9 +254,6 @@ struct DwarfCUToModule::CUContext {
 
   // A map of function pointers to the its forward specification DIE's offset.
   map<Module::Function*, uint64_t> spec_function_offsets;
-
-  // From file index to vector of subprogram's offset in this CU.
-  map<uint64_t, vector<uint64_t>> inline_origins;
 };
 
 // Information about the context of a particular DIE. This is for
@@ -561,7 +558,8 @@ class DwarfCUToModule::InlineHandler : public GenericDIEHandler {
   DwarfForm high_pc_form_;     // DW_AT_high_pc can be length or address.
   DwarfForm ranges_form_;      // DW_FORM_sec_offset or DW_FORM_rnglistx
   uint64_t ranges_data_;       // DW_AT_ranges
-  int call_site_line_;
+  int call_site_line_;         // DW_AT_call_line
+  int call_site_file_id_;      // DW_AT_call_file
   int inline_nest_level_;
   // A vector of inlines in the same nest level. It's owned by its parent
   // function/inline. At Finish(), add this inline into the vector.
@@ -589,6 +587,9 @@ void DwarfCUToModule::InlineHandler::ProcessAttributeUnsigned(
     case DW_AT_call_line:
       call_site_line_ = data;
       break;
+    case DW_AT_call_file:
+      call_site_file_id_ = data;
+      break;
     default:
       GenericDIEHandler::ProcessAttributeUnsigned(attr, form, data);
       break;
@@ -661,8 +662,8 @@ void DwarfCUToModule::InlineHandler::Finish() {
       cu_context_->file_context->module_->inline_origin_map
           .GetOrCreateInlineOrigin(specification_offset_, name_);
   unique_ptr<Module::Inline> in(
-      new Module::Inline(origin, ranges, call_site_line_, inline_nest_level_,
-                         std::move(child_inlines_)));
+      new Module::Inline(origin, ranges, call_site_line_, call_site_file_id_,
+                         inline_nest_level_, std::move(child_inlines_)));
   inlines_.push_back(std::move(in));
 }
 
@@ -679,7 +680,6 @@ class DwarfCUToModule::FuncHandler: public GenericDIEHandler {
         high_pc_form_(DW_FORM_addr),
         ranges_form_(DW_FORM_sec_offset),
         ranges_data_(0),
-        decl_file_data_(UINT64_MAX),
         inline_(false),
         handle_inline_(handle_inline) {}
 
@@ -700,9 +700,7 @@ class DwarfCUToModule::FuncHandler: public GenericDIEHandler {
   uint64_t low_pc_, high_pc_; // DW_AT_low_pc, DW_AT_high_pc
   DwarfForm high_pc_form_; // DW_AT_high_pc can be length or address.
   DwarfForm ranges_form_; // DW_FORM_sec_offset or DW_FORM_rnglistx
-  uint64_t ranges_data_; // DW_AT_ranges
-  // DW_AT_decl_file, value of UINT64_MAX means undefined.
-  uint64_t decl_file_data_;
+  uint64_t ranges_data_;  // DW_AT_ranges
   bool inline_;
   vector<unique_ptr<Module::Inline>> child_inlines_;
   bool handle_inline_;
@@ -727,9 +725,6 @@ void DwarfCUToModule::FuncHandler::ProcessAttributeUnsigned(
       ranges_data_ = data;
       ranges_form_ = form;
       break;
-    case DW_AT_decl_file:
-      decl_file_data_ = data;
-      break;
     default:
       GenericDIEHandler::ProcessAttributeUnsigned(attr, form, data);
       break;
@@ -857,17 +852,14 @@ void DwarfCUToModule::FuncHandler::Finish() {
 
   // Only keep track of DW_TAG_subprogram which have the attributes we are
   // interested.
-  if (handle_inline_ &&
-      (!empty_range || inline_ || decl_file_data_ != UINT64_MAX)) {
+  if (handle_inline_ && (!empty_range || inline_)) {
     StringView name = name_.empty() ? name_omitted : name_;
     uint64_t offset =
         specification_offset_ != 0 ? specification_offset_ : offset_;
     cu_context_->file_context->module_->inline_origin_map.SetReference(offset_,
                                                                        offset);
     cu_context_->file_context->module_->inline_origin_map
         .GetOrCreateInlineOrigin(offset_, name);
-    if (decl_file_data_ != UINT64_MAX)
-      cu_context_->inline_origins[decl_file_data_].push_back(offset_);
   }
 }
 
@@ -1450,10 +1442,12 @@ void DwarfCUToModule::AssignLinesToFunctions() {
 }
 
 void DwarfCUToModule::AssignFilesToInlines() {
-  for (auto iter : files_) {
-    cu_context_->file_context->module_->inline_origin_map
-        .AssignFilesToInlineOrigins(cu_context_->inline_origins[iter.first],
-                                    iter.second);
+  // Assign File* to Inlines inside this CU.
+  auto assignFile = [this](unique_ptr<Module::Inline>& in) {
+    in->call_site_file = files_[in->call_site_file_id];
+  };
+  for (auto func : cu_context_->functions) {
+    Module::Inline::InlineDFS(func->inlines, assignFile);
   }
 }
 

src/common/module.cc

@@ -97,17 +97,6 @@ void Module::InlineOriginMap::SetReference(uint64_t offset,
   references_[offset] = specification_offset;
 }
 
-void Module::InlineOriginMap::AssignFilesToInlineOrigins(
-    const vector<uint64_t>& inline_origin_offsets,
-    Module::File* file) {
-  for (uint64_t offset : inline_origin_offsets)
-    if (references_.find(offset) != references_.end()) {
-      auto origin = inline_origins_.find(references_[offset]);
-      if (origin != inline_origins_.end())
-        origin->second->file = file;
-    }
-}
-
 Module::Module(const string& name, const string& os,
                const string& architecture, const string& id,
                const string& code_id /* = "" */) :
@@ -276,13 +265,18 @@ void Module::AssignSourceIds(
          line_it != func->lines.end(); ++line_it)
       line_it->file->source_id = 0;
   }
-  // Also mark all files cited by inline functions by setting each one's source
+
+  // Also mark all files cited by inline callsite by setting each one's source
   // id to zero.
-  for (InlineOrigin* origin : inline_origins)
+  auto markInlineFiles = [](unique_ptr<Inline>& in) {
     // There are some artificial inline functions which don't belong to
     // any file. Those will have file id -1.
-    if (origin->file)
-      origin->file->source_id = 0;
+    if (in->call_site_file)
+      in->call_site_file->source_id = 0;
+  };
+  for (auto func : functions_) {
+    Inline::InlineDFS(func->inlines, markInlineFiles);
+  }
 
   // Finally, assign source ids to those files that have been marked.
   // We could have just assigned source id numbers while traversing
@@ -296,15 +290,6 @@ void Module::AssignSourceIds(
   }
 }
 
-static void InlineDFS(
-    vector<unique_ptr<Module::Inline>>& inlines,
-    std::function<void(unique_ptr<Module::Inline>&)> const& forEach) {
-  for (unique_ptr<Module::Inline>& in : inlines) {
-    forEach(in);
-    InlineDFS(in->child_inlines, forEach);
-  }
-}
-
 void Module::CreateInlineOrigins(
     set<InlineOrigin*, InlineOriginCompare>& inline_origins) {
   // Only add origins that have file and deduplicate origins with same name and
@@ -317,7 +302,7 @@ void Module::CreateInlineOrigins(
       in->origin = *it;
   };
   for (Function* func : functions_)
-    InlineDFS(func->inlines, addInlineOrigins);
+    Module::Inline::InlineDFS(func->inlines, addInlineOrigins);
   int next_id = 0;
   for (InlineOrigin* origin : inline_origins) {
     origin->id = next_id++;
@@ -381,8 +366,7 @@ bool Module::Write(std::ostream& stream, SymbolData symbol_data) {
     }
     // Write out inline origins.
     for (InlineOrigin* origin : inline_origins) {
-      stream << "INLINE_ORIGIN " << origin->id << " " << origin->getFileID()
-             << " " << origin->name << "\n";
+      stream << "INLINE_ORIGIN " << origin->id << " " << origin->name << "\n";
       if (!stream.good())
         return ReportError();
     }
@@ -407,12 +391,12 @@ bool Module::Write(std::ostream& stream, SymbolData symbol_data) {
         auto write_inline = [&](unique_ptr<Inline>& in) {
           stream << "INLINE ";
           stream << in->inline_nest_level << " " << in->call_site_line << " "
-                 << in->origin->id << hex;
+                 << in->getCallSiteFileID() << " " << in->origin->id << hex;
           for (const Range& r : in->ranges)
             stream << " " << (r.address - load_address_) << " " << r.size;
           stream << dec << "\n";
         };
-        InlineDFS(func->inlines, write_inline);
+        Module::Inline::InlineDFS(func->inlines, write_inline);
         if (!stream.good())
           return ReportError();
 

src/common/module.h

@@ -38,6 +38,7 @@
 #ifndef COMMON_LINUX_MODULE_H__
 #define COMMON_LINUX_MODULE_H__
 
+#include <functional>
 #include <iostream>
 #include <limits>
 #include <map>
@@ -131,30 +132,29 @@ class Module {
   };
 
   struct InlineOrigin {
-    explicit InlineOrigin(StringView name): id(-1), name(name), file(nullptr) {}
+    explicit InlineOrigin(StringView name) : id(-1), name(name) {}
 
     // A unique id for each InlineOrigin object. INLINE records use the id to
     // refer to its INLINE_ORIGIN record.
     int id;
 
     // The inlined function's name.
     StringView name;
-
-    File* file;
-
-    int getFileID() const { return file ? file->source_id : -1; }
   };
 
   // A inlined call site.
   struct Inline {
     Inline(InlineOrigin* origin,
            const vector<Range>& ranges,
            int call_site_line,
+           int call_site_file_id,
            int inline_nest_level,
            vector<std::unique_ptr<Inline>> child_inlines)
         : origin(origin),
           ranges(ranges),
           call_site_line(call_site_line),
+          call_site_file_id(call_site_file_id),
+          call_site_file(nullptr),
           inline_nest_level(inline_nest_level),
           child_inlines(std::move(child_inlines)) {}
 
@@ -165,10 +165,29 @@ class Module {
 
     int call_site_line;
 
+    // The id is only meanful inside a CU. It's only used for looking up real
+    // File* after scanning a CU.
+    int call_site_file_id;
+
+    File* call_site_file;
+
     int inline_nest_level;
 
     // A list of inlines which are children of this inline.
     vector<std::unique_ptr<Inline>> child_inlines;
+
+    int getCallSiteFileID() const {
+      return call_site_file ? call_site_file->source_id : -1;
+    }
+
+    static void InlineDFS(
+        vector<std::unique_ptr<Module::Inline>>& inlines,
+        std::function<void(std::unique_ptr<Module::Inline>&)> const& forEach) {
+      for (std::unique_ptr<Module::Inline>& in : inlines) {
+        forEach(in);
+        InlineDFS(in->child_inlines, forEach);
+      }
+    }
   };
 
   typedef map<uint64_t, InlineOrigin*> InlineOriginByOffset;
@@ -182,9 +201,7 @@ class Module {
     // value of its DW_AT_specification or equals to offset if
     // DW_AT_specification doesn't exist in that DIE.
     void SetReference(uint64_t offset, uint64_t specification_offset);
-    void AssignFilesToInlineOrigins(
-        const vector<uint64_t>& inline_origin_offsets,
-        File* file);
+
     ~InlineOriginMap() {
       for (const auto& iter : inline_origins_) {
         delete iter.second;
@@ -261,10 +278,8 @@ class Module {
   };
 
   struct InlineOriginCompare {
-    bool operator() (const InlineOrigin* lhs, const InlineOrigin* rhs) const {
-      if (lhs->getFileID() == rhs->getFileID())
-        return lhs->name < rhs->name;
-      return lhs->getFileID() < rhs->getFileID();
+    bool operator()(const InlineOrigin* lhs, const InlineOrigin* rhs) const {
+      return lhs->name < rhs->name;
     }
   };
 

src/google_breakpad/processor/basic_source_line_resolver.h

@@ -98,28 +98,28 @@ class SymbolParseHelper {
                         char** filename);  // out
 
   // Parses a |inline_origin_line| declaration.  Returns true on success.
-  // Format: INLINE_ORIGIN <origin_id> <file_id> <name>.
+  // Format: INLINE_ORIGIN <origin_id> <name>.
   // Notice, that this method modifies the input |inline_origin_line| which is
-  // why it can't be const.  On success, <origin_id>, <file_id> and <name> are
-  // stored in |*origin_id|, |*file_id|, and |*name|.  No allocation is
+  // why it can't be const.  On success, <origin_id> and <name> are
+  // stored in |*origin_id| and |*name|.  No allocation is
   // done, |*name| simply points inside |inline_origin_line|.
   static bool ParseInlineOrigin(char* inline_origin_line,  // in
                                 long* origin_id,           // out
-                                long* file_id,             // out
                                 char** name);              // out
 
   // Parses a |inline| declaration.  Returns true on success.
-  // Format: INLINE <inline_nest_level> <call_site_line> <origin_id> <address>
-  // <size> ....
-  // Notice, that this method modifies the input |inline|
-  // which is why it can't be const.  On success, <inline_nest_level>,
-  // <call_site_line> and <origin_id> are stored in |*inline_nest_level|,
-  // |*call_site_line|, and |*origin_id|, and all pairs of (<address>, <size>)
-  // are added into ranges .
+  // Format: INLINE <inline_nest_level> <call_site_line> <call_site_file_id>
+  // <origin_id> [<address> <size>]+
+  // Notice, that this method modifies the input
+  // |inline| which is why it can't be const.  On success, <inline_nest_level>,
+  // <call_site_line>, <call_site_file_id> and <origin_id> are stored in
+  // |*inline_nest_level|, |*call_site_line|, |*call_site_file_id| and
+  // |*origin_id|, and all pairs of (<address>, <size>) are added into ranges.
   static bool ParseInline(
       char* inline_line,                                  // in
       long* inline_nest_level,                            // out
       long* call_site_line,                               // out
+      long* call_site_file_id,                            // out
       long* origin_id,                                    // out
       std::vector<std::pair<MemAddr, MemAddr>>* ranges);  // out
 

src/google_breakpad/processor/source_line_resolver_base.h

@@ -41,6 +41,7 @@
 #ifndef GOOGLE_BREAKPAD_PROCESSOR_SOURCE_LINE_RESOLVER_BASE_H__
 #define GOOGLE_BREAKPAD_PROCESSOR_SOURCE_LINE_RESOLVER_BASE_H__
 
+#include <deque>
 #include <map>
 #include <set>
 #include <string>
@@ -86,7 +87,7 @@ class SourceLineResolverBase : public SourceLineResolverInterface {
   virtual bool IsModuleCorrupt(const CodeModule* module);
   virtual void FillSourceLineInfo(
       StackFrame* frame,
-      std::vector<std::unique_ptr<StackFrame>>* inlined_frames);
+      std::deque<std::unique_ptr<StackFrame>>* inlined_frames);
   virtual WindowsFrameInfo* FindWindowsFrameInfo(const StackFrame* frame);
   virtual CFIFrameInfo* FindCFIFrameInfo(const StackFrame* frame);
 

src/google_breakpad/processor/source_line_resolver_interface.h

@@ -34,6 +34,7 @@
 #ifndef GOOGLE_BREAKPAD_PROCESSOR_SOURCE_LINE_RESOLVER_INTERFACE_H__
 #define GOOGLE_BREAKPAD_PROCESSOR_SOURCE_LINE_RESOLVER_INTERFACE_H__
 
+#include <deque>
 #include <memory>
 #include <string>
 #include <vector>
@@ -98,7 +99,7 @@ class SourceLineResolverInterface {
   // inlined_frames in an order from outermost frame to inner most frame.
   virtual void FillSourceLineInfo(
       StackFrame* frame,
-      std::vector<std::unique_ptr<StackFrame>>* inlined_frames) = 0;
+      std::deque<std::unique_ptr<StackFrame>>* inlined_frames) = 0;
 
   // If Windows stack walking information is available covering
   // FRAME's instruction address, return a WindowsFrameInfo structure

src/google_breakpad/processor/stack_frame_symbolizer.h

@@ -35,6 +35,7 @@
 #ifndef GOOGLE_BREAKPAD_PROCESSOR_STACK_FRAME_SYMBOLIZER_H__
 #define GOOGLE_BREAKPAD_PROCESSOR_STACK_FRAME_SYMBOLIZER_H__
 
+#include <deque>
 #include <memory>
 #include <set>
 #include <string>
@@ -82,7 +83,7 @@ class StackFrameSymbolizer {
       const CodeModules* unloaded_modules,
       const SystemInfo* system_info,
       StackFrame* stack_frame,
-      std::vector<std::unique_ptr<StackFrame>>* inlined_frames);
+      std::deque<std::unique_ptr<StackFrame>>* inlined_frames);
 
   virtual WindowsFrameInfo* FindWindowsFrameInfo(const StackFrame* frame);