symupload parameters don't match processor expectations (#91). r=bryner

- Interface change: the "guid" and "age" parameters supplied to a symbol
   server by symupload have been merged into "debug_identifier".  Some
   other parameters have had their names changed.  Additional code_file,
   os, and cpu parameters have been added.
 - Interface change: the format of the MODULE line at the top of dumped .sym
   files has changed slightly.  The fields used for uuid and age have
   merged into a debug_identifier-type field.
 - debug_identifier is formatted the same way as CodeModule::debug_identifier
   for ease of server-side processing.

http://groups.google.com/group/airbag-dev/browse_thread/thread/8022f504cf01f994


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

src/common/windows/guid_string.cc

@@ -52,4 +52,17 @@ wstring GUIDString::GUIDToWString(GUID *guid) {
   return wstring(guid_string);
 }
 
+// static
+wstring GUIDString::GUIDToSymbolServerWString(GUID *guid) {
+  wchar_t guid_string[33];
+  WindowsStringUtils::safe_swprintf(
+      guid_string, sizeof(guid_string) / sizeof(guid_string[0]),
+      L"%08X%04X%04X%02X%02X%02X%02X%02X%02X%02X%02X",
+      guid->Data1, guid->Data2, guid->Data3,
+      guid->Data4[0], guid->Data4[1], guid->Data4[2],
+      guid->Data4[3], guid->Data4[4], guid->Data4[5],
+      guid->Data4[6], guid->Data4[7]);
+  return wstring(guid_string);
+}
+
 }  // namespace google_airbag

src/common/windows/guid_string.h

@@ -45,6 +45,12 @@ class GUIDString {
   // Converts guid to a string in the format recommended by RFC 4122 and
   // returns the string.
   static wstring GUIDToWString(GUID *guid);
+
+  // Converts guid to a string formatted as uppercase hexadecimal, with
+  // no separators, and returns the string.  This is the format used for
+  // symbol server identifiers, although identifiers have an age tacked
+  // on to the string.
+  static wstring GUIDToSymbolServerWString(GUID *guid);
 };
 
 }  // namespace google_airbag

src/common/windows/pdb_source_line_writer.cc

