From 0dd6c95b3fcf0bef0a473901e47eb4c23fb30f5b Mon Sep 17 00:00:00 2001 From: "thestig@chromium.org" Date: Tue, 13 Jul 2010 18:14:27 +0000 Subject: 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 | 21 +++++++---- src/common/module.h | 25 ++++++++---- src/common/module_unittest.cc | 69 +++++++++++++++++++++++++++++++--- src/common/stabs_to_module.cc | 19 +++------- src/common/stabs_to_module_unittest.cc | 34 ++++++++++++++++- 5 files changed, 134 insertions(+), 34 deletions(-) diff --git a/src/common/module.cc b/src/common/module.cc index f7f5788e..01f4985f 100644 --- a/src/common/module.cc +++ b/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::iterator it = functions_.begin(); + for (set::iterator it = functions_.begin(); it != functions_.end(); it++) delete *it; for (vector::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::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::iterator begin, vector::iterator end) { - functions_.insert(functions_.end(), begin, end); + for (vector::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::const_iterator func_it = functions_.begin(); + for (set::const_iterator func_it = functions_.begin(); func_it != functions_.end(); func_it++) { Function *func = *func_it; for (vector::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::const_iterator func_it = functions_.begin(); + for (set::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", @@ -217,7 +222,7 @@ bool Module::Write(FILE *stream) { || !WriteRuleMap(entry->initial_rules, stream) || 0 > putc('\n', stream)) return ReportError(); - + // Write out this entry's delta rules as 'STACK CFI' records. for (RuleChangeMap::const_iterator delta_it = entry->rule_changes.begin(); delta_it != entry->rule_changes.end(); delta_it++) { diff --git a/src/common/module.h b/src/common/module.h index 64707f3f..18351319 100644 --- a/src/common/module.h +++ b/src/common/module.h @@ -41,6 +41,7 @@ #include #include +#include #include #include @@ -48,6 +49,7 @@ namespace google_breakpad { +using std::set; using std::string; using std::vector; using std::map; @@ -90,7 +92,7 @@ class Module { // The function's name. string name; - + // The start address and length of the function's code. Address address, size; @@ -124,7 +126,7 @@ class Module { // A map from addresses to RuleMaps, representing changes that take // effect at given addresses. typedef map RuleChangeMap; - + // A range of 'STACK CFI' stack walking information. An instance of // this structure corresponds to a 'STACK CFI INIT' record and the // subsequent 'STACK CFI' records that fall within its range. @@ -143,10 +145,19 @@ class Module { // including the address you're interested in. 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, + Module(const string &name, const string &os, const string &architecture, const string &id); ~Module(); @@ -176,7 +187,7 @@ class Module { vector::iterator end); // Add STACK_FRAME_ENTRY to the module. - // + // // This module owns all StackFrameEntry objects added with this // function: destroying the module destroys them as well. void AddStackFrameEntry(StackFrameEntry *stack_frame_entry); @@ -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. - vector functions_; // This module's functions. + FileByNameMap files_; // This module's source files. + set functions_; // This module's functions. // The module owns all the call frame info entries that have been // added to it. diff --git a/src/common/module_unittest.cc b/src/common/module_unittest.cc index 18c8ad61..7ba1c17a 100644 --- a/src/common/module_unittest.cc +++ b/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" @@ -222,7 +235,7 @@ TEST(Write, OmitUnusedFiles) { m.AddFunction(function); m.AssignSourceIds(); - + vector vec; m.GetFiles(&vec); EXPECT_EQ((size_t) 3, vec.size()); @@ -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. @@ -303,7 +316,7 @@ TEST(Construct, AddFrames) { entry1->address = 0xddb5f41285aa7757ULL; entry1->size = 0x1486493370dc5073ULL; m.AddStackFrameEntry(entry1); - + // Second STACK CFI entry, with initial rules but no deltas. Module::StackFrameEntry *entry2 = new Module::StackFrameEntry(); entry2->address = 0x8064f3af5e067e38ULL; @@ -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()); +} diff --git a/src/common/stabs_to_module.cc b/src/common/stabs_to_module.cc index fbe4c02f..62fcd39e 100644 --- a/src/common/stabs_to_module.cc +++ b/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::iterator func_it = functions_.begin(); + for (vector::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 - // entry can meet this requirement. - // - // (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) - // + // of duplicated entries for functions in the STABS data. We will + // count on the Module to remove the duplicates. 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::iterator func_it = functions_.begin(); + + for (vector::const_iterator func_it = functions_.begin(); func_it != functions_.end(); func_it++) { Module::Function *f = *func_it; // Compute the function f's size. - vector::iterator boundary + vector::const_iterator boundary = std::upper_bound(boundaries_.begin(), boundaries_.end(), f->address); if (boundary != boundaries_.end()) f->size = *boundary - f->address; diff --git a/src/common/stabs_to_module_unittest.cc b/src/common/stabs_to_module_unittest.cc index 4248b3c0..2c432a3e 100644 --- a/src/common/stabs_to_module_unittest.cc +++ b/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 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); @@ -88,7 +120,7 @@ TEST(InferSizes, LineSize) { EXPECT_TRUE(h.EndFunction(0)); // unknown function end address EXPECT_TRUE(h.EndCompilationUnit(0)); // unknown CU end address EXPECT_TRUE(h.StartCompilationUnit("compilation-unit-2", 0xb4523963eff94e92LL, - "build-directory-2")); // next boundary + "build-directory-2")); // next boundary EXPECT_TRUE(h.EndCompilationUnit(0)); h.Finalize(); -- cgit v1.2.1