Put PUBLIC lines in Mac symbol files.

Exported symbols on Mach-O binaries are defined in a STABS section. This patch makes stabs_reader handle them, adds support for Extern symbols in the Module class (which are output as PUBLIC lines in symbol files), and the proper processing in stabs_to_module to hook it all up.

A=mark R=jimb at http://breakpad.appspot.com/163001 and http://breakpad.appspot.com/267001

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

9 files changed, 270 insertions, 11 deletions

diff --git a/src/common/module.cc b/src/common/module.cc
index c980b96f..0c36516a 100644
--- a/src/common/module.cc
+++ b/src/common/module.cc
@@ -55,6 +55,9 @@ Module::~Module() {
   for (vector<StackFrameEntry *>::iterator it = stack_frame_entries_.begin();
        it != stack_frame_entries_.end(); it++)
     delete *it;
+  for (ExternSet::iterator it = externs_.begin();
+       it != externs_.end(); it++)
+    delete *it;
 }
 
 void Module::SetLoadAddress(Address address) {
@@ -64,7 +67,8 @@ void Module::SetLoadAddress(Address address) {
 void Module::AddFunction(Function *function) {
   std::pair<FunctionSet::iterator,bool> ret = functions_.insert(function);
   if (!ret.second) {
-    // Free the duplicate we failed to insert because we own it.
+    // Free the duplicate that was not inserted because this Module
+    // now owns it.
     delete function;
   }
 }
@@ -79,11 +83,25 @@ void Module::AddStackFrameEntry(StackFrameEntry *stack_frame_entry) {
   stack_frame_entries_.push_back(stack_frame_entry);
 }
 
+void Module::AddExtern(Extern *ext) {
+  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;
+  }
+}
+
 void Module::GetFunctions(vector<Function *> *vec,
                           vector<Function *>::iterator i) {
   vec->insert(i, functions_.begin(), functions_.end());
 }
 
+void Module::GetExterns(vector<Extern *> *vec,
+                        vector<Extern *>::iterator i) {
+  vec->insert(i, externs_.begin(), externs_.end());
+}
+
 Module::File *Module::FindFile(const string &name) {
   // A tricky bit here.  The key of each map entry needs to be a
   // pointer to the entry's File's name string.  This means that we
@@ -211,6 +229,16 @@ bool Module::Write(FILE *stream) {
         return ReportError();
   }
 
+  // Write out 'PUBLIC' records.
+  for (ExternSet::const_iterator extern_it = externs_.begin();
+       extern_it != externs_.end(); extern_it++) {
+    Extern *ext = *extern_it;
+    if (0 > fprintf(stream, "PUBLIC %llx 0 %s\n",
+                    (unsigned long long) (ext->address - load_address_),
+                    ext->name.c_str()))
+      return ReportError();
+  }
+
   // Write out 'STACK CFI INIT' and 'STACK CFI' records.
   vector<StackFrameEntry *>::const_iterator frame_it;
   for (frame_it = stack_frame_entries_.begin();
diff --git a/src/common/module.h b/src/common/module.h
index 8c20cea0..83e8403c 100644
--- a/src/common/module.h
+++ b/src/common/module.h
@@ -65,6 +65,7 @@ class Module {
   struct File;
   struct Function;
   struct Line;
+  struct Extern;
 
   // Addresses appearing in File, Function, and Line structures are
   // absolute, not relative to the the module's load address.  That
@@ -117,6 +118,12 @@ class Module {
     int number;                // The source line number.
   };
 
+  // An exported symbol.
+  struct Extern {
+    Address address;
+    string name;
+  };
+
   // A map from register names to postfix expressions that recover
   // their their values. This can represent a complete set of rules to
   // follow at some address, or a set of changes to be applied to an
@@ -155,6 +162,13 @@ class Module {
     }
   };
 
+  struct ExternCompare {
+    bool operator() (const Extern *lhs,
+                     const Extern *rhs) const {
+      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,
@@ -187,11 +201,15 @@ class Module {
                     vector<Function *>::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);
 
+  // Add PUBLIC to the module.
+  // This module owns all Extern objects added with this function:
+  // destroying the module destroys them as well.
+  void AddExtern(Extern *ext);
+
   // If this module has a file named NAME, return a pointer to it. If
   // it has none, then create one and return a pointer to the new
   // file. This module owns all File objects created using these
@@ -210,6 +228,13 @@ class Module {
   // appropriate interface.)
   void GetFunctions(vector<Function *> *vec, vector<Function *>::iterator i);
 
+  // Insert pointers to the externs added to this module at I in
+  // VEC. The pointed-to Externs are still owned by this module.
+  // (Since this is effectively a copy of the extern list, this is
+  // mostly useful for testing; other uses should probably get a more
+  // appropriate interface.)
+  void GetExterns(vector<Extern *> *vec, vector<Extern *>::iterator i);
+
   // Clear VEC and fill it with pointers to the Files added to this
   // module, sorted by name. The pointed-to Files are still owned by
   // this module. (Since this is effectively a copy of the file list,
@@ -269,8 +294,13 @@ class Module {
   // A map from filenames to File structures.  The map's keys are
   // pointers to the Files' names.
   typedef map<const string *, File *, CompareStringPtrs> FileByNameMap;
+
+  // A set containing Function structures, sorted by address.
   typedef set<Function *, FunctionCompare> FunctionSet;
 
+  // A set containing Extern structures, sorted by address.
+  typedef set<Extern *, ExternCompare> ExternSet;
+
   // 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.
@@ -280,6 +310,10 @@ class Module {
   // The module owns all the call frame info entries that have been
   // added to it.
   vector<StackFrameEntry *> stack_frame_entries_;
+
+  // The module owns all the externs that have been added to it;
+  // destroying the module frees the Externs these point to.
+  ExternSet externs_;
 };
 
 } // namespace google_breakpad
diff --git a/src/common/module_unittest.cc b/src/common/module_unittest.cc
index 24189944..63ad5056 100644
--- a/src/common/module_unittest.cc
+++ b/src/common/module_unittest.cc
@@ -455,3 +455,62 @@ TEST(Construct, FunctionsWithSameAddress) {
                " _without_form\n",
                contents.c_str());
 }
+
+// Externs should be written out as PUBLIC records, sorted by
+// address.
+TEST(Construct, Externs) {
+  FILE *f = checked_tmpfile();
+  Module m(MODULE_NAME, MODULE_OS, MODULE_ARCH, MODULE_ID);
+
+  // Two externs.
+  Module::Extern *extern1 = new(Module::Extern);
+  extern1->address = 0xffff;
+  extern1->name = "_abc";
+  Module::Extern *extern2 = new(Module::Extern);
+  extern2->address = 0xaaaa;
+  extern2->name = "_xyz";
+
+  m.AddExtern(extern1);
+  m.AddExtern(extern2);
+
+  m.Write(f);
+  checked_fflush(f);
+  rewind(f);
+  string contents = checked_read(f);
+  checked_fclose(f);
+
+  EXPECT_STREQ("MODULE " MODULE_OS " " MODULE_ARCH " "
+               MODULE_ID " " MODULE_NAME "\n"
+               "PUBLIC aaaa 0 _xyz\n"
+               "PUBLIC ffff 0 _abc\n",
+               contents.c_str());
+}
+
+// Externs with the same address should only keep the first entry
+// added.
+TEST(Construct, DuplicateExterns) {
+  FILE *f = checked_tmpfile();
+  Module m(MODULE_NAME, MODULE_OS, MODULE_ARCH, MODULE_ID);
+
+  // Two externs.
+  Module::Extern *extern1 = new(Module::Extern);
+  extern1->address = 0xffff;
+  extern1->name = "_xyz";
+  Module::Extern *extern2 = new(Module::Extern);
+  extern2->address = 0xffff;
+  extern2->name = "_abc";
+
+  m.AddExtern(extern1);
+  m.AddExtern(extern2);
+
+  m.Write(f);
+  checked_fflush(f);
+  rewind(f);
+  string contents = checked_read(f);
+  checked_fclose(f);
+
+  EXPECT_STREQ("MODULE " MODULE_OS " " MODULE_ARCH " "
+               MODULE_ID " " MODULE_NAME "\n"
+               "PUBLIC ffff 0 _xyz\n",
+               contents.c_str());
+}
diff --git a/src/common/stabs_reader.cc b/src/common/stabs_reader.cc
index 1ca97412..7dd2eecd 100644
--- a/src/common/stabs_reader.cc
+++ b/src/common/stabs_reader.cc
@@ -107,8 +107,19 @@ bool StabsReader::Process() {
       string_offset_ = next_cu_string_offset_;
       next_cu_string_offset_ = iterator_->value;
       ++iterator_;
-    } else
+    }
+#if defined(HAVE_MACH_O_NLIST_H)
+    // Export symbols in Mach-O binaries look like this.
+    // This is necessary in order to be able to dump symbols
+    // from OS X system libraries.
+    else if ((iterator_->type & N_STAB) == 0 &&
+               (iterator_->type & N_TYPE) == N_SECT) {
+      ProcessExtern();
+    }
+#endif
+    else {
       ++iterator_;
+    }
   }
   return true;
 }
@@ -282,4 +293,19 @@ bool StabsReader::ProcessFunction() {
   return true;
 }
 
+bool StabsReader::ProcessExtern() {
+#if defined(HAVE_MACH_O_NLIST_H)
+  assert(!iterator_->at_end &&
+         (iterator_->type & N_STAB) == 0 &&
+         (iterator_->type & N_TYPE) == N_SECT);
+#endif
+
+  // TODO(mark): only do symbols in the text section?
+  if (!handler_->Extern(SymbolString(), iterator_->value))
+    return false;
+
+  ++iterator_;
+  return true;
+}
+
 } // namespace google_breakpad
diff --git a/src/common/stabs_reader.h b/src/common/stabs_reader.h
index c17fcc7a..b22ebfd3 100644
--- a/src/common/stabs_reader.h
+++ b/src/common/stabs_reader.h
@@ -193,7 +193,11 @@ class StabsReader {
   // Return true to continue processing, or false to abort.
   bool ProcessFunction();
 
-  // The STABS entries we're parsing.
+  // Process an exported function symbol.
+  // Return true to continue processing, or false to abort.
+  bool ProcessExtern();
+
+  // The STABS entries being parsed.
   ByteBuffer entries_;
 
   // The string section to which the entries refer.
@@ -305,6 +309,12 @@ class StabsHandler {
     return true;
   }
 
+  // Report that an exported function NAME is present at ADDRESS.
+  // The size of the function is unknown.
+  virtual bool Extern(const std::string &name, uint64_t address) {
+    return true;
+  }
+
   // Report a warning.  FORMAT is a printf-like format string,
   // specifying how to format the subsequent arguments.
   virtual void Warning(const char *format, ...) = 0;
diff --git a/src/common/stabs_reader_unittest.cc b/src/common/stabs_reader_unittest.cc
index 9461ac0c..2a9b5d41 100644
--- a/src/common/stabs_reader_unittest.cc
+++ b/src/common/stabs_reader_unittest.cc
@@ -221,6 +221,7 @@ class MockStabsReaderHandler: public StabsHandler {
   MOCK_METHOD2(StartFunction, bool(const std::string &, uint64_t));
   MOCK_METHOD1(EndFunction, bool(uint64_t));
   MOCK_METHOD3(Line, bool(uint64_t, const char *, int));
+  MOCK_METHOD2(Extern, bool(const std::string &, uint64_t));
   void Warning(const char *format, ...) { MockWarning(format); }
   MOCK_METHOD1(MockWarning, void(const char *));
 };
@@ -555,6 +556,55 @@ TEST_F(Stabs, LeadingLine) {
   ASSERT_TRUE(ApplyHandlerToMockStabsData());
 }
 
-// name duplication
+
+#if defined(HAVE_MACH_O_NLIST_H)
+// These tests have no meaning on non-Mach-O-based systems, as
+// only Mach-O uses N_SECT to represent public symbols.
+TEST_F(Stabs, OnePublicSymbol) {
+  stabs.set_endianness(kLittleEndian);
+  stabs.set_value_size(4);
+
+  const u_int32_t kExpectedAddress = 0x9000;
+  const string kExpectedFunctionName("public_function");
+  stabs
+    .Stab(N_SECT, 1, 0, kExpectedAddress, kExpectedFunctionName);
+
+  {
+    InSequence s;
+    EXPECT_CALL(mock_handler,
+                Extern(StrEq(kExpectedFunctionName),
+                       kExpectedAddress))
+        .WillOnce(Return(true));
+  }
+  ASSERT_TRUE(ApplyHandlerToMockStabsData());
+}
+
+TEST_F(Stabs, TwoPublicSymbols) {
+  stabs.set_endianness(kLittleEndian);
+  stabs.set_value_size(4);
+
+  const u_int32_t kExpectedAddress1 = 0xB0B0B0B0;
+  const string kExpectedFunctionName1("public_function");
+  const u_int32_t kExpectedAddress2 = 0xF0F0F0F0;
+  const string kExpectedFunctionName2("something else");
+  stabs
+    .Stab(N_SECT, 1, 0, kExpectedAddress1, kExpectedFunctionName1)
+    .Stab(N_SECT, 1, 0, kExpectedAddress2, kExpectedFunctionName2);
+
+  {
+    InSequence s;
+    EXPECT_CALL(mock_handler,
+                Extern(StrEq(kExpectedFunctionName1),
+                       kExpectedAddress1))
+        .WillOnce(Return(true));
+    EXPECT_CALL(mock_handler,
+                Extern(StrEq(kExpectedFunctionName2),
+                       kExpectedAddress2))
+        .WillOnce(Return(true));
+  }
+  ASSERT_TRUE(ApplyHandlerToMockStabsData());
+}
+
+#endif
 
 } // anonymous namespace
diff --git a/src/common/stabs_to_module.cc b/src/common/stabs_to_module.cc
index 62fcd39e..e694febc 100644
--- a/src/common/stabs_to_module.cc
+++ b/src/common/stabs_to_module.cc
@@ -132,6 +132,22 @@ bool StabsToModule::Line(uint64_t address, const char *name, int number) {
   return true;
 }
 
+bool StabsToModule::Extern(const string &name, uint64_t address) {
+  Module::Extern *ext = new Module::Extern;
+  // Older libstdc++ demangle implementations can crash on unexpected
+  // input, so be careful about what gets passed in.
+  if (name.compare(0, 3, "__Z") == 0) {
+    ext->name = Demangle(name.substr(1));
+  } else if (name[0] == '_') {
+    ext->name = name.substr(1);
+  } else {
+    ext->name = name;
+  }
+  ext->address = address;
+  module_->AddExtern(ext);
+  return true;
+}
+
 void StabsToModule::Warning(const char *format, ...) {
   va_list args;
   va_start(args, format);
diff --git a/src/common/stabs_to_module.h b/src/common/stabs_to_module.h
index 6538d78d..632f4d00 100644
--- a/src/common/stabs_to_module.h
+++ b/src/common/stabs_to_module.h
@@ -35,8 +35,8 @@
 // STABS debugging information from a parser and adds it to a Breakpad
 // symbol file.
 
-#ifndef COMMON_LINUX_DUMP_STABS_H__
-#define COMMON_LINUX_DUMP_STABS_H__
+#ifndef BREAKPAD_COMMON_STABS_TO_MODULE_H_
+#define BREAKPAD_COMMON_STABS_TO_MODULE_H_
 
 #include <stdint.h>
 
@@ -51,12 +51,14 @@ namespace google_breakpad {
 using std::string;
 using std::vector;
 
-// A StabsToModule is a handler that receives parsed STABS
-// debugging information from a StabsReader, and uses that to populate
+// A StabsToModule is a handler that receives parsed STABS debugging 
+// information from a StabsReader, and uses that to populate
 // a Module. (All classes are in the google_breakpad namespace.) A
 // Module represents the contents of a Breakpad symbol file, and knows
 // how to write itself out as such. A StabsToModule thus acts as
 // the bridge between STABS and Breakpad data.
+// When processing Darwin Mach-O files, this also receives public linker
+// symbols, like those found in system libraries.
 class StabsToModule: public google_breakpad::StabsHandler {
  public:
   // Receive parsed debugging information from a StabsReader, and
@@ -77,6 +79,7 @@ class StabsToModule: public google_breakpad::StabsHandler {
   bool StartFunction(const string &name, uint64_t address);
   bool EndFunction(uint64_t address);
   bool Line(uint64_t address, const char *name, int number);
+  bool Extern(const string &name, uint64_t address);
   void Warning(const char *format, ...);
 
   // Do any final processing necessary to make module_ contain all the
@@ -135,6 +138,6 @@ class StabsToModule: public google_breakpad::StabsHandler {
   const char *current_source_file_name_;
 };
 
-} // namespace google_breakpad
+}  // namespace google_breakpad
 
-#endif // COMMON_LINUX_DUMP_STABS_H__
+#endif  // BREAKPAD_COMMON_STABS_TO_MODULE_H_
diff --git a/src/common/stabs_to_module_unittest.cc b/src/common/stabs_to_module_unittest.cc
index 2c432a3e..d445d1d6 100644
--- a/src/common/stabs_to_module_unittest.cc
+++ b/src/common/stabs_to_module_unittest.cc
@@ -74,6 +74,35 @@ TEST(StabsToModule, SimpleCU) {
   EXPECT_EQ(174823314, line->number);
 }
 
+#ifdef __GNUC__
+// Function name mangling can vary by compiler, so only run mangled-name
+// tests on GCC for simplicity's sake.
+TEST(StabsToModule, Externs) {
+  Module m("name", "os", "arch", "id");
+  StabsToModule h(&m);
+
+  // Feed in a few Extern symbols.
+  EXPECT_TRUE(h.Extern("_foo", 0xffff));
+  EXPECT_TRUE(h.Extern("__Z21dyldGlobalLockAcquirev", 0xaaaa));
+  EXPECT_TRUE(h.Extern("_MorphTableGetNextMorphChain", 0x1111));
+  h.Finalize();
+
+  // Now check to see what has been added to the Module.
+  vector<Module::Extern *> externs;
+  m.GetExterns(&externs, externs.end());
+  ASSERT_EQ((size_t) 3, externs.size());
+  Module::Extern *extern1 = externs[0];
+  EXPECT_STREQ("MorphTableGetNextMorphChain", extern1->name.c_str());
+  EXPECT_EQ((Module::Address)0x1111, extern1->address);
+  Module::Extern *extern2 = externs[1];
+  EXPECT_STREQ("dyldGlobalLockAcquire()", extern2->name.c_str());
+  EXPECT_EQ((Module::Address)0xaaaa, extern2->address);
+  Module::Extern *extern3 = externs[2];
+  EXPECT_STREQ("foo", extern3->name.c_str());
+  EXPECT_EQ((Module::Address)0xffff, extern3->address);
+}
+#endif  // __GNUC__
+
 TEST(StabsToModule, DuplicateFunctionNames) {
   Module m("name", "os", "arch", "id");
   StabsToModule h(&m);
@@ -154,6 +183,9 @@ TEST(InferSizes, LineSize) {
   EXPECT_EQ(87660088, line2->number);
 }
 
+#ifdef __GNUC__
+// Function name mangling can vary by compiler, so only run mangled-name
+// tests on GCC for simplicity's sake.
 TEST(FunctionNames, Mangled) {
   Module m("name", "os", "arch", "id");
   StabsToModule h(&m);
@@ -188,6 +220,7 @@ TEST(FunctionNames, Mangled) {
   EXPECT_EQ(0U, function->parameter_size);
   ASSERT_EQ(0U, function->lines.size());
 }
+#endif  // __GNUC__
 
 // The GNU toolchain can omit functions that are not used; however,
 // when it does so, it doesn't clean up the debugging information that