Android: improve static initializer counting in resource_sizes.py.

build/android/resource_sizes.py

 #!/usr/bin/python
 # Copyright (c) 2011 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Prints the size of each given file and optionally computes the size of
    libchrome.so without the dependencies added for building with android NDK.
    Also breaks down the contents of the APK to determine the installed size
    and assign size contributions to different classes of file.
 """
 
 import collections
+from contextlib import contextmanager
 import json
 import logging
 import operator
 import optparse
 import os
 import re
-import shutil
 import struct
 import sys
 import tempfile
 import zipfile
 import zlib
 
 import devil_chromium
 from devil.android.sdk import build_tools
 from devil.utils import cmd_helper
 from devil.utils import lazy
 import method_count
 from pylib import constants
 from pylib.constants import host_paths
 
 _AAPT_PATH = lazy.WeakConstant(lambda: build_tools.GetPath('aapt'))
 _GRIT_PATH = os.path.join(host_paths.DIR_SOURCE_ROOT, 'tools', 'grit')
+_BUILD_UTILS_PATH = os.path.join(
+    host_paths.DIR_SOURCE_ROOT, 'build', 'android', 'gyp')
 
 # Prepend the grit module from the source tree so it takes precedence over other
 # grit versions that might present in the search path.
 with host_paths.SysPath(_GRIT_PATH, 1):
   from grit.format import data_pack # pylint: disable=import-error
 
 with host_paths.SysPath(host_paths.BUILD_COMMON_PATH):
   import perf_tests_results_helper # pylint: disable=import-error
 
+with host_paths.SysPath(_BUILD_UTILS_PATH, 1):
+  from util import build_utils # pylint: disable=import-error
+
 
 # Python had a bug in zipinfo parsing that triggers on ChromeModern.apk
 # https://bugs.python.org/issue14315
 def _PatchedDecodeExtra(self):
   # Try to decode the extra field.
   extra = self.extra
   unpack = struct.unpack
   while len(extra) >= 4:
     tp, ln = unpack('<HH', extra[:4])
     if tp == 1:
@@ skipping 53 matching lines @@
   'symbols': ['.dynsym', '.dynstr', '.dynamic', '.shstrtab', '.got', '.plt',
               '.got.plt', '.hash'],
   'bss': ['.bss'],
   'other': ['.init_array', '.fini_array', '.comment', '.note.gnu.gold-version',
             '.ARM.attributes', '.note.gnu.build-id', '.gnu.version',
             '.gnu.version_d', '.gnu.version_r', '.interp', '.gcc_except_table']
 }
 
 
 def _ExtractMainLibSectionSizesFromApk(apk_path, main_lib_path):
-  tmpdir = tempfile.mkdtemp(suffix='_apk_extract')
-  grouped_section_sizes = collections.defaultdict(int)
-  try:
-    with zipfile.ZipFile(apk_path, 'r') as z:
-      extracted_lib_path = z.extract(main_lib_path, tmpdir)
-      section_sizes = _CreateSectionNameSizeMap(extracted_lib_path)
+  with Unzip(apk_path, pattern=main_lib_path) as extracted_lib_path:
+    grouped_section_sizes = collections.defaultdict(int)
+    section_sizes = _CreateSectionNameSizeMap(extracted_lib_path)
+    for group_name, section_names in _READELF_SIZES_METRICS.iteritems():
+      for section_name in section_names:
+        if section_name in section_sizes:
+          grouped_section_sizes[group_name] += section_sizes.pop(section_name)
 
-      for group_name, section_names in _READELF_SIZES_METRICS.iteritems():
-        for section_name in section_names:
-          if section_name in section_sizes:
-            grouped_section_sizes[group_name] += section_sizes.pop(section_name)
+    # Group any unknown section headers into the "other" group.
+    for section_header, section_size in section_sizes.iteritems():
+      print "Unknown elf section header:", section_header
+      grouped_section_sizes['other'] += section_size
 
-      # Group any unknown section headers into the "other" group.
-      for section_header, section_size in section_sizes.iteritems():
-        print "Unknown elf section header:", section_header
-        grouped_section_sizes['other'] += section_size
-
-      return grouped_section_sizes
-  finally:
-    shutil.rmtree(tmpdir)
+    return grouped_section_sizes
 
 
 def _CreateSectionNameSizeMap(so_path):
   stdout = cmd_helper.GetCmdOutput(['readelf', '-S', '--wide', so_path])
   section_sizes = {}
   # Matches  [ 2] .hash HASH 00000000006681f0 0001f0 003154 04   A  3   0  8
   for match in re.finditer(r'\[[\s\d]+\] (\..*)$', stdout, re.MULTILINE):
     items = match.group(1).split()
     section_sizes[items[0]] = int(items[4], 16)
 
   return section_sizes
 
 
+def _ParseLibBuildId(so_path):
+  """Returns the Build ID of the given native library."""
+  stdout = cmd_helper.GetCmdOutput(['readelf', '-n', so_path])
+  match = re.search(r'Build ID: (\w{40})', stdout)

[agrieve, 2017/02/24 20:04:21]

nit: BuildIDs don't need to be 40 characters (although they should be when using
sha1, like we do). It would be better to just take the entire line (since we're
just comparing, there's no need to even strip the prefix, although if you want
to keep that in that's fine)

[estevenson, 2017/02/28 00:58:45]

Left it in. Done.

+  return match.group(1) if match else None
+
+
 def CountStaticInitializers(so_path):
   # Static initializers expected in official builds. Note that this list is
   # built using 'nm' on libchrome.so which results from a GCC official build
   # (i.e. Clang is not supported currently).
   def get_elf_section_size(readelf_stdout, section_name):
     # Matches: .ctors PROGBITS 000000000516add0 5169dd0 000010 00 WA 0 0 8
     match = re.search(r'\.%s.*$' % re.escape(section_name),
                       readelf_stdout, re.MULTILINE)
     if not match:
       return (False, -1)
@@ skipping 427 matching lines @@
   with zipfile.ZipFile(apk_filename) as z:
     infolist = z.infolist()
   out_dir = constants.GetOutDirectory()
   si_count = 0
   for zip_info in infolist:
     # Check file size to account for placeholder libraries.
     if zip_info.filename.endswith('.so') and zip_info.file_size > 0:
       lib_name = os.path.basename(zip_info.filename).replace('crazy.', '')
       unstripped_path = os.path.join(out_dir, 'lib.unstripped', lib_name)
       if os.path.exists(unstripped_path):
-        si_count += _PrintStaticInitializersCount(unstripped_path)
+        si_count += _PrintStaticInitializersCount(
+            apk_filename, zip_info.filename, unstripped_path)
       else:
         raise Exception('Unstripped .so not found. Looked here: %s',
                         unstripped_path)
   ReportPerfResult(chartjson, 'StaticInitializersCount', 'count', si_count,
                    'count')
 
 
-def _PrintStaticInitializersCount(so_with_symbols_path):
+def _PrintStaticInitializersCount(apk_path, apk_so_name, so_with_symbols_path):
   """Counts the number of static initializers in the given shared library.
      Additionally, files for which static initializers were found are printed
      on the standard output.
 
      Args:
