Fix inline_origin_map key collision when split dwarf is enabled.

It fixes following two problems:
1. When we have skeleton compilation unit (DW_TAG_skeleton_unit) in a
binary file refers to the complete unit in a split dwarf file
(.dwo/.dwp file), we should use the split dwarf file's path in warning
reporting. Right now, it uses the original file (binary file) path in
warning report, which is incorrect.

For example, if we have chrome.debug which is the binary with skeleton
debug info and chrome.dwp which is the complete debug info and the debug
info in chrome.dwp has some incorrect reference, it will warn on
chrome.debug rather than chrome.dwp

2. When split dwarf is enabled, the global inline_origin_map will likely
encounter key collision because the offsets as keys are now relative to
each CU's offset which is relative to .debug_info section. Also
offsets from different files might collide.

This change makes a inline_origin_map for each debug file and use
offsets only relative to .debug_info section as keys.

Bug: b/280290608
Change-Id: If70e2e1bfcbeeeef2d425c918796d351a0e9ab3b
Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/4544694
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>

src/common/dwarf/dwarf2reader.cc

@@ -81,8 +81,8 @@ CompilationUnit::CompilationUnit(const string& path,
       addr_buffer_(NULL), addr_buffer_length_(0),
       is_split_dwarf_(false), is_type_unit_(false), dwo_id_(0), dwo_name_(),
       skeleton_dwo_id_(0), ranges_base_(0), addr_base_(0),
-      str_offsets_base_(0), have_checked_for_dwp_(false), dwp_path_(),
+      str_offsets_base_(0), have_checked_for_dwp_(false),
-      dwp_byte_reader_(), dwp_reader_() {}
+      should_process_split_dwarf_(false) {}
 
 // Initialize a compilation unit from a .dwo or .dwp file.
 // In this case, we need the .debug_addr section from the
@@ -400,7 +400,13 @@ uint64_t CompilationUnit::Start() {
 
   // Set up our buffer
   buffer_ = iter->second.first + offset_from_section_start_;
-  buffer_length_ = iter->second.second - offset_from_section_start_;
+  if (is_split_dwarf_) {
+    iter = GetSectionByName(sections_, ".debug_info_offset");
+    assert(iter != sections_.end());
+    buffer_length_ = iter->second.second;
+  } else {
+    buffer_length_ = iter->second.second - offset_from_section_start_;
+  }
 
   // Read the header
   ReadHeader();
@@ -461,10 +467,8 @@ uint64_t CompilationUnit::Start() {
   // If this is a skeleton compilation unit generated with split DWARF,
   // and the client needs the full debug info, we need to find the full
   // compilation unit in a .dwo or .dwp file.
-  if (!is_split_dwarf_
+  should_process_split_dwarf_ =
-      && dwo_name_ != NULL
+      !is_split_dwarf_ && dwo_name_ != NULL && handler_->NeedSplitDebugInfo();
-      && handler_->NeedSplitDebugInfo())
-    ProcessSplitDwarf();
 
   return ourlength;
 }
@@ -994,66 +998,68 @@ inline int GetElfWidth(const ElfReader& elf) {
   return 0;
 }
 
-void CompilationUnit::ProcessSplitDwarf() {
+bool CompilationUnit::ProcessSplitDwarf(std::string& split_file,
+                                        SectionMap& sections,
+                                        ByteReader& split_byte_reader,
+                                        uint64_t& cu_offset) {
+  if (!should_process_split_dwarf_)
+    return false;
   struct stat statbuf;
+  bool found_in_dwp = false;
   if (!have_checked_for_dwp_) {
     // Look for a .dwp file in the same directory as the executable.
     have_checked_for_dwp_ = true;
     string dwp_suffix(".dwp");
-    dwp_path_ = path_ + dwp_suffix;
+    std::string dwp_path = path_ + dwp_suffix;
-    if (stat(dwp_path_.c_str(), &statbuf) != 0) {
+    if (stat(dwp_path.c_str(), &statbuf) != 0) {
       // Fall back to a split .debug file in the same directory.
       string debug_suffix(".debug");
-      dwp_path_ = path_;
+      dwp_path = path_;
       size_t found = path_.rfind(debug_suffix);
       if (found + debug_suffix.length() == path_.length())
-        dwp_path_ = dwp_path_.replace(found, debug_suffix.length(), dwp_suffix);
+        dwp_path = dwp_path.replace(found, debug_suffix.length(), dwp_suffix);
     }
-    if (stat(dwp_path_.c_str(), &statbuf) == 0) {
+    if (stat(dwp_path.c_str(), &statbuf) == 0) {
-      ElfReader* elf = new ElfReader(dwp_path_);
+      split_elf_reader_ = std::make_unique<ElfReader>(dwp_path);
-      int width = GetElfWidth(*elf);
+      int width = GetElfWidth(*split_elf_reader_.get());
       if (width != 0) {
-        dwp_byte_reader_.reset(new ByteReader(reader_->GetEndianness()));
+        split_byte_reader = ByteReader(reader_->GetEndianness());
-        dwp_byte_reader_->SetAddressSize(width);
+        split_byte_reader.SetAddressSize(width);
-        dwp_reader_.reset(new DwpReader(*dwp_byte_reader_, elf));
+        dwp_reader_ = std::make_unique<DwpReader>(split_byte_reader,
+                                                  split_elf_reader_.get());
         dwp_reader_->Initialize();
-      } else {
+        // If we have a .dwp file, read the debug sections for the requested CU.
-        delete elf;
+        dwp_reader_->ReadDebugSectionsForCU(dwo_id_, &sections);
+        if (!sections.empty()) {
+          SectionMap::const_iterator cu_iter =
+              GetSectionByName(sections, ".debug_info_offset");
+          SectionMap::const_iterator debug_info_iter =
+              GetSectionByName(sections, ".debug_info");
+          assert(cu_iter != sections.end());
+          assert(debug_info_iter != sections.end());
+          cu_offset = cu_iter->second.first - debug_info_iter->second.first;
+          found_in_dwp = true;
+          split_file = dwp_path;
+        }
       }
     }
   }
-  bool found_in_dwp = false;
-  if (dwp_reader_) {
-    // If we have a .dwp file, read the debug sections for the requested CU.
-    SectionMap sections;
-    dwp_reader_->ReadDebugSectionsForCU(dwo_id_, &sections);
-    if (!sections.empty()) {
-      found_in_dwp = true;
-      CompilationUnit dwp_comp_unit(dwp_path_, sections, 0,
-                                    dwp_byte_reader_.get(), handler_);
-      dwp_comp_unit.SetSplitDwarf(addr_buffer_, addr_buffer_length_, addr_base_,
-                                  ranges_base_, dwo_id_);
-      dwp_comp_unit.Start();
-    }
-  }
   if (!found_in_dwp) {
     // If no .dwp file, try to open the .dwo file.
     if (stat(dwo_name_, &statbuf) == 0) {
-      ElfReader elf(dwo_name_);
+      split_elf_reader_ = std::make_unique<ElfReader>(dwo_name_);
-      int width = GetElfWidth(elf);
+      int width = GetElfWidth(*split_elf_reader_.get());
       if (width != 0) {
-        ByteReader reader(ENDIANNESS_LITTLE);
+        split_byte_reader = ByteReader(ENDIANNESS_LITTLE);
-        reader.SetAddressSize(width);
+        split_byte_reader.SetAddressSize(width);
-        SectionMap sections;
+        ReadDebugSectionsFromDwo(split_elf_reader_.get(), &sections);
-        ReadDebugSectionsFromDwo(&elf, &sections);
+        if (!sections.empty()) {
-        CompilationUnit dwo_comp_unit(dwo_name_, sections, 0, &reader,
+          split_file = dwo_name_;
-                                      handler_);
+        }
-        dwo_comp_unit.SetSplitDwarf(addr_buffer_, addr_buffer_length_,
-                                    addr_base_, ranges_base_, dwo_id_);
-        dwo_comp_unit.Start();
       }
     }
   }
+  return !split_file.empty();
 }
 
 void CompilationUnit::ReadDebugSectionsFromDwo(ElfReader* elf_reader,
@@ -1088,10 +1094,6 @@ DwpReader::DwpReader(const ByteReader& byte_reader, ElfReader* elf_reader)
       abbrev_size_(0), info_data_(NULL), info_size_(0),
       str_offsets_data_(NULL), str_offsets_size_(0) {}
 
-DwpReader::~DwpReader() {
-  if (elf_reader_) delete elf_reader_;
-}
-
 void DwpReader::Initialize() {
   cu_index_ = elf_reader_->GetSectionByName(".debug_cu_index",
                                             &cu_index_size_);
@@ -1231,8 +1233,13 @@ void DwpReader::ReadDebugSectionsForCU(uint64_t dwo_id,
       } else if (section_id == DW_SECT_INFO) {
         sections->insert(std::make_pair(
             ".debug_info",
-            std::make_pair(reinterpret_cast<const uint8_t*> (info_data_)
+            std::make_pair(reinterpret_cast<const uint8_t*>(info_data_), 0)));
-                           + offset, size)));
+        // .debug_info_offset will points the buffer for the CU with given
+        // dwo_id.
+        sections->insert(std::make_pair(
+            ".debug_info_offset",
+            std::make_pair(
+                reinterpret_cast<const uint8_t*>(info_data_) + offset, size)));
       } else if (section_id == DW_SECT_STR_OFFSETS) {
         sections->insert(std::make_pair(
             ".debug_str_offsets",

src/common/dwarf/dwarf2reader.h

@@ -481,6 +481,24 @@ class CompilationUnit {
   // start of the next compilation unit, if there is one.
   uint64_t Start();
 
+  // Process the actual debug information in a split DWARF file.
+  bool ProcessSplitDwarf(std::string& split_file,
+                         SectionMap& sections,
+                         ByteReader& split_byte_reader,
+                         uint64_t& cu_offset);
+
+  const uint8_t* GetAddrBuffer() { return addr_buffer_; }
+
+  uint64_t GetAddrBufferLen() { return addr_buffer_length_; }
+
+  uint64_t GetAddrBase() { return addr_base_; }
+
+  uint64_t GetRangeBase() { return ranges_base_; }
+
+  uint64_t GetDWOID() { return dwo_id_; }
+
+  bool ShouldProcessSplitDwarf() { return should_process_split_dwarf_; }
+
  private:
 
   // This struct represents a single DWARF2/3 abbreviation
@@ -647,9 +665,6 @@ class CompilationUnit {
   // new place to position the stream to.
   const uint8_t* SkipAttribute(const uint8_t* start, enum DwarfForm form);
 
-  // Process the actual debug information in a split DWARF file.
-  void ProcessSplitDwarf();
-
   // Read the debug sections from a .dwo file.
   void ReadDebugSectionsFromDwo(ElfReader* elf_reader,
                                 SectionMap* sections);
@@ -658,7 +673,7 @@ class CompilationUnit {
   const string path_;
 
   // Offset from section start is the offset of this compilation unit
-  // from the beginning of the .debug_info section.
+  // from the beginning of the .debug_info/.debug_info.dwo section.
   uint64_t offset_from_section_start_;
 
   // buffer is the buffer for our CU, starting at .debug_info + offset
@@ -743,14 +758,13 @@ class CompilationUnit {
   // True if we have already looked for a .dwp file.
   bool have_checked_for_dwp_;
 
-  // Path to the .dwp file.
+  // ElfReader for the dwo/dwo file.
-  string dwp_path_;
+  std::unique_ptr<ElfReader> split_elf_reader_;
-
-  // ByteReader for the DWP file.
-  std::unique_ptr<ByteReader> dwp_byte_reader_;
 
   // DWP reader.
    std::unique_ptr<DwpReader> dwp_reader_;
+
+   bool should_process_split_dwarf_;
 };
 
 // A Reader for a .dwp file.  Supports the fetching of DWARF debug
@@ -770,8 +784,6 @@ class DwpReader {
  public:
   DwpReader(const ByteReader& byte_reader, ElfReader* elf_reader);
 
-  ~DwpReader();
-
   // Read the CU index and initialize data members.
   void Initialize();
 

src/common/dwarf/dwarf2reader_die_unittest.cc

@@ -333,7 +333,8 @@ struct DwarfFormsFixture: public DIEFixture {
                             uint64_t offset=0) {
     ByteReader byte_reader(params.endianness == kLittleEndian ?
                            ENDIANNESS_LITTLE : ENDIANNESS_BIG);
-    CompilationUnit parser("", MakeSectionMap(), offset, &byte_reader, &handler);
+    CompilationUnit parser("", MakeSectionMap(), offset, &byte_reader,
+                           &handler);
     EXPECT_EQ(offset + parser.Start(), info_contents.size());
   }
 

src/common/dwarf_cu_to_module.cc

@@ -696,11 +696,12 @@ void DwarfCUToModule::InlineHandler::Finish() {
   // Every DW_TAG_inlined_subroutine should have a DW_AT_abstract_origin.
   assert(specification_offset_ != 0);
 
-  cu_context_->file_context->module_->inline_origin_map.SetReference(
+  Module::InlineOriginMap& inline_origin_map =
-      specification_offset_, specification_offset_);
+      cu_context_->file_context->module_
+          ->inline_origin_maps[cu_context_->file_context->filename_];
+  inline_origin_map.SetReference(specification_offset_, specification_offset_);
   Module::InlineOrigin* origin =
-      cu_context_->file_context->module_->inline_origin_map
+      inline_origin_map.GetOrCreateInlineOrigin(specification_offset_, name_);
-          .GetOrCreateInlineOrigin(specification_offset_, name_);
   unique_ptr<Module::Inline> in(
       new Module::Inline(origin, ranges, call_site_line_, call_site_file_id_,
                          inline_nest_level_, std::move(child_inlines_)));
@@ -929,10 +930,11 @@ void DwarfCUToModule::FuncHandler::Finish() {
     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_,
+    Module::InlineOriginMap& inline_origin_map =
-                                                                       offset);
+        cu_context_->file_context->module_
-    cu_context_->file_context->module_->inline_origin_map
+            ->inline_origin_maps[cu_context_->file_context->filename_];
-        .GetOrCreateInlineOrigin(offset_, name);
+    inline_origin_map.SetReference(offset_, offset);
+    inline_origin_map.GetOrCreateInlineOrigin(offset_, name);
   }
 }
 

src/common/linux/dump_symbols.cc

@@ -334,6 +334,40 @@ std::pair<uint8_t *, uint64_t> UncompressSectionContents(
     : std::make_pair(uncompressed_buffer.release(), uncompressed_size);
 }
 
+void StartProcessSplitDwarf(google_breakpad::CompilationUnit* reader,
+                            Module* module,
+                            google_breakpad::Endianness endianness,
+                            bool handle_inter_cu_refs,
+                            bool handle_inline) {
+  std::string split_file;
+  google_breakpad::SectionMap split_sections;
+  google_breakpad::ByteReader split_byte_reader(endianness);
+  uint64_t cu_offset = 0;
+  if (!reader->ProcessSplitDwarf(split_file, split_sections, split_byte_reader,
+                                 cu_offset))
+    return;
+  DwarfCUToModule::FileContext file_context(split_file, module,
+                                            handle_inter_cu_refs);
+  DumperRangesHandler ranges_handler(&split_byte_reader);
+  DumperLineToModule line_to_module(&split_byte_reader);
+  DwarfCUToModule::WarningReporter reporter(split_file, cu_offset);
+  DwarfCUToModule root_handler(&file_context, &line_to_module, &ranges_handler,
+                               &reporter, handle_inline);
+  google_breakpad::DIEDispatcher die_dispatcher(&root_handler);
+  google_breakpad::CompilationUnit split_reader(split_file, split_sections,
+                                                cu_offset, &split_byte_reader,
+                                                &die_dispatcher);
+  split_reader.SetSplitDwarf(reader->GetAddrBuffer(),
+                             reader->GetAddrBufferLen(), reader->GetAddrBase(),
+                             reader->GetRangeBase(), reader->GetDWOID());
+  split_reader.Start();
+  // Normally, it won't happen unless we have transitive reference.
+  if (split_reader.ShouldProcessSplitDwarf()) {
+    StartProcessSplitDwarf(&split_reader, module, endianness,
+                           handle_inter_cu_refs, handle_inline);
+  }
+}
+
 template<typename ElfClass>
 bool LoadDwarf(const string& dwarf_filename,
                const typename ElfClass::Ehdr* elf_header,
@@ -421,6 +455,11 @@ bool LoadDwarf(const string& dwarf_filename,
                                          &die_dispatcher);
     // Process the entire compilation unit; get the offset of the next.
     offset += reader.Start();
+    // Start to process split dwarf file.
+    if (reader.ShouldProcessSplitDwarf()) {
+      StartProcessSplitDwarf(&reader, module, endianness, handle_inter_cu_refs,
+                             handle_inline);
+    }
   }
   return true;
 }

src/common/mac/dump_syms.cc

@@ -424,14 +424,49 @@ bool DumpSymbols::CreateEmptyModule(scoped_ptr<Module>& module) {
   return true;
 }
 
+void DumpSymbols::StartProcessSplitDwarf(
+    google_breakpad::CompilationUnit* reader,
+    Module* module,
+    google_breakpad::Endianness endianness,
+    bool handle_inter_cu_refs,
+    bool handle_inline) const {
+  std::string split_file;
+  google_breakpad::SectionMap split_sections;
+  google_breakpad::ByteReader split_byte_reader(endianness);
+  uint64_t cu_offset = 0;
+  if (reader->ProcessSplitDwarf(split_file, split_sections, split_byte_reader,
+                                cu_offset))
+    return;
+  DwarfCUToModule::FileContext file_context(split_file, module,
+                                            handle_inter_cu_refs);
+  DumperRangesHandler ranges_handler(&split_byte_reader);
+  DumperLineToModule line_to_module(&split_byte_reader);
+  DwarfCUToModule::WarningReporter reporter(split_file, cu_offset);
+  DwarfCUToModule root_handler(&file_context, &line_to_module, &ranges_handler,
+                               &reporter, handle_inline);
+  google_breakpad::DIEDispatcher die_dispatcher(&root_handler);
+  google_breakpad::CompilationUnit split_reader(split_file, split_sections,
+                                                cu_offset, &split_byte_reader,
+                                                &die_dispatcher);
+  split_reader.SetSplitDwarf(reader->GetAddrBuffer(),
+                             reader->GetAddrBufferLen(), reader->GetAddrBase(),
+                             reader->GetRangeBase(), reader->GetDWOID());
+  split_reader.Start();
+  // Normally, it won't happen unless we have transitive reference.
+  if (split_reader.ShouldProcessSplitDwarf()) {
+    StartProcessSplitDwarf(&split_reader, module, endianness,
+                           handle_inter_cu_refs, handle_inline);
+  }
+}
+
 void DumpSymbols::ReadDwarf(google_breakpad::Module* module,
                             const mach_o::Reader& macho_reader,
                             const mach_o::SectionMap& dwarf_sections,
                             bool handle_inter_cu_refs) const {
   // Build a byte reader of the appropriate endianness.
-  ByteReader byte_reader(macho_reader.big_endian()
+  google_breakpad::Endianness endianness =
-                         ? ENDIANNESS_BIG
+      macho_reader.big_endian() ? ENDIANNESS_BIG : ENDIANNESS_LITTLE;
-                         : ENDIANNESS_LITTLE);
+  ByteReader byte_reader(endianness);
 
   // Construct a context for this file.
   DwarfCUToModule::FileContext file_context(selected_object_name_,
@@ -467,14 +502,14 @@ void DumpSymbols::ReadDwarf(google_breakpad::Module* module,
 
   // Walk the __debug_info section, one compilation unit at a time.
   uint64_t debug_info_length = debug_info_section.second;
+  bool handle_inline = symbol_data_ & INLINES;
   for (uint64_t offset = 0; offset < debug_info_length;) {
     // Make a handler for the root DIE that populates MODULE with the
     // debug info.
     DwarfCUToModule::WarningReporter reporter(selected_object_name_,
                                               offset);
     DwarfCUToModule root_handler(&file_context, &line_to_module,
-                                 &ranges_handler, &reporter,
+                                 &ranges_handler, &reporter, handle_inline);
-                                 symbol_data_ & INLINES);
     // Make a Dwarf2Handler that drives our DIEHandler.
     DIEDispatcher die_dispatcher(&root_handler);
     // Make a DWARF parser for the compilation unit at OFFSET.
@@ -485,6 +520,11 @@ void DumpSymbols::ReadDwarf(google_breakpad::Module* module,
                                                &die_dispatcher);
     // Process the entire compilation unit; get the offset of the next.
     offset += dwarf_reader.Start();
+    // Start to process split dwarf file.
+    if (dwarf_reader.ShouldProcessSplitDwarf()) {
+      StartProcessSplitDwarf(&dwarf_reader, module, endianness,
+                             handle_inter_cu_refs, handle_inline);
+    }
   }
 }
 

src/common/mac/dump_syms.h

@@ -43,6 +43,7 @@
 #include <vector>
 
 #include "common/byte_cursor.h"
+#include "common/dwarf/dwarf2reader.h"
 #include "common/mac/arch_utilities.h"
 #include "common/mac/macho_reader.h"
 #include "common/mac/super_fat_arch.h"
@@ -143,6 +144,13 @@ class DumpSymbols {
   // Creates an empty module object.
   bool CreateEmptyModule(scoped_ptr<Module>& module);
 
+  // Process the split dwarf file referenced by reader.
+  void StartProcessSplitDwarf(google_breakpad::CompilationUnit* reader,
+                              Module* module,
+                              google_breakpad::Endianness endianness,
+                              bool handle_inter_cu_refs,
+                              bool handle_inline) const;
+
   // Read debugging information from |dwarf_sections|, which was taken from
   // |macho_reader|, and add it to |module|.
   void ReadDwarf(google_breakpad::Module* module,

src/common/module.cc

@@ -289,8 +289,7 @@ void Module::GetStackFrameEntries(vector<StackFrameEntry*>* vec) const {
   }
 }
 
-void Module::AssignSourceIds(
+void Module::AssignSourceIds() {
-    set<InlineOrigin*, InlineOriginCompare>& inline_origins) {
   // First, give every source file an id of -1.
   for (FileByNameMap::iterator file_it = files_.begin();
        file_it != files_.end(); ++file_it) {
@@ -394,7 +393,7 @@ bool Module::Write(std::ostream& stream, SymbolData symbol_data) {
     // Get all referenced inline origins.
     set<InlineOrigin*, InlineOriginCompare> inline_origins;
     CreateInlineOrigins(inline_origins);
-    AssignSourceIds(inline_origins);
+    AssignSourceIds();
 
     // Write out files.
     for (FileByNameMap::iterator file_it = files_.begin();

src/common/module.h

@@ -146,10 +146,6 @@ class Module {
 
     // The inlined function's name.
     StringView name;
-
-    File* file;
-
-    int getFileID() const { return file ? file->source_id : -1; }
   };
 
   // A inlined call site.
@@ -228,7 +224,7 @@ class Module {
     map<uint64_t, uint64_t> references_;
   };
 
-  InlineOriginMap inline_origin_map;
+  map<std::string, InlineOriginMap> inline_origin_maps;
 
   // A source line.
   struct Line {
@@ -407,7 +403,7 @@ class Module {
   // Set the source id numbers for all other files --- unused by the
   // source line data --- to -1.  We do this before writing out the
   // symbol file, at which point we omit any unused files.
-  void AssignSourceIds(set<InlineOrigin*, InlineOriginCompare>& inline_origins);
+  void AssignSourceIds();
 
   // This function should be called before AssignSourceIds() to get the set of
   // valid InlineOrigins*.

src/common/module_unittest.cc

@@ -198,9 +198,7 @@ TEST(Module, WriteOmitUnusedFiles) {
   function->lines.push_back(line1);
   function->lines.push_back(line2);
   m.AddFunction(function);
-
+  m.AssignSourceIds();
-  std::set<Module::InlineOrigin*, Module::InlineOriginCompare> inline_origins;
-  m.AssignSourceIds(inline_origins);
 
   vector<Module::File*> vec;
   m.GetFiles(&vec);