Symbol file should contain module GUID at beginning (#66). r=bryner

- The dumped symbol format now begins with a MODULE line identifying the
   uuid, age, and name of the source pdb file.
 - The processor ignores MODULE lines, but they are useful in figuring out
   how to index symbol files in a symbol store.
 - dump_syms and symupload now both accept either a pdb or exe/dll and
   will read the pdb regardless.
 - Figured out that MSSS always represents a module's age in pathnames in
   hexadecimal, and updated SimpleSymbolSupplier to match.

http://groups.google.com/group/airbag-dev/browse_thread/thread/572108d6567edd58


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

src/common/windows/pdb_source_line_writer.cc

@@ -77,6 +77,14 @@ bool PDBSourceLineWriter::Open(const wstring &file, FileFormat format) {
         return false;
       }
       break;
+    case ANY_FILE:
+      if (FAILED(data_source->loadDataFromPdb(file.c_str()))) {
+        if (FAILED(data_source->loadDataForExe(file.c_str(), NULL, NULL))) {
+          fprintf(stderr, "loadDataForPdb and loadDataFromExe failed\n");
+          return false;
+        }
+      }
+      break;
     default:
       fprintf(stderr, "Unknown file format\n");
       return false;
@@ -379,6 +387,18 @@ bool PDBSourceLineWriter::PrintCodePublicSymbol(IDiaSymbol *symbol) {
   return true;
 }
 
+bool PDBSourceLineWriter::PrintPDBInfo() {
+  wstring guid, filename;
+  int age;
+  if (!GetModuleInfo(&guid, &age, &filename)) {
+    return false;
+  }
+
+  fprintf(output_, "MODULE %ws %x %ws\n", guid.c_str(), age, filename.c_str());
+
+  return true;
+}
+
 // wcstol_positive_strict is sort of like wcstol, but much stricter.  string
 // should be a buffer pointing to a null-terminated string containing only
 // decimal digits.  If the entire string can be converted to an integer
@@ -608,11 +628,12 @@ next_child:
 }
 
 bool PDBSourceLineWriter::WriteMap(FILE *map_file) {
-  bool ret = false;
   output_ = map_file;
-  if (PrintSourceFiles() && PrintFunctions() && PrintFrameData()) {
-    ret = true;
-  }
+
+  bool ret = PrintPDBInfo() &&
+             PrintSourceFiles() && 
+             PrintFunctions() &&
+             PrintFrameData();
 
   output_ = NULL;
   return ret;
@@ -622,18 +643,47 @@ void PDBSourceLineWriter::Close() {
   session_.Release();
 }
 
-wstring PDBSourceLineWriter::GetModuleGUID() {
+// static
+wstring PDBSourceLineWriter::GetBaseName(const wstring &filename) {
+  wstring base_name(filename);
+  size_t slash_pos = base_name.find_last_of(L"/\\");
+  if (slash_pos != wstring::npos) {
+    base_name.erase(0, slash_pos + 1);
+  }
+  return base_name;
+}
+
+bool PDBSourceLineWriter::GetModuleInfo(wstring *guid, int *age,
+                                        wstring *filename) {
+  guid->clear();
+  *age = 0;
+  filename->clear();
+
   CComPtr<IDiaSymbol> global;
   if (FAILED(session_->get_globalScope(&global))) {
-    return L"";
+    return false;
   }
 
-  GUID guid;
-  if (FAILED(global->get_guid(&guid))) {
-    return L"";
+  GUID guid_number;
+  if (FAILED(global->get_guid(&guid_number))) {
+    return false;
   }
+  *guid = GUIDString::GUIDToWString(&guid_number);
 
-  return GUIDString::GUIDToWString(&guid);
+  // DWORD* and int* are not compatible.  This is clean and avoids a cast.
+  DWORD age_dword;
+  if (FAILED(global->get_age(&age_dword))) {
+    return false;
+  }
+  *age = age_dword;
+
+  CComBSTR filename_string;
+  if (FAILED(global->get_symbolsFileName(&filename_string))) {
+    return false;
+  }
+  *filename = GetBaseName(wstring(filename_string));
+
+  return true;
 }
 
 }  // namespace google_airbag

src/common/windows/pdb_source_line_writer.h

@@ -50,6 +50,7 @@ class PDBSourceLineWriter {
   enum FileFormat {
     PDB_FILE,  // a .pdb file containing debug symbols
     EXE_FILE,  // a .exe or .dll file
+    ANY_FILE   // try PDB_FILE and then EXE_FILE
   };
 
   explicit PDBSourceLineWriter();
@@ -73,9 +74,11 @@ class PDBSourceLineWriter {
   // Closes the current pdb file and its associated resources.
   void Close();
 
-  // Returns the GUID for the module, as a string,
-  // e.g. "11111111-2222-3333-4444-555555555555".
-  wstring GetModuleGUID();
+  // Sets guid to the GUID for the module, as a string,
+  // e.g. "11111111-2222-3333-4444-555555555555".  age will be set to the
+  // age of the pdb file, and filename will be set to the basename of the
+  // PDB's file name.  Returns true on success and false on failure.
+  bool GetModuleInfo(wstring *guid, int *age, wstring *filename);
 
  private:
   // Outputs the line/address pairs for each line in the enumerator.
@@ -102,6 +105,13 @@ class PDBSourceLineWriter {
   // correspond to code, returns true without outputting anything.
   bool PrintCodePublicSymbol(IDiaSymbol *symbol);
 
+  // Outputs a line identifying the PDB file that is being dumped, along with
+  // its uuid and age.
+  bool PrintPDBInfo();
+
+  // Returns the base name of a file, e.g. strips off the path.
+  static wstring GetBaseName(const wstring &filename);
+
   // Returns the function name for a symbol.  If possible, the name is
   // undecorated.  If the symbol's decorated form indicates the size of
   // parameters on the stack, this information is returned in stack_param_size.

src/processor/simple_symbol_supplier.cc

@@ -67,7 +67,7 @@ string SimpleSymbolSupplier::GetSymbolFile(MinidumpModule *module) {
   path.append("/");
   char uuid_age_string[43];
   snprintf(uuid_age_string, sizeof(uuid_age_string),
-           "%08X%04X%04X%02X%02X%02X%02X%02X%02X%02X%02X%d",
+           "%08X%04X%04X%02X%02X%02X%02X%02X%02X%02X%02X%X",
            cv_record->signature.data1, cv_record->signature.data2,
            cv_record->signature.data3,
            cv_record->signature.data4[0], cv_record->signature.data4[1],

src/processor/simple_symbol_supplier.h

@@ -36,8 +36,8 @@
 // directory with a name identical to the corresponding pdb file.  Within
 // each of these directories, there are subdirectories named for the uuid and
 // age of each pdb file.  The uuid is presented in hexadecimal form, with
-// uppercase characters and no dashes.  The age is appended to it in decimal
-// form, without any separators.  Within that subdirectory,
+// uppercase characters and no dashes.  The age is appended to it in
+// hexadecimal form, without any separators.  Within that subdirectory,
 // SimpleSymbolSupplier expects to find the symbol file, which is named
 // identically to the pdb file, but with a .sym extension.  This sample
 // hierarchy is rooted at the "symbols" base directory:
@@ -59,9 +59,6 @@
 // SimpleSymbolServer, provided that the pdb files are transformed to dumped
 // format using a tool such as dump_syms, and given a .sym extension.
 //
-// TODO(mmentovai): verify that MSSS stores the age as presented as decimal
-// and not hexadecimal or something else.  Adjust here to match if necessary.
-//
 // SimpleSymbolSupplier presently only supports symbol files that have
 // the MSVC 7.0 CodeView record format.  See MDCVInfoPDB70 in
 // minidump_format.h.

src/processor/source_line_resolver.cc

@@ -249,6 +249,13 @@ bool SourceLineResolver::Module::LoadMap(const string &map_file) {
       if (!ParsePublicSymbol(buffer)) {
         return false;
       }
+    } else if (strncmp(buffer, "MODULE ", 7) == 0) {
+      // Ignore these.  They're not of any use to SourceLineResolver, which
+      // is fed modules by a SymbolSupplier.  These lines are present to
+      // aid other tools in properly placing symbol files so that they can
+      // be accessed by a SymbolSupplier.
+      //
+      // MODULE <guid> <age> <filename>
     } else {
       if (!cur_func) {
         return false;

src/processor/testdata/module1.out

@@ -1,3 +1,4 @@
+MODULE 11111111-1111-1111-1111-111111111111 1 module1.pdb
 FILE 1 file1_1.cc
 FILE 2 file1_2.cc
 FILE 3 file1_3.cc

src/processor/testdata/module2.out

@@ -1,3 +1,4 @@
+MODULE 22222222-2222-2222-2222-222222222222 2 module2.pdb
 FILE 1 file2_1.cc
 FILE 2 file2_2.cc
 FILE 3 file2_3.cc

src/processor/testdata/module3_bad.out

@@ -1,2 +1,3 @@
+MODULE 33333333-3333-3333-3333-333333333333 3 module3.pdb
 FILE 1 file1.cc
 FUNC 1000

src/processor/testdata/symbols/kernel32.pdb/BCE8785C57B44245A669896B6A19B9542/kernel32.sym

@@ -1,3 +1,4 @@
+MODULE bce8785c-57b4-4245-a669-896b6a19b954 2 kernel32.pdb
 PUBLIC b5ae c BaseDllInitialize
 PUBLIC 9b47 4 CloseHandle
 PUBLIC 17826 0 BaseDllInitializeMemoryManager

src/processor/testdata/symbols/test_app.pdb/63FE4780728D49379B9D7BB6460CB42A1/test_app.sym

@@ -1,3 +1,4 @@
+MODULE 63fe4780-728d-4937-9b9d-7bb6460cb42a 1 test_app.pdb
 FILE 1 c:\program files\microsoft visual studio 8\vc\platformsdk\include\winsock.h
 FILE 2 c:\program files\microsoft visual studio 8\vc\platformsdk\include\winuser.h
 FILE 3 c:\program files\microsoft visual studio 8\vc\platformsdk\include\wincon.h

src/tools/windows/dump_syms/dump_syms.cc

@@ -52,7 +52,7 @@ int main(int argc, char **argv) {
   }
 
   PDBSourceLineWriter writer;
-  if (!writer.Open(wstring(filename), PDBSourceLineWriter::PDB_FILE)) {
+  if (!writer.Open(wstring(filename), PDBSourceLineWriter::ANY_FILE)) {
     fprintf(stderr, "Open failed\n");
     return 1;
   }

src/tools/windows/dump_syms/run_regtest.sh

@@ -29,10 +29,25 @@
 # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-Release/dump_syms.exe testdata/dump_syms_regtest.pdb > testdata/dump_syms_regtest.new
-if diff -u testdata/dump_syms_regtest.new testdata/dump_syms_regtest.out >& testdata/dump_syms_regtest.diff; then
+Release/dump_syms.exe testdata/dump_syms_regtest.pdb | \
+ tr -d '\015' > \
+ testdata/dump_syms_regtest.new
+status=$?
+
+if [ $status -ne 0 ] ; then
+  echo "FAIL, dump_syms.exe failed"
+  exit $status
+fi
+
+diff -u testdata/dump_syms_regtest.new testdata/dump_syms_regtest.out > \
+ testdata/dump_syms_regtest.diff
+status=$?
+
+if [ $status -eq 0 ] ; then
   rm testdata/dump_syms_regtest.diff testdata/dump_syms_regtest.new
   echo "PASS"
 else
   echo "FAIL, see testdata/dump_syms_regtest.[new|diff]"
 fi
+
+exit $status

src/tools/windows/symupload/symupload.cc

@@ -95,11 +95,13 @@ static bool GetFileVersionString(const wchar_t *filename, wstring *version) {
 // Creates a new temporary file and writes the symbol data from the given
 // exe/dll file to it.  Returns the path to the temp file in temp_file_path,
 // and the unique identifier (GUID) for the pdb in module_guid.
-static bool DumpSymbolsToTempFile(const wchar_t *exe_file,
+static bool DumpSymbolsToTempFile(const wchar_t *file,
                                   wstring *temp_file_path,
-                                  wstring *module_guid) {
+                                  wstring *module_guid,
+                                  int *module_age,
+                                  wstring *module_filename) {
   google_airbag::PDBSourceLineWriter writer;
-  if (!writer.Open(exe_file, PDBSourceLineWriter::EXE_FILE)) {
+  if (!writer.Open(file, PDBSourceLineWriter::ANY_FILE)) {
     return false;
   }
 
@@ -126,37 +128,33 @@ static bool DumpSymbolsToTempFile(const wchar_t *exe_file,
   }
 
   *temp_file_path = temp_filename;
-  *module_guid = writer.GetModuleGUID();
-  return true;
-}
 
-// Returns the base name of a file, e.g. strips off the path.
-static wstring GetBaseName(const wstring &filename) {
-  wstring base_name(filename);
-  size_t slash_pos = base_name.find_last_of(L"/\\");
-  if (slash_pos != string::npos) {
-    base_name.erase(0, slash_pos + 1);
-  }
-  return base_name;
+  return writer.GetModuleInfo(module_guid, module_age, module_filename);
 }
 
 int wmain(int argc, wchar_t *argv[]) {
   if (argc < 3) {
-    wprintf(L"Usage: %s file.[exe|dll] <symbol upload URL>\n", argv[0]);
+    wprintf(L"Usage: %s file.[pdb|exe|dll] <symbol upload URL>\n", argv[0]);
     return 0;
   }
   const wchar_t *module = argv[1], *url = argv[2];
-  wstring module_basename = GetBaseName(module);
 
-  wstring symbol_file, module_guid;
-  if (!DumpSymbolsToTempFile(module, &symbol_file, &module_guid)) {
+  wstring symbol_file, module_guid, module_basename;
+  int module_age;
+  if (!DumpSymbolsToTempFile(module, &symbol_file,
+                             &module_guid, &module_age, &module_basename)) {
     fwprintf(stderr, L"Could not get symbol data from %s\n", module);
     return 1;
   }
 
+  wchar_t module_age_string[11];
+  _snwprintf_s(module_age_string, sizeof(module_age_string) / sizeof(wchar_t),
+               _TRUNCATE, L"0x%x", module_age);
+
   map<wstring, wstring> parameters;
   parameters[L"module"] = module_basename;
   parameters[L"guid"] = module_guid;
+  parameters[L"age"] = module_age_string;
 
   // Don't make a missing version a hard error.  Issue a warning, and let the
   // server decide whether to reject files without versions.