Follow debug link correctly

As thestig@chromium.org pointed out in https://breakpad.appspot.com/9684002,
LoadSymbols() should return false if |read_gnu_debug_link| is false.

BUG=chromium:453498
R=thestig@chromium.org

Review URL: https://breakpad.appspot.com/2844002

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

2 files changed, 52 insertions, 50 deletions

diff --git a/src/common/linux/dump_symbols.cc b/src/common/linux/dump_symbols.cc
index 5a74d805..eb7f1820 100644
--- a/src/common/linux/dump_symbols.cc
+++ b/src/common/linux/dump_symbols.cc
@@ -628,6 +628,35 @@ bool LoadSymbols(const string& obj_file,
                 "DWARF debugging information\n", obj_file.c_str());
       }
     }
+
+    // See if there are export symbols available.
+    const Shdr* dynsym_section =
+      FindElfSectionByName<ElfClass>(".dynsym", SHT_DYNSYM,
+                                     sections, names, names_end,
+                                     elf_header->e_shnum);
+    const Shdr* dynstr_section =
+      FindElfSectionByName<ElfClass>(".dynstr", SHT_STRTAB,
+                                     sections, names, names_end,
+                                     elf_header->e_shnum);
+    if (dynsym_section && dynstr_section) {
+      info->LoadedSection(".dynsym");
+
+      const uint8_t* dynsyms =
+          GetOffset<ElfClass, uint8_t>(elf_header,
+                                       dynsym_section->sh_offset);
+      const uint8_t* dynstrs =
+          GetOffset<ElfClass, uint8_t>(elf_header,
+                                       dynstr_section->sh_offset);
+      bool result =
+          ELFSymbolsToModule(dynsyms,
+                             dynsym_section->sh_size,
+                             dynstrs,
+                             dynstr_section->sh_size,
+                             big_endian,
+                             ElfClass::kAddrSize,
+                             module);
+      found_usable_info = found_usable_info || result;
+    }
   }
 
   if (options.symbol_data != NO_CFI) {
@@ -689,8 +718,6 @@ bool LoadSymbols(const string& obj_file,
                                            names_end, elf_header->e_shnum);
       if (gnu_debuglink_section) {
         if (!info->debug_dirs().empty()) {
-          found_debug_info_section = true;
-
           const char* debuglink_contents =
               GetOffset<ElfClass, char>(elf_header,
                                         gnu_debuglink_section->sh_offset);
@@ -709,45 +736,18 @@ bool LoadSymbols(const string& obj_file,
         fprintf(stderr, "%s does not contain a .gnu_debuglink section.\n",
                 obj_file.c_str());
       }
+    } else {
+      // Return true if some usable information was found, since the caller
+      // doesn't want to use .gnu_debuglink.
+      return found_usable_info;
     }
-  }
-
-  if (options.symbol_data != ONLY_CFI) {
-    const Shdr* dynsym_section =
-      FindElfSectionByName<ElfClass>(".dynsym", SHT_DYNSYM,
-                                     sections, names, names_end,
-                                     elf_header->e_shnum);
-    const Shdr* dynstr_section =
-      FindElfSectionByName<ElfClass>(".dynstr", SHT_STRTAB,
-                                     sections, names, names_end,
-                                     elf_header->e_shnum);
-    if (dynsym_section && dynstr_section) {
-      info->LoadedSection(".dynsym");
-
-      const uint8_t* dynsyms =
-          GetOffset<ElfClass, uint8_t>(elf_header,
-                                       dynsym_section->sh_offset);
-      const uint8_t* dynstrs =
-          GetOffset<ElfClass, uint8_t>(elf_header,
-                                       dynstr_section->sh_offset);
-      bool result =
-          ELFSymbolsToModule(dynsyms,
-                             dynsym_section->sh_size,
-                             dynstrs,
-                             dynstr_section->sh_size,
-                             big_endian,
-                             ElfClass::kAddrSize,
-                             module);
-      found_usable_info = found_usable_info || result;
-    }
-  }
 
-  if (read_gnu_debug_link) {
-    return found_debug_info_section;
+    // No debug info was found, let the user try again with .gnu_debuglink
+    // if present.
+    return false;
   }
 
-  // Return true if some usable information was found
-  return found_usable_info;
+  return true;
 }
 
 // Return the breakpad symbol file identifier for the architecture of
diff --git a/src/common/module.cc b/src/common/module.cc
index f682e24a..d880ce57 100644
--- a/src/common/module.cc
+++ b/src/common/module.cc
@@ -79,6 +79,17 @@ void Module::AddFunction(Function *function) {
   // FUNC lines must not hold an empty name, so catch the problem early if
   // callers try to add one.
   assert(!function->name.empty());
+
+  // FUNCs are better than PUBLICs as they come with sizes, so remove an extern
+  // with the same address if present.
+  Extern ext(function->address);
+  ExternSet::iterator it_ext = externs_.lower_bound(&ext);
+  if (it_ext != externs_.end() &&
+      (*it_ext)->address < function->address + function->size) {
+    delete *it_ext;
+    externs_.erase(it_ext);
+  }
+
   std::pair<FunctionSet::iterator,bool> ret = functions_.insert(function);
   if (!ret.second && (*ret.first != function)) {
     // Free the duplicate that was not inserted because this Module
@@ -98,19 +109,10 @@ void Module::AddStackFrameEntry(StackFrameEntry *stack_frame_entry) {
 }
 
 void Module::AddExtern(Extern *ext) {
-  Function func(ext->name, ext->address);
-
-  // Since parsing debug section and public info are not necessarily
-  // mutually exclusive, check if the symbol has already been read
-  // as a function to avoid duplicates.
-  if (functions_.find(&func) == functions_.end()) {
-    std::pair<ExternSet::iterator,bool> ret = externs_.insert(ext);
-    if (!ret.second) {
-      // Free the duplicate that was not inserted because this Module
-      // now owns it.
-      delete ext;
-    }
-  } else {
+  std::pair<ExternSet::iterator,bool> ret = externs_.insert(ext);
+  if (!ret.second) {
+    // Free the duplicate that was not inserted because this Module
+    // now owns it.
     delete ext;
   }
 }