simpleperf: fix binary_cache_builder.py.

libc.so/libart.so on device has .symtab but not .debug_line. In this
case, we still want to replace it with the unstripped binary.

Bug: none
Test: run test.py.
Change-Id: I0308e7fafe1f41c2e57870cf72355dc8151ed9fd

3 files changed, 91 insertions, 47 deletions

diff --git a/simpleperf/scripts/binary_cache_builder.py b/simpleperf/scripts/binary_cache_builder.py
index 1f4041f8..6151d16f 100755
--- a/simpleperf/scripts/binary_cache_builder.py
+++ b/simpleperf/scripts/binary_cache_builder.py
@@ -26,47 +26,36 @@ import os.path
 import shutil
 
 from simpleperf_report_lib import ReportLib
-from utils import AdbHelper, flatten_arg_list, log_info, log_warning, log_exit, ReadElf
+from utils import AdbHelper, extant_dir, extant_file, flatten_arg_list, log_info, log_warning
+from utils import ReadElf
 
 def is_jit_symfile(dso_name):
     return dso_name.split('/')[-1].startswith('TemporaryFile')
 
 class BinaryCacheBuilder(object):
     """Collect all binaries needed by perf.data in binary_cache."""
-    def __init__(self, config):
-        config_names = ['perf_data_path', 'symfs_dirs', 'ndk_path']
-        for name in config_names:
-            if name not in config:
-                log_exit('config for "%s" is missing' % name)
-
-        self.perf_data_path = config.get('perf_data_path')
-        if not os.path.isfile(self.perf_data_path):
-            log_exit("can't find file %s" % self.perf_data_path)
-        self.symfs_dirs = config.get('symfs_dirs')
-        for symfs_dir in self.symfs_dirs:
-            if not os.path.isdir(symfs_dir):
-                log_exit("symfs_dir '%s' is not a directory" % symfs_dir)
-        self.adb = AdbHelper(enable_switch_to_root=not config['disable_adb_root'])
-        self.readelf = ReadElf(config.get('ndk_path'))
+    def __init__(self, ndk_path, disable_adb_root):
+        self.adb = AdbHelper(enable_switch_to_root=not disable_adb_root)
+        self.readelf = ReadElf(ndk_path)
         self.binary_cache_dir = 'binary_cache'
         if not os.path.isdir(self.binary_cache_dir):
             os.makedirs(self.binary_cache_dir)
         self.binaries = {}
 
 
-    def build_binary_cache(self):
-        self._collect_used_binaries()
-        self._copy_binaries_from_symfs_dirs()
+    def build_binary_cache(self, perf_data_path, symfs_dirs):
+        self._collect_used_binaries(perf_data_path)
+        self.copy_binaries_from_symfs_dirs(symfs_dirs)
         self._pull_binaries_from_device()
         self._pull_kernel_symbols()
 
 
-    def _collect_used_binaries(self):
+    def _collect_used_binaries(self, perf_data_path):
         """read perf.data, collect all used binaries and their build id (if available)."""
         # A dict mapping from binary name to build_id
         binaries = {}
         lib = ReportLib()
-        lib.SetRecordFile(self.perf_data_path)
+        lib.SetRecordFile(perf_data_path)
         lib.SetLogSeverity('error')
         while True:
             sample = lib.GetNextSample()
@@ -87,9 +76,9 @@ class BinaryCacheBuilder(object):
         self.binaries = binaries
 
 
-    def _copy_binaries_from_symfs_dirs(self):
+    def copy_binaries_from_symfs_dirs(self, symfs_dirs):
         """collect all files in symfs_dirs."""
-        if not self.symfs_dirs:
+        if not symfs_dirs:
             return
 
         # It is possible that the path of the binary in symfs_dirs doesn't match
@@ -110,7 +99,7 @@ class BinaryCacheBuilder(object):
             paths.append(binary)
 
         # Walk through all files in symfs_dirs, and copy matching files to build_cache.
-        for symfs_dir in self.symfs_dirs:
+        for symfs_dir in symfs_dirs:
             for root, _, files in os.walk(symfs_dir):
                 for filename in files:
                     paths = filename_dict.get(filename)
