Fix the stack script issue when symbolizing tombstones.

build/android/pylib/symbols/stack_symbolizer.py

Index: build/android/pylib/symbols/stack_symbolizer.py
diff --git a/build/android/pylib/symbols/stack_symbolizer.py b/build/android/pylib/symbols/stack_symbolizer.py
new file mode 100644
index 0000000000000000000000000000000000000000..f293ead00df3cc6af454f61e9a92c48d38ef97c9
--- /dev/null
+++ b/build/android/pylib/symbols/stack_symbolizer.py
@@ -0,0 +1,105 @@
+# Copyright 2017 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.
+
+import logging
+import os
+import re
+import shutil
+import tempfile
+
+from devil.utils import cmd_helper
+from pylib import constants
+
+STACK_TOOL = os.path.join(os.path.dirname(__file__), '..', '..', '..', '..',

[agrieve, 2017/07/26 01:26:10]

nit: prefix with _

[BigBossZhiling, 2017/07/26 17:36:26]

Done.

+                          'third_party', 'android_platform', 'development',
+                          'scripts', 'stack')
+ABI_REG = re.compile('ABI: \'(.+?)\'')
+

[agrieve, 2017/07/26 01:26:10]

nit: 2 blank lines

[BigBossZhiling, 2017/07/26 17:36:26]

Done.

+def _DeviceAbiToArch(device_abi):
+    # The order of this list is significant to find the more specific match
+    # (e.g., arm64) before the less specific (e.g., arm).
+    arches = ['arm64', 'arm', 'x86_64', 'x86_64', 'x86', 'mips']
+    for arch in arches:
+      if arch in device_abi:
+        return arch
+    raise RuntimeError('Unknown device ABI: %s' % device_abi)
+
+
+class Symbolizer(object):
+  """ A helper class to symbolize stack. """

[agrieve, 2017/07/26 01:26:10]

nit: remove spaces before/after """

[BigBossZhiling, 2017/07/26 17:36:27]

Done.

+
+  def __init__(self, apk_under_test=None,
+               enable_relocation_packing=None):
+    self.apk_under_test = apk_under_test

[agrieve, 2017/07/26 01:26:10]

nit: prefix with _ unless you need these to be public

[BigBossZhiling, 2017/07/26 17:36:26]

Done.

+    self.enable_relocation_packing = enable_relocation_packing
+    self.libs_dir = None
+    self.apk_libs = []
+    self.has_unzipped = False
+
+
+  def __del__(self):
+    self.CleanUp()

[agrieve, 2017/07/26 01:26:10]

Maybe log a warning if CleanUp() has not already been called a this point?

[BigBossZhiling, 2017/07/26 17:36:27]

Done.

+
+
+  def CleanUp(self):
+    """ Clean up the temporary directory of apk libs. """
+    if self.libs_dir:
+      shutil.rmtree(self.libs_dir)
+      self.libs_dir = None
+
+
+  def UnzipAPKIfNecessary(self):
+    """ Unzip apk if packed relocation is enabled. """
+    if (self.has_unzipped
+        or not self.enable_relocation_packing
+        or not self.apk_under_test):
+      return
+    self.libs_dir = tempfile.mkdtemp()
+    cmd_helper.GetCmdOutput(

[agrieve, 2017/07/26 01:26:10]

Python's zipfile module makes extracting easy. You should prefer it over calling
unzip since it's more portable & easier to get right.

Example:
https://cs.chromium.org/chromium/src/build/android/gyp/util/build_utils.py?q=...

[BigBossZhiling, 2017/07/26 17:36:26]

Done.

+        ['unzip', '-o', self.apk_under_test, '-d', self.libs_dir])
+    self.has_unzipped = True
+    if os.path.exists(os.path.join(self.libs_dir, 'lib')):
+      for root, _, files in os.walk(os.path.join(self.libs_dir,
+                                                 'lib')):
+        for file_name in files:
+          if file_name.endswith('.so'):
+            self.apk_libs.append(os.path.join(root, file_name))
+
+
+  def ResolveSymbols(self, data_to_symbolize, device_abi, include_stack=True):

[agrieve, 2017/07/26 01:26:10]

This is somewhat misleading. While the stack tool does symbolize, it also
extracts stacks (all non-stack output is removed). Maybe call it
"ExtractAndResolveNativeStackTraces"

[BigBossZhiling, 2017/07/26 17:36:26]

Done.

+    """Run the stack tool for given input.
+
+    Args:
+      data_to_symbolize: a list of strings to symbolize.
+      include_stack: boolean whether to include stack data in output.
+      device_abi: the default ABI of the device which generated the tombstone.
+
+    Yields:
+      A string for each line of resolved stack output.
+    """
+    self.UnzipAPKIfNecessary()
+    # Check if the data_to_symbolize has an ABI listed,
+    # if so use this in preference to the device's default ABI.
+    for line in data_to_symbolize:
+      found_abi = ABI_REG.match(line)

[agrieve, 2017/07/26 01:26:10]

The stack script already does this logic:
https://cs.chromium.org/chromium/src/third_party/android_platform/development...

Is it necessary to do it here as well?

[BigBossZhiling, 2017/07/26 17:36:26]

Done.

+      if found_abi:
+        device_abi = found_abi.group(1)
+    arch = _DeviceAbiToArch(device_abi)
+    if not arch:
+      logging.warning('No device_abi can be found.')
+      return
+
+    cmd = [STACK_TOOL, '--arch', arch, '--output-directory',
+           constants.GetOutDirectory()]
+    if self.enable_relocation_packing and self.apk_libs:
+      for apk_lib in self.apk_libs:
+        cmd.extend(['--packed-lib', apk_lib])
+    with tempfile.NamedTemporaryFile() as f:
+      f.write('\n'.join(data_to_symbolize))
+      f.flush()
+      _, output = cmd_helper.GetCmdStatusAndOutput(cmd + [f.name])
+    for line in output.splitlines():
+      if not include_stack and 'Stack Data:' in line:

[agrieve, 2017/07/26 01:26:10]

Won't this skip over other stack traces that are in the file? Or are tombstones
guaranteed to have only one? 

[BigBossZhiling, 2017/07/26 17:36:27]

My understanding is that stack trace and stack data are two different things.
https://luci-logdog.appspot.com/v/?s=chromium%2Fandroid%2Fswarming%2Flogcats%...
You will see stack traces in the sample resolved tombstone there. There are
multiple of them. We only filter out Stack Data if it exists. 

[agrieve, 2017/07/26 17:50:38]

Actually, it looks like these lines are added only when --more-info is passed to
stack script. Probably don't need to post-process at all.

[BigBossZhiling, 2017/07/26 18:36:58]

Acknowledged. I didn't notice there is a more-info tag. Thanks for pointing it
out. Instead I am adding the --more-info to the call of stack script and keep
the check statement here. Does that sound good?

+        break
+      yield line