Edit breakpad_file_extractor and exceptions.

This CL improves documentation, and exception message
clarity and consistency. It also fixes a minor bug in
tools/tracing/breakpad_file_extractor.py where it runs
dump_syms on '.so.dwp' files when it should not.

Bug: 1224717
Change-Id: I390663461381e5c56ba83f646b89500a06f0c416
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3044546
Commit-Queue: Ryan Huckleberry <rhuckleberry@google.com>
Reviewed-by: ssid <ssid@chromium.org>
Cr-Commit-Position: refs/heads/master@{#904501}

tools/tracing/breakpad_file_extractor.py

@@ -16,9 +16,10 @@ def ExtractBreakpadFiles(dump_syms_path, build_dir, breakpad_output_dir):
   Args:
     dump_syms_path: The path to the dump_syms binary that should be run.
     build_dir: The path to the input directory containing the binaries that
-    dump_syms will use.
+      dump_syms will use. If the directory '|build_dir|/lib.unstipped'
+      exists, dump_syms is run on this directory instead.
     breakpad_base_dir: The output directory for the breakpad symbol files
-    produced.
+      produced.
 
   Raises:
     FileNotFoundError: If the dump_syms binary or the input and output
@@ -48,7 +49,7 @@ def ExtractBreakpadFiles(dump_syms_path, build_dir, breakpad_output_dir):
     if os.path.isfile(input_file_path) and _IsValidBinaryPath(input_file_path):
       # Construct absolute file paths for input and output files.
       output_file_path = os.path.join(breakpad_output_dir, file + '.breakpad')
-      cmd = [dump_syms_path, input_file_path]
+      cmd = [dump_syms_path, input_file_path, '-c', '-r']
       if _RunDumpSyms(cmd, output_file_path):
         breakpad_file_count += 1
 
@@ -102,7 +103,8 @@ def _DumpSymsExists(dump_syms_path, build_dir):
 def _IsValidBinaryPath(path):
   # Get the file name from the full file path.
   file_name = os.path.basename(path)
-  if file_name.endswith('partition.so'):
+  if file_name.endswith('partition.so') or file_name.endswith(
+      '.dwp') or file_name.endswith('.dwo'):
     return False
   return 'chrome' in file_name or file_name.endswith(
       '.so') or file_name.endswith('.exe')

tools/tracing/breakpad_file_extractor_unittest.py

@@ -28,10 +28,7 @@ def setUp(self):
       pass
 
   def tearDown(self):
-    if len(os.listdir(self.test_build_dir)) == 0:
-      os.rmdir(self.test_build_dir)
-    else:
-      shutil.rmtree(self.test_build_dir)
+    shutil.rmtree(self.test_build_dir)
     shutil.rmtree(self.test_breakpad_dir)
     shutil.rmtree(self.test_dump_syms_dir)
 
@@ -56,7 +53,7 @@ def _checkExtractWithOneBinary(self, dump_syms_path, build_dir, breakpad_dir):
     breakpad_file_extractor.ExtractBreakpadFiles(dump_syms_path, build_dir,
                                                  breakpad_dir)
 
-    expected_cmd = [dump_syms_path, test_input_file.name]
+    expected_cmd = [dump_syms_path, test_input_file.name, '-c', '-r']
     breakpad_file_extractor._RunDumpSyms.assert_called_once_with(
         expected_cmd, test_output_file_path)
 
@@ -106,11 +103,11 @@ def testMultipleBinaryFiles(self):
 
     # Check that each call expected call to _RunDumpSyms() has been made.
     expected_calls = [
-        call([self.test_dump_syms_binary, input_files[0].name],
+        call([self.test_dump_syms_binary, input_files[0].name, '-c', '-r'],
              output_file_paths[0]),
-        call([self.test_dump_syms_binary, input_files[1].name],
+        call([self.test_dump_syms_binary, input_files[1].name, '-c', '-r'],
              output_file_paths[1]),
-        call([self.test_dump_syms_binary, input_files[2].name],
+        call([self.test_dump_syms_binary, input_files[2].name, '-c', '-r'],
              output_file_paths[2])
     ]
     breakpad_file_extractor._RunDumpSyms.assert_has_calls(expected_calls,
@@ -131,7 +128,7 @@ def testMultipleBinaryFiles(self):
 
   def testDumpSymsNotFound(self):
     breakpad_file_extractor._RunDumpSyms = MagicMock()
-    with self.assertRaisesRegex(FileNotFoundError, '^dump_syms is missing\.'):
+    with self.assertRaisesRegex(FileNotFoundError, 'dump_syms is missing.'):
       breakpad_file_extractor.ExtractBreakpadFiles('fake/path/dump_syms',
                                                    self.test_build_dir,
                                                    self.test_breakpad_dir)
@@ -150,7 +147,22 @@ def testFakeDirectories(self):
 
   def testSymbolizedNoFiles(self):
     with self.assertRaisesRegex(
-        Exception, '^Could not create breakpad symbols from any files'):
+        Exception, 'Could not create breakpad symbols from any files'):
+      breakpad_file_extractor.ExtractBreakpadFiles(self.test_dump_syms_binary,
+                                                   self.test_build_dir,
+                                                   self.test_breakpad_dir)
+
+  def testIgnoredwp(self):
+    dwp_file1 = os.path.join(self.test_build_dir, 'name.so.dwp')
+    dwp_file2 = os.path.join(self.test_build_dir, 'name.so.dwo')
+    with open(dwp_file1, 'w') as file1:
+      file1.write('MODULE Linux x86_64 34984AB4EF948C name1.so')
+    with open(dwp_file2, 'w') as file2:
+      file2.write('MODULE Linux x86_64 34984AB4EF948C name2.so')
+
+    exception_msg = ('Could not create breakpad symbols from any files from ' +
+                     self.test_build_dir + '.')
+    with self.assertRaisesRegex(Exception, exception_msg):
       breakpad_file_extractor.ExtractBreakpadFiles(self.test_dump_syms_binary,
                                                    self.test_build_dir,
                                                    self.test_breakpad_dir)

tools/tracing/metadata_extractor.py

@@ -82,6 +82,10 @@ def __str__(self):
                 version_code=self.version_code,
                 modules=self.modules))
 
+  @property
+  def trace_file(self):
+    return self._trace_file
+
   def Initialize(self):
     """Extracts metadata from perfetto system trace.
     """
@@ -141,7 +145,8 @@ def _ParseOSName(self, raw_os_name):
     elif raw_os_name == 'Fuschia':
       return OSName.FUSCHIA
     else:
-      raise Exception('OS name %s not recognized.' % (raw_os_name))
+      raise Exception('OS name "%s" not recognized: %s' %
+                      (raw_os_name, self._trace_file))
 
   def InitializeForTesting(self,
                            version_number=None,

tools/tracing/metadata_extractor_unittests.py

@@ -272,10 +272,10 @@ def side_effect(*args):
                                                      self.trace_file)
     trace_processor.RunQuery = mock.MagicMock(side_effect=side_effect)
 
+    exception_msg = 'OS name "blah" not recognized: ' + self.trace_file
     with self.assertRaises(Exception) as context:
       extractor.Initialize()
-
-    self.assertTrue('OS name blah not recognized.' in str(context.exception))
+    self.assertEqual(exception_msg, str(context.exception))
 
 
 if __name__ == '__main__':

tools/tracing/profile_chrome_startup

@@ -73,7 +73,7 @@ def main():
   if options.platform.lower() == 'android':
     trace_file = adb_profile_chrome_startup.ProfileChrome(options)
   else:
-    raise ValueError('Platform "%s" is not supported. '
+    raise Exception('Platform "%s" is not supported. '
                     'Specify platform with the --platform flag.' % (options.platform))
 
   # TODO(rhuckleberry): Make --view flag show the trace after symbolization, instead

tools/tracing/symbol_fetcher.py

@@ -35,9 +35,10 @@ def GetTraceBreakpadSymbols(cloud_storage_bucket, metadata,
 
   metadata.Initialize()
   if metadata.os_name is None:
-    raise Exception('Failed to extract trace OS name.')
+    raise Exception('Failed to extract trace OS name: ' + metadata.trace_file)
   if metadata.version_number is None:
-    raise Exception('Failed to extract trace version number.')
+    raise Exception('Failed to extract trace version number: ' +
+                    metadata.trace_file)
 
   # Extract symbols by platform.
   if metadata.os_name == OSName.ANDROID:
@@ -47,7 +48,8 @@ def GetTraceBreakpadSymbols(cloud_storage_bucket, metadata,
   elif metadata.os_name == OSName.LINUX or metadata.os_name == OSName.MAC:
     _FetchBreakpadSymbols(cloud_storage_bucket, metadata, breakpad_output_dir)
   else:
-    raise Exception('Trace OS is not supported: ' + metadata.os_name)
+    raise Exception('Trace OS "%s" is not supported: %s' %
+                    (metadata.os_name, metadata.trace_file))
 
   logging.info('Breakpad symbols located at: ' +
                os.path.abspath(breakpad_output_dir))
@@ -88,7 +90,8 @@ def _FetchBreakpadSymbols(cloud_storage_bucket, metadata, breakpad_output_dir):
         logging.warning('Architecture not found, so using x86-64.')
       folder = 'mac64'
   else:
-    raise Exception('Expected OS to be Linux or Mac: ' + metadata.os_name)
+    raise Exception('Expected OS "%s" to be Linux or Mac: %s' %
+                    (metadata.os_name, metadata.trace_file))
 
   # Build Google Cloud Storage path to the symbols.
   gsc_folder = 'desktop-*/' + metadata.version_number + '/' + folder

tools/tracing/symbol_fetcher_unittests.py

@@ -157,7 +157,8 @@ def testSymbolFetcherNoOSName(self):
                                              architecture='x86_64',
                                              bitness='64')
 
-    with self.assertRaisesRegex(Exception, 'Failed to extract trace OS name.'):
+    exception_msg = 'Failed to extract trace OS name: ' + self.trace_file
+    with self.assertRaisesRegex(Exception, exception_msg):
       symbol_fetcher.GetTraceBreakpadSymbols(self.cloud_storage_bucket,
                                              metadata, self.breakpad_output_dir)
 
@@ -170,7 +171,8 @@ def testSymbolFetcherOSNameNotRecognized(self):
                                              architecture='x86_64',
                                              bitness='64')
 
-    with self.assertRaisesRegex(Exception, 'Trace OS is not supported: blah'):
+    exception_msg = 'Trace OS "blah" is not supported: ' + self.trace_file
+    with self.assertRaisesRegex(Exception, exception_msg):
       symbol_fetcher.GetTraceBreakpadSymbols(self.cloud_storage_bucket,
                                              metadata, self.breakpad_output_dir)
 
@@ -197,8 +199,8 @@ def testSymbolFetcherNoVersionNumber(self):
                                              architecture='x86_64',
                                              bitness='64')
 
-    with self.assertRaisesRegex(Exception,
-                                'Failed to extract trace version number.'):
+    exception_msg = 'Failed to extract trace version number: ' + self.trace_file
+    with self.assertRaisesRegex(Exception, exception_msg):
       symbol_fetcher.GetTraceBreakpadSymbols(self.cloud_storage_bucket,
                                              metadata, self.breakpad_output_dir)
 
@@ -211,8 +213,9 @@ def testFetchBreakpadSymbolsWrongOS(self):
                                              architecture='x86_64',
                                              bitness='64')
 
-    with self.assertRaisesRegex(Exception,
-                                'Expected OS to be Linux or Mac: Android'):
+    exception_msg = ('Expected OS "Android" to be Linux or Mac: ' +
+                     self.trace_file)
+    with self.assertRaisesRegex(Exception, exception_msg):
       symbol_fetcher._FetchBreakpadSymbols(self.cloud_storage_bucket, metadata,
                                            self.breakpad_output_dir)
 

tools/tracing/symbolize_trace

@@ -8,7 +8,6 @@ This executable script symbolizes proto perfetto traces.
 """
 
 import optparse
-import logging
 import sys
 
 import symbolize_trace
@@ -36,9 +35,9 @@ def main():
   # Argument error checking
   trace_file = None
   if not args:
-    raise ValueError('Proto trace file argument is required.')
+    raise Exception('Proto trace file argument is required.')
   elif len(args) > 1:
-    raise ValueError('Too many arguments passed. Pass only one proto trace file.')
+    raise Exception('Too many arguments passed. Pass only one proto trace file.')
   else:
     trace_file = args[0]