Remove duplicate FUNC entries from dump_syms output.

Review URL: http://breakpad.appspot.com/128001

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

src/common/module.cc

@@ -49,7 +49,7 @@ Module::Module(const string &name, const string &os,
 Module::~Module() {
   for (FileByNameMap::iterator it = files_.begin(); it != files_.end(); it++)
     delete it->second;
-  for (vector<Function *>::iterator it = functions_.begin();
+  for (set<Function *>::iterator it = functions_.begin();
        it != functions_.end(); it++)
     delete *it;
   for (vector<StackFrameEntry *>::iterator it = stack_frame_entries_.begin();
@@ -62,12 +62,17 @@ void Module::SetLoadAddress(Address address) {
 }
 
 void Module::AddFunction(Function *function) {
-  functions_.push_back(function);
+  std::pair<set<Function *>::iterator,bool> ret = functions_.insert(function);
+  if (!ret.second) {
+    // Free the duplicate we failed to insert because we own it.
+    delete function;
+  }
 }
 
 void Module::AddFunctions(vector<Function *>::iterator begin,
                           vector<Function *>::iterator end) {
-  functions_.insert(functions_.end(), begin, end);
+  for (vector<Function *>::iterator it = begin; it != end; it++)
+    AddFunction(*it);
 }
 
 void Module::AddStackFrameEntry(StackFrameEntry *stack_frame_entry) {
@@ -130,7 +135,7 @@ void Module::AssignSourceIds() {
 
   // Next, mark all files actually cited by our functions' line number
   // info, by setting each one's source id to zero.
-  for (vector<Function *>::const_iterator func_it = functions_.begin();
+  for (set<Function *>::const_iterator func_it = functions_.begin();
        func_it != functions_.end(); func_it++) {
     Function *func = *func_it;
     for (vector<Line>::iterator line_it = func->lines.begin();
@@ -145,13 +150,13 @@ void Module::AssignSourceIds() {
   int next_source_id = 0;
   for (FileByNameMap::iterator file_it = files_.begin();
        file_it != files_.end(); file_it++)
-    if (! file_it->second->source_id)
+    if (!file_it->second->source_id)
       file_it->second->source_id = next_source_id++;
 }
 
 bool Module::ReportError() {
   fprintf(stderr, "error writing symbol file: %s\n",
-          strerror (errno));
+          strerror(errno));
   return false;
 }
 
@@ -187,7 +192,7 @@ bool Module::Write(FILE *stream) {
   }
 
   // Write out functions and their lines.
-  for (vector<Function *>::const_iterator func_it = functions_.begin();
+  for (set<Function *>::const_iterator func_it = functions_.begin();
        func_it != functions_.end(); func_it++) {
     Function *func = *func_it;
     if (0 > fprintf(stream, "FUNC %llx %llx %llx %s\n",

src/common/module.h

@@ -41,6 +41,7 @@
 #include <stdio.h>
 
 #include <map>
+#include <set>
 #include <string>
 #include <vector>
 
@@ -48,6 +49,7 @@
 
 namespace google_breakpad {
 
+using std::set;
 using std::string;
 using std::vector;
 using std::map;
@@ -144,6 +146,15 @@ class Module {
     RuleChangeMap rule_changes;
   };
 
+  struct FunctionCompare {
+    bool operator() (const Function *lhs,
+                     const Function *rhs) const {
+      if (lhs->address == rhs->address)
+        return lhs->name < rhs->name;
+      return lhs->address < rhs->address;
+    }
+  };
+
   // Create a new module with the given name, operating system,
   // architecture, and ID string.
   Module(const string &name, const string &os, const string &architecture,
@@ -262,8 +273,8 @@ class Module {
   // The module owns all the files and functions that have been added
   // to it; destroying the module frees the Files and Functions these
   // point to.
-  FileByNameMap files_;                 // This module's source files.  
+  FileByNameMap files_;                         // This module's source files.
-  vector<Function *> functions_;        // This module's functions.
+  set<Function *, FunctionCompare> functions_;  // This module's functions.
 
   // The module owns all the call frame info entries that have been
   // added to it.

src/common/module_unittest.cc

@@ -91,6 +91,19 @@ void checked_fclose(FILE *stream) {
   }
 }
 
+Module::Function *generate_duplicate_function(const string &name) {
+  const Module::Address DUP_ADDRESS = 0xd35402aac7a7ad5cLL;
+  const Module::Address DUP_SIZE = 0x200b26e605f99071LL;
+  const Module::Address DUP_PARAMETER_SIZE = 0xf14ac4fed48c4a99LL;
+
+  Module::Function *function = new(Module::Function);
+  function->name = name;
+  function->address = DUP_ADDRESS;
+  function->size = DUP_SIZE;
+  function->parameter_size = DUP_PARAMETER_SIZE;
+  return function;
+}
+
 #define MODULE_NAME "name with spaces"
 #define MODULE_OS "os-name"
 #define MODULE_ARCH "architecture"
@@ -280,10 +293,10 @@ TEST(Construct, AddFunctions) {
   string contents = checked_read(f);
   checked_fclose(f);
   EXPECT_STREQ("MODULE os-name architecture id-string name with spaces\n"
-               "FUNC d35024aa7ca7da5c 200b26e605f99071 f14ac4fed48c4a99"
-               " _without_form\n"
                "FUNC 2987743d0b35b13f b369db048deb3010 938e556cb5a79988"
-               " _and_void\n",
+               " _and_void\n"
+               "FUNC d35024aa7ca7da5c 200b26e605f99071 f14ac4fed48c4a99"
+               " _without_form\n",
                contents.c_str());
 
   // Check that m.GetFunctions returns the functions we expect.
@@ -396,3 +409,49 @@ TEST(Construct, UniqueFiles) {
   EXPECT_EQ(file1, m.FindExistingFile("foo"));
   EXPECT_TRUE(m.FindExistingFile("baz") == NULL);
 }
+
+TEST(Construct, DuplicateFunctions) {
+  FILE *f = checked_tmpfile();
+  Module m(MODULE_NAME, MODULE_OS, MODULE_ARCH, MODULE_ID);
+
+  // Two functions.
+  Module::Function *function1 = generate_duplicate_function("_without_form");
+  Module::Function *function2 = generate_duplicate_function("_without_form");
+
+  m.AddFunction(function1);
+  m.AddFunction(function2);
+
+  m.Write(f);
+  checked_fflush(f);
+  rewind(f);
+  string contents = checked_read(f);
+  checked_fclose(f);
+  EXPECT_STREQ("MODULE os-name architecture id-string name with spaces\n"
+               "FUNC d35402aac7a7ad5c 200b26e605f99071 f14ac4fed48c4a99"
+               " _without_form\n",
+               contents.c_str());
+}
+
+TEST(Construct, FunctionsWithSameAddress) {
+  FILE *f = checked_tmpfile();
+  Module m(MODULE_NAME, MODULE_OS, MODULE_ARCH, MODULE_ID);
+
+  // Two functions.
+  Module::Function *function1 = generate_duplicate_function("_without_form");
+  Module::Function *function2 = generate_duplicate_function("_and_void");
+
+  m.AddFunction(function1);
+  m.AddFunction(function2);
+
+  m.Write(f);
+  checked_fflush(f);
+  rewind(f);
+  string contents = checked_read(f);
+  checked_fclose(f);
+  EXPECT_STREQ("MODULE os-name architecture id-string name with spaces\n"
+               "FUNC d35402aac7a7ad5c 200b26e605f99071 f14ac4fed48c4a99"
+               " _and_void\n"
+               "FUNC d35402aac7a7ad5c 200b26e605f99071 f14ac4fed48c4a99"
+               " _without_form\n",
+               contents.c_str());
+}

src/common/stabs_to_module.cc

@@ -58,7 +58,7 @@ static string Demangle(const string &mangled) {
 
 StabsToModule::~StabsToModule() {
   // Free any functions we've accumulated but not added to the module.
-  for (vector<Module::Function *>::iterator func_it = functions_.begin();
+  for (vector<Module::Function *>::const_iterator func_it = functions_.begin();
        func_it != functions_.end(); func_it++)
     delete *func_it;
   // Free any function that we're currently within.
@@ -104,16 +104,8 @@ bool StabsToModule::EndFunction(uint64_t address) {
   assert(current_function_);
   // Functions in this compilation unit should have address bigger
   // than the compilation unit's starting address.  There may be a lot
-  // of duplicated entries for functions in the STABS data; only one
+  // of duplicated entries for functions in the STABS data. We will
-  // entry can meet this requirement.
+  // count on the Module to remove the duplicates.
-  //
-  // (I don't really understand the above comment; just bringing it along
-  // from the previous code, and leaving the behavior unchanged. GCC marks
-  // the end of each function with an N_FUN entry with no name, whose value
-  // is the size of the function; perhaps this test was concerned with
-  // skipping those. Now StabsReader interprets them properly. If you know
-  // the whole story, please patch this comment. --jimb)
-  //
   if (current_function_->address >= comp_unit_base_address_)
     functions_.push_back(current_function_);
   else
@@ -153,12 +145,13 @@ void StabsToModule::Finalize() {
   // Sort all functions by address, just for neatness.
   sort(functions_.begin(), functions_.end(),
        Module::Function::CompareByAddress);
-  for (vector<Module::Function *>::iterator func_it = functions_.begin();
+
+  for (vector<Module::Function *>::const_iterator func_it = functions_.begin();
        func_it != functions_.end();
        func_it++) {
     Module::Function *f = *func_it;
     // Compute the function f's size.
-    vector<Module::Address>::iterator boundary
+    vector<Module::Address>::const_iterator boundary
         = std::upper_bound(boundaries_.begin(), boundaries_.end(), f->address);
     if (boundary != boundaries_.end())
       f->size = *boundary - f->address;

src/common/stabs_to_module_unittest.cc

@@ -74,6 +74,38 @@ TEST(StabsToModule, SimpleCU) {
   EXPECT_EQ(174823314, line->number);
 }
 
+TEST(StabsToModule, DuplicateFunctionNames) {
+  Module m("name", "os", "arch", "id");
+  StabsToModule h(&m);
+
+  // Compilation unit with one function, mangled name.
+  EXPECT_TRUE(h.StartCompilationUnit("compilation-unit", 0xf2cfda36ecf7f46cLL,
+                                     "build-directory"));
+  EXPECT_TRUE(h.StartFunction("funcfoo",
+                              0xf2cfda36ecf7f46dLL));
+  EXPECT_TRUE(h.EndFunction(0));
+  EXPECT_TRUE(h.StartFunction("funcfoo",
+                              0xf2cfda36ecf7f46dLL));
+  EXPECT_TRUE(h.EndFunction(0));
+  EXPECT_TRUE(h.EndCompilationUnit(0));
+
+  h.Finalize();
+
+  // Now check to see what has been added to the Module.
+  Module::File *file = m.FindExistingFile("compilation-unit");
+  ASSERT_TRUE(file != NULL);
+
+  vector<Module::Function *> functions;
+  m.GetFunctions(&functions, functions.end());
+  ASSERT_EQ(1U, functions.size());
+
+  Module::Function *function = functions[0];
+  EXPECT_EQ(0xf2cfda36ecf7f46dLL, function->address);
+  EXPECT_LT(0U, function->size);  // should have used dummy size
+  EXPECT_EQ(0U, function->parameter_size);
+  ASSERT_EQ(0U, function->lines.size());
+}
+
 TEST(InferSizes, LineSize) {
   Module m("name", "os", "arch", "id");
   StabsToModule h(&m);