@@ -132,7 +121,7 @@ class BinaryCacheBuilder(object):
             target_file = target_file[1:]
         target_file = target_file.replace('/', os.sep)
         target_file = os.path.join(self.binary_cache_dir, target_file)
-        if not self._need_to_copy(target_file, expected_build_id):
+        if not self._need_to_copy(from_path, target_file, expected_build_id):
             # The existing file in binary_cache can provide more information, so no need to copy.
             return
         target_dir = os.path.dirname(target_file)
@@ -142,14 +131,23 @@ class BinaryCacheBuilder(object):
         shutil.copy(from_path, target_file)
 
 
-    def _need_to_copy(self, target_file, expected_build_id):
+    def _need_to_copy(self, source_file, target_file, expected_build_id):
         if not os.path.isfile(target_file):
             return True
         if self._read_build_id(target_file) != expected_build_id:
             return True
-        if not self._file_has_symbol_table(target_file):
-            return True
-        return False
+        return self._get_file_stripped_level(source_file) < self._get_file_stripped_level(
+            target_file)
+
+
+    def _get_file_stripped_level(self, file_path):
+        """Return stripped level of an ELF file. Larger value means more stripped."""
+        sections = self.readelf.get_sections(file_path)
+        if '.debug_line' in sections:
+            return 0
+        if '.symtab' in sections:
+            return 1
+        return 2
 
 
     def _pull_binaries_from_device(self):
@@ -192,11 +190,6 @@ class BinaryCacheBuilder(object):
         return self.readelf.get_build_id(file_path)
 
 
-    def _file_has_symbol_table(self, file_path):
-        """Test if an elf file has symbol table section."""
-        return '.symtab' in self.readelf.get_sections(file_path)
-
-
     def _pull_file_from_device(self, device_path, host_path):
         if self.adb.run(['pull', device_path, host_path]):
             return True
@@ -223,22 +216,19 @@ class BinaryCacheBuilder(object):
 def main():
     parser = argparse.ArgumentParser(description="""
         Pull binaries needed by perf.data from device to binary_cache directory.""")