@@ -408,17 +408,17 @@ bool PDBSourceLineWriter::PrintCodePublicSymbol(IDiaSymbol *symbol) {
 }
 
 bool PDBSourceLineWriter::PrintPDBInfo() {
-  wstring guid, filename, cpu;
+  PDBModuleInfo info;
-  int age;
+  if (!GetModuleInfo(&info)) {
-  if (!GetModuleInfo(&guid, &age, &filename, &cpu)) {
     return false;
   }
 
   // Hard-code "windows" for the OS because that's the only thing that makes
   // sense for PDB files.  (This might not be strictly correct for Windows CE
   // support, but we don't care about that at the moment.)
-  fprintf(output_, "MODULE windows %ws %ws %x %ws\n",
+  fprintf(output_, "MODULE windows %ws %ws %ws\n",
-          cpu.c_str(), guid.c_str(), age, filename.c_str());
+          info.cpu.c_str(), info.debug_identifier.c_str(),
+          info.debug_file.c_str());
 
   return true;
 }
@@ -674,38 +674,34 @@ void PDBSourceLineWriter::Close() {
   session_.Release();
 }
 
-// static
+bool PDBSourceLineWriter::GetModuleInfo(PDBModuleInfo *info) {
-wstring PDBSourceLineWriter::GetBaseName(const wstring &filename) {
+  if (!info) {
-  wstring base_name(filename);
+    return false;
-  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,
+  info->debug_file.clear();
-                                        wstring *filename, wstring *cpu) {
+  info->debug_identifier.clear();
-  guid->clear();
+  info->cpu.clear();
-  *age = 0;
-  filename->clear();
 
   CComPtr<IDiaSymbol> global;
   if (FAILED(session_->get_globalScope(&global))) {
     return false;
   }
 
-  // cpu is permitted to be NULL.
+  // All CPUs in CV_CPU_TYPE_e (cvconst.h) below 0x10 are x86.  There's no
-  if (cpu) {
+  // single specific constant to use.
-    // All CPUs in CV_CPU_TYPE_e (cvconst.h) below 0x10 are x86.  There's no
+  DWORD platform;
-    // single specific constant to use.
+  if (SUCCEEDED(global->get_platform(&platform)) && platform < 0x10) {
-    DWORD platform;
+    info->cpu = L"x86";
-    if (SUCCEEDED(global->get_platform(&platform)) && platform < 0x10) {
+  } else {
-      *cpu = L"x86";
+    // Unexpected, but handle gracefully.
-    } else {
+    info->cpu = L"unknown";
-      // Unexpected, but handle gracefully.
+  }
-      *cpu = L"unknown";
+
-    }
+  // DWORD* and int* are not compatible.  This is clean and avoids a cast.
+  DWORD age;
+  if (FAILED(global->get_age(&age))) {
+    return false;
   }
 
   bool uses_guid;
@@ -714,38 +710,38 @@ bool PDBSourceLineWriter::GetModuleInfo(wstring *guid, int *age,
   }
 
   if (uses_guid) {
-    GUID guid_number;
+    GUID guid;
-    if (FAILED(global->get_guid(&guid_number))) {
+    if (FAILED(global->get_guid(&guid))) {
       return false;
     }
 
-    *guid = GUIDString::GUIDToWString(&guid_number);
+    wchar_t age_string[9];
+    WindowsStringUtils::safe_swprintf(
+        age_string, sizeof(age_string) / sizeof(age_string[0]),
+        L"%X", age);
+
+    info->debug_identifier = GUIDString::GUIDToSymbolServerWString(&guid);
+    info->debug_identifier.append(age_string);
   } else {
     DWORD signature;
     if (FAILED(global->get_signature(&signature))) {
       return false;
     }
 
-    wchar_t signature_string[9];
+    wchar_t identifier_string[17];
     WindowsStringUtils::safe_swprintf(
-        signature_string,
+        identifier_string,
-        sizeof(signature_string) / sizeof(signature_string[0]),
+        sizeof(identifier_string) / sizeof(identifier_string[0]),
-        L"%08x", signature);
+        L"%08x%x", signature, age);
-    *guid = signature_string;
+    info->debug_identifier = identifier_string;
   }
 
-  // DWORD* and int* are not compatible.  This is clean and avoids a cast.
+  CComBSTR debug_file_string;
-  DWORD age_dword;
+  if (FAILED(global->get_symbolsFileName(&debug_file_string))) {
-  if (FAILED(global->get_age(&age_dword))) {
     return false;
   }
-  *age = age_dword;
+  info->debug_file =
-
+      WindowsStringUtils::GetBaseName(wstring(debug_file_string));
-  CComBSTR filename_string;
-  if (FAILED(global->get_symbolsFileName(&filename_string))) {
-    return false;
-  }
-  *filename = GetBaseName(wstring(filename_string));
 
   return true;
 }

src/common/windows/pdb_source_line_writer.h

@@ -45,6 +45,26 @@ namespace google_airbag {
 
 using std::wstring;
 
+// A structure that carries information that identifies a pdb file.
+struct PDBModuleInfo {
+ public:
+  // The basename of the pdb file from which information was loaded.
+  wstring debug_file;
+
+  // The pdb's identifier.  For recent pdb files, the identifier consists
+  // of the pdb's guid, in uppercase hexadecimal form without any dashes
+  // or separators, followed immediately by the pdb's age, also in
+  // uppercase hexadecimal form.  For older pdb files which have no guid,
+  // the identifier is the pdb's 32-bit signature value, in zero-padded
+  // hexadecimal form, followed immediately by the pdb's age, in lowercase
+  // hexadecimal form.
+  wstring debug_identifier;
+
+  // A string identifying the cpu that the pdb is associated with.
+  // Currently, this may be "x86" or "unknown".
+  wstring cpu;
+};
+
 class PDBSourceLineWriter {
  public:
   enum FileFormat {
@@ -74,15 +94,9 @@ class PDBSourceLineWriter {
   // Closes the current pdb file and its associated resources.
   void Close();
 
-  // Sets guid to the GUID for the module, as a string,
+  // Retrieves information about the module's debugging file.  Returns
-  // e.g. "11111111-2222-3333-4444-555555555555".  If the module has no guid,
+  // true on success and false on failure.
-  // guid will instead be set to the module's 32-bit signature value, in
+  bool GetModuleInfo(PDBModuleInfo *info);
-  // zero-padded hexadecimal form, such as "0004beef".  age will be set to the
-  // age of the pdb file, filename will be set to the basename of the pdb's
-  // file name, and cpu will be set to a string identifying the associated CPU
-  // architecture.  cpu is permitted to be NULL, in which case CPU information
-  // will not be returned.  Returns true on success and false on failure.
-  bool GetModuleInfo(wstring *guid, int *age, wstring *filename, wstring *cpu);
 
   // Sets uses_guid to true if the opened file uses a new-style CodeView
   // record with a 128-bit GUID, or false if the opened file uses an old-style
@@ -120,9 +134,6 @@ class PDBSourceLineWriter {
   // 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/common/windows/string_utils-inl.h

@@ -36,6 +36,8 @@
 #include <stdarg.h>
 #include <wchar.h>
 
+#include <string>
+
 // The "ll" printf format size specifier corresponding to |long long| was
 // intrudced in MSVC8.  Earlier versions did not provide this size specifier,
 // but "I64" can be used to print 64-bit types.  Don't use "I64" where "ll"
@@ -49,6 +51,8 @@
 
 namespace google_airbag {
 
+using std::wstring;
+
 class WindowsStringUtils {
  public:
   // Equivalent to MSVC8's swprintf, which always 0-terminates buffer.
@@ -68,6 +72,9 @@ class WindowsStringUtils {
   static void safe_wcsncpy(wchar_t *destination, size_t destination_size,
                            const wchar_t *source, size_t count);
 
+  // Returns the base name of a file, e.g. strips off the path.
+  static wstring GetBaseName(const wstring &filename);
+
  private:
   // Disallow instantiation and other object-based operations.
   WindowsStringUtils();

src/common/windows/string_utils.cc

@@ -0,0 +1,44 @@
+// Copyright (c) 2006, Google Inc.
+// All rights reserved.
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following disclaimer
+// in the documentation and/or other materials provided with the
+// distribution.
+//     * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+#include "common/windows/string_utils-inl.h"
+
+namespace google_airbag {
+
+// static
+wstring WindowsStringUtils::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;
+}
+
+}  // namespace google_airbag

src/tools/windows/dump_syms/dump_syms.vcproj

@@ -211,6 +211,10 @@
 				RelativePath="..\..\..\common\windows\pdb_source_line_writer.cc"
 				>
 			</File>
+			<File
+				RelativePath="..\..\..\common\windows\string_utils.cc"
+				>
+			</File>
 		</Filter>
 	</Files>
 	<Globals>

src/tools/windows/symupload/symupload.cc

@@ -31,11 +31,16 @@
 // The PDB file is located automatically, using the path embedded in the
 // executable.  The upload is sent as a multipart/form-data POST request,
 // with the following parameters:
-//  module: the name of the module, e.g. app.exe
+//  code_file: the basename of the module, e.g. "app.exe"
-//  ver: the file version of the module, e.g. 1.2.3.4
+//  debug_file: the basename of the debugging file, e.g. "app.pdb"
-//  guid: the GUID string embedded in the module pdb,
+//  debug_identifier: the debug file's identifier, usually consisting of
-//        e.g. 11111111-2222-3333-4444-555555555555
+//                    the guid and age embedded in the pdb, e.g.
-//  symbol_file: the airbag-format symbol file
+//                    "11111111BBBB3333DDDD555555555555F"
+//  version: the file version of the module, e.g. "1.2.3.4"
+//  os: the operating system that the module was built for, always
+//      "windows" in this implementation.
+//  cpu: the CPU that the module was built for, typically "x86".
+//  symbol_file: the contents of the airbag-format symbol file
 
 #include <Windows.h>
 #include <DbgHelp.h>
@@ -56,6 +61,7 @@ using std::wstring;
 using std::vector;
 using std::map;
 using google_airbag::HTTPUpload;
+using google_airbag::PDBModuleInfo;
 using google_airbag::PDBSourceLineWriter;
 using google_airbag::WindowsStringUtils;
 
@@ -97,15 +103,16 @@ 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,
+// 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.
+// and information about the pdb in pdb_info.
 static bool DumpSymbolsToTempFile(const wchar_t *file,
                                   wstring *temp_file_path,
-                                  wstring *module_guid,
+                                  PDBModuleInfo *pdb_info) {
-                                  int *module_age,
-                                  wstring *module_filename) {
   google_airbag::PDBSourceLineWriter writer;
-  if (!writer.Open(file, PDBSourceLineWriter::ANY_FILE)) {
+  // Use EXE_FILE to get information out of the exe/dll in addition to the
+  // pdb.  The name and version number of the exe/dll are of value, and
+  // there's no way to locate an exe/dll given a pdb.
+  if (!writer.Open(file, PDBSourceLineWriter::EXE_FILE)) {
     return false;
   }
 
@@ -139,34 +146,31 @@ static bool DumpSymbolsToTempFile(const wchar_t *file,
 
   *temp_file_path = temp_filename;
 
-  return writer.GetModuleInfo(module_guid, module_age, module_filename, NULL);
+  return writer.GetModuleInfo(pdb_info);
 }
 
 int wmain(int argc, wchar_t *argv[]) {
   if (argc < 3) {
-    wprintf(L"Usage: %s file.[pdb|exe|dll] <symbol upload URL>\n", argv[0]);
+    wprintf(L"Usage: %s <file.exe|file.dll> <symbol upload URL>\n", argv[0]);
     return 0;
   }
   const wchar_t *module = argv[1], *url = argv[2];
 
-  wstring symbol_file, module_guid, module_basename;
+  wstring symbol_file;
-  int module_age;
+  PDBModuleInfo pdb_info;
-  if (!DumpSymbolsToTempFile(module, &symbol_file,
+  if (!DumpSymbolsToTempFile(module, &symbol_file, &pdb_info)) {
-                             &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];
+  wstring code_file = WindowsStringUtils::GetBaseName(wstring(module));
-  WindowsStringUtils::safe_swprintf(
-      module_age_string,
-      sizeof(module_age_string) / sizeof(module_age_string[0]),
-      L"0x%x", module_age);
 
   map<wstring, wstring> parameters;
-  parameters[L"module"] = module_basename;
+  parameters[L"code_file"] = code_file;
-  parameters[L"guid"] = module_guid;
+  parameters[L"debug_file"] = pdb_info.debug_file;
-  parameters[L"age"] = module_age_string;
+  parameters[L"debug_identifier"] = pdb_info.debug_identifier;
+  parameters[L"os"] = L"windows";  // This version of symupload is Windows-only
+  parameters[L"cpu"] = pdb_info.cpu;
 
   // Don't make a missing version a hard error.  Issue a warning, and let the
   // server decide whether to reject files without versions.
@@ -186,7 +190,9 @@ int wmain(int argc, wchar_t *argv[]) {
     return 1;
   }
 
-  wprintf(L"Uploaded symbols for %s/%s/%s\n",
+  wprintf(L"Uploaded symbols for windows-%s/%s/%s (%s %s)\n",
-         module_basename.c_str(), file_version.c_str(), module_guid.c_str());
+          pdb_info.cpu.c_str(), pdb_info.debug_file.c_str(),
+          pdb_info.debug_identifier.c_str(), code_file.c_str(),
+          file_version.c_str());
   return 0;
 }

src/tools/windows/symupload/symupload.vcproj

@@ -216,6 +216,10 @@
 				RelativePath="..\..\..\common\windows\pdb_source_line_writer.cc"
 				>
 			</File>
+			<File
+				RelativePath="..\..\..\common\windows\string_utils.cc"
+				>
+			</File>
 			<File
 				RelativePath=".\symupload.cc"
 				>