-       so_with_symbols_path: Path to the unstripped libchrome.so file.
-
+      apk_path: Path to the apk.
+      apk_so_name: Name of the so.
+      so_with_symbols_path: Path to the unstripped libchrome.so file.
      Returns:
        The number of static initializers found.
   """
   # GetStaticInitializers uses get-static-initializers.py to get a list of all
   # static initializers. This does not work on all archs (particularly arm).
   # TODO(rnephew): Get rid of warning when crbug.com/585588 is fixed.
-  si_count = CountStaticInitializers(so_with_symbols_path)
+  with Unzip(apk_path, pattern="*%s" % apk_so_name) as unzipped_so:
+    _VerifyLibBuildIdsMatch(unzipped_so, so_with_symbols_path)
+    si_count = CountStaticInitializers(unzipped_so)
   static_initializers = GetStaticInitializers(so_with_symbols_path)
   static_initializers_count = len(static_initializers) - 1  # Minus summary.
   if si_count != static_initializers_count:
     print ('There are %d files with static initializers, but '
            'dump-static-initializers found %d:' %
            (si_count, static_initializers_count))
   else:
     print '%s - Found %d files with static initializers:' % (
         os.path.basename(so_with_symbols_path), si_count)
   print '\n'.join(static_initializers)
@@ skipping 28 matching lines @@
   graph_title = os.path.basename(apk_filename) + '_Dex'
   dex_metrics = method_count.CONTRIBUTORS_TO_DEX_CACHE
   for key, label in dex_metrics.iteritems():
     ReportPerfResult(chartjson, graph_title, label, sizes[key], 'entries')
 
   graph_title = '%sCache' % graph_title
   ReportPerfResult(chartjson, graph_title, 'DexCache', sizes['dex_cache_size'],
                    'bytes')
 
 
+@contextmanager
+def Unzip(zip_file, pattern=None, predicate=None):
+  """Utility for temporary use of a set of files in a zip archive."""
+  with build_utils.TempDir() as unzipped_dir:
+    unzipped_files = build_utils.ExtractAll(
+        zip_file, unzipped_dir, True, pattern=pattern, predicate=predicate)
+    yield unzipped_files[0] if len(unzipped_files) == 1 else unzipped_files
+
+
+def _VerifyLibBuildIdsMatch(*so_files):
+  if len(set(_ParseLibBuildId(f) for f in so_files)) > 1:
+    raise Exception('Found differing build ids in output directory and apk. '
+                    'Your output directory is likely stale.')
+
+
 def main(argv):
   usage = """Usage: %prog [options] file1 file2 ...
 
 Pass any number of files to graph their sizes. Any files with the extension
 '.apk' will be broken down into their components on a separate graph."""
   option_parser = optparse.OptionParser(usage=usage)
   option_parser.add_option('--so-path',
                            help='Obsolete. Pass .so as positional arg instead.')
   option_parser.add_option('--so-with-symbols-path',
                            help='Mostly obsolete. Use .so within .apk instead.')
@@ skipping 27 matching lines @@
 
   # For backward compatibilty with buildbot scripts, treat --so-path as just
   # another file to print the size of. We don't need it for anything special any
   # more.
   if options.so_path:
     files.append(options.so_path)
 
   if not files:
     option_parser.error('Must specify a file')
 
-  if options.so_with_symbols_path:
-    si_count = _PrintStaticInitializersCount(options.so_with_symbols_path)
-    ReportPerfResult(chartjson, 'StaticInitializersCount', 'count', si_count,
-                     'count')
-
   PrintResourceSizes(files, chartjson=chartjson)
 
   for f in files:
     if f.endswith('.apk'):
       PrintApkAnalysis(f, chartjson=chartjson)
       _PrintDexAnalysis(f, chartjson=chartjson)
       if not options.no_output_dir:
         PrintPakAnalysis(f, options.min_pak_resource_size)
-        if not options.so_with_symbols_path:
+        so_path = options.so_with_symbols_path

[agrieve, 2017/02/24 20:04:21]

This actually makes less sense inside the loop. However, this flag doesn't even
look to be used anymore (with one exception), so we should just make it a no-op.

Exception is here:
https://codereview.chromium.org/2718743002/

[estevenson, 2017/02/28 00:58:45]

I removed obsolete args and changed the script to just take 1 apk as an
argument, was there a reason to allow more?

+        if so_path:
+          si_count = _PrintStaticInitializersCount(
+              f, os.path.basename(so_path), so_path)
+          ReportPerfResult(
+              chartjson, 'StaticInitializersCount', 'count', si_count, 'count')
+        else:
           _PrintStaticInitializersCountFromApk(f, chartjson=chartjson)
 
   if chartjson:
     results_path = os.path.join(options.output_dir, 'results-chart.json')
     logging.critical('Dumping json to %s', results_path)
     with open(results_path, 'w') as json_file:
       json.dump(chartjson, json_file)
 
 
 if __name__ == '__main__':
   sys.exit(main(sys.argv))