-    parser.add_argument('-i', '--perf_data_path', default='perf.data', help="""
+    parser.add_argument('-i', '--perf_data_path', default='perf.data', type=extant_file, help="""
         The path of profiling data.""")
-    parser.add_argument('-lib', '--native_lib_dir', nargs='+', help="""
+    parser.add_argument('-lib', '--native_lib_dir', type=extant_dir, nargs='+', help="""
         Path to find debug version of native shared libraries used in the app.""", action='append')
     parser.add_argument('--disable_adb_root', action='store_true', help="""
         Force adb to run in non root mode.""")
     parser.add_argument('--ndk_path', nargs=1, help='Find tools in the ndk path.')
     args = parser.parse_args()
-    config = {}
-    config['perf_data_path'] = args.perf_data_path
-    config['symfs_dirs'] = flatten_arg_list(args.native_lib_dir)
-    config['disable_adb_root'] = args.disable_adb_root
-    config['ndk_path'] = None if not args.ndk_path else args.ndk_path[0]
-
-    builder = BinaryCacheBuilder(config)
-    builder.build_binary_cache()
+
+    ndk_path = None if not args.ndk_path else args.ndk_path[0]
+    builder = BinaryCacheBuilder(ndk_path, args.disable_adb_root)
+    symfs_dirs = flatten_arg_list(args.native_lib_dir)
+    builder.build_binary_cache(args.perf_data_path, symfs_dirs)
 
 
 if __name__ == '__main__':
diff --git a/simpleperf/scripts/test.py b/simpleperf/scripts/test.py
index a0fb41f6..d0d2ff2a 100755
--- a/simpleperf/scripts/test.py
+++ b/simpleperf/scripts/test.py
@@ -36,6 +36,7 @@ Test using both `adb root` and `adb unroot`.
 """
 from __future__ import print_function
 import argparse
+import filecmp
 import fnmatch
 import inspect
 import os
@@ -49,10 +50,11 @@ import types
 import unittest
 
 from app_profiler import NativeLibDownloader
+from binary_cache_builder import BinaryCacheBuilder
 from simpleperf_report_lib import ReportLib
 from utils import log_exit, log_info, log_fatal
-from utils import AdbHelper, Addr2Nearestline, get_script_dir, is_windows, Objdump, ReadElf, remove
-from utils import SourceFileSearcher
+from utils import AdbHelper, Addr2Nearestline, find_tool_path, get_script_dir, is_windows, Objdump
+from utils import ReadElf, remove, SourceFileSearcher
 
 try:
     # pylint: disable=unused-import
@@ -1221,6 +1223,39 @@ class TestReportHtml(TestBase):
         self.run_cmd(['report_html.py', '-i', 'testdata/perf_with_long_callchain.data'])
 
 
+class TestBinaryCacheBuilder(TestBase):
+    def test_copy_binaries_from_symfs_dirs(self):
+        readelf = ReadElf(None)
+        strip = find_tool_path('strip', arch='arm')
+        self.assertIsNotNone(strip)
+        symfs_dir = os.path.join('testdata', 'symfs_dir')
+        remove(symfs_dir)
+        os.mkdir(symfs_dir)
+        filename = 'simpleperf_runtest_two_functions_arm'
+        origin_file = os.path.join('testdata', filename)
+        source_file = os.path.join(symfs_dir, filename)
+        target_file = os.path.join('binary_cache', filename)
+        expected_build_id = readelf.get_build_id(origin_file)
+        binary_cache_builder = BinaryCacheBuilder(None, False)
+        binary_cache_builder.binaries['simpleperf_runtest_two_functions_arm'] = expected_build_id
+
+        # Copy binary if target file doesn't exist.
+        remove(target_file)
+        self.run_cmd([strip, '--strip-all', '-o', source_file, origin_file])
+        binary_cache_builder.copy_binaries_from_symfs_dirs([symfs_dir])
+        self.assertTrue(filecmp.cmp(target_file, source_file))
+
+        # Copy binary if target file doesn't have .symtab and source file has .symtab.
+        self.run_cmd([strip, '--strip-debug', '-o', source_file, origin_file])
+        binary_cache_builder.copy_binaries_from_symfs_dirs([symfs_dir])
+        self.assertTrue(filecmp.cmp(target_file, source_file))
+
+        # Copy binary if target file doesn't have .debug_line and source_files has .debug_line.
+        shutil.copy(origin_file, source_file)
+        binary_cache_builder.copy_binaries_from_symfs_dirs([symfs_dir])
+        self.assertTrue(filecmp.cmp(target_file, source_file))
+
+
 def get_all_tests():
     tests = []
     for name, value in globals().items():
@@ -1255,6 +1290,8 @@ def main():
             if pattern.match(test):
                 new_tests.append(test)
         tests = new_tests
+        if not tests:
+            log_exit('No tests are matched.')
 
     os.chdir(get_script_dir())
     build_testdata()
diff --git a/simpleperf/scripts/utils.py b/simpleperf/scripts/utils.py
index 3abcfce2..7ae6cc98 100644
--- a/simpleperf/scripts/utils.py
+++ b/simpleperf/scripts/utils.py
@@ -19,6 +19,7 @@
 """
 
 from __future__ import print_function
+import argparse
 import logging
 import os
 import os.path
@@ -147,6 +148,9 @@ EXPECTED_TOOLS = {
     'objdump': {
         'is_binutils': True,
     },
+    'strip': {
+        'is_binutils': True,
+    },
 }
 
 def _get_binutils_path_in_ndk(toolname, arch, platform):
@@ -773,8 +777,21 @@ def extant_dir(arg):
     """
     path = os.path.realpath(arg)
     if not os.path.isdir(path):
-        import argparse
         raise argparse.ArgumentTypeError('{} is not a directory.'.format(path))
     return path
 
+def extant_file(arg):
+    """ArgumentParser type that only accepts extant files.
+
+    Args:
+        arg: The string argument given on the command line.
+    Returns: The argument as a realpath.
+    Raises:
+        argparse.ArgumentTypeError: The given path isn't a file.
+    """
+    path = os.path.realpath(arg)
+    if not os.path.isfile(path):
+        raise argparse.ArgumentTypeError('{} is not a file.'.format(path))
+    return path
+
 logging.getLogger().setLevel(logging.DEBUG)