Refactor the symbol parsing, and move to NDK's nm.

tools/cygprofile/symbols_extractor.py

+#!/usr/bin/python
+# Copyright 2015 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.
+
+"""Utilities to get and manipulate symbols from a binary."""
+
+import collections
+import os
+import subprocess
+import sys
+
+sys.path.append(os.path.join(sys.path[0], '..', '..', 'build', 'android'))
+from pylib import constants
+
+
+# TODO(lizeb): Fix this when the hardcoded assumptions (ARM 32bits, linux

[pasko, 2015/01/28 14:37:01]

Can this be trivially reused via ToolPath() from
third_party/android_platform/development/scripts/symbol.py ?

That's not an ideal solution, but seems that's what we are using on bots and
crash reports.

[Benoit L, 2015/01/28 15:20:41]

Thank you for the tip !
Done.

+# x86_64) are no longer true. Even better, encapsulate the
+# architecture-dependent bits.
+_NM_BINARY = os.path.join(constants.ANDROID_NDK_ROOT, 'toolchains',
+                          'arm-linux-androideabi-4.9', 'prebuilt',
+                          'linux-x86_64', 'bin', 'arm-linux-androideabi-nm')
+
+__SymbolInfo = collections.namedtuple(

[pasko, 2015/01/28 14:37:01]

Two blank lines between top-level definitions:
https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Blank...

[Benoit L, 2015/01/28 15:20:41]

Done.

+    '__SymbolInfo', ('name', 'offset', 'size'))
+
+class SymbolInfo(__SymbolInfo):

[pasko, 2015/01/28 14:37:01]

Making this a class complicates reading without clear benefits. I was originally
not familiar with the concept of __slots__, and it's not clear why I should be
thinking about the immutability of this class. Inheritance adds extra ovehead
when reading this.

A function FromNmLine() to return a namedtuple, would have been nicer to
not-very-pythonic readers, whom we have plenty.

$ git grep namedtuple | grep -v third_party/ | wc -l
57

$ git grep __slots__ | grep -v third_party/ | wc -l
5

[Benoit L, 2015/01/28 15:20:41]

Done.

+  """Represent the information gathered from a symbol.
+  """
+  __slots__ = ()
+
+  @classmethod
+  def fromNmLine(cls, line):
+    """Create a SymbolInfo by parsing a properly formatted nm output line.
+
+    Args:
+      line: line from nm
+
+    Returns:
+      An instance of SymbolInfo if the line represent a symbol, None otherwise.

[pasko, 2015/01/28 14:37:01]

s/represent/represents/

[Benoit L, 2015/01/28 15:20:41]

Done.

+    """
+    # We are interested in two types of lines:
+    # This:
+    # 00210d59 00000002 t _ZN34BrowserPluginHostMsg_Attach_ParamsD2Ev
+    # offset size <symbol_type> symbol_name
+    # And that:
+    # 0070ee8c T WebRtcSpl_ComplexBitReverse
+    # In the second case we don't have a size, so use -1 as a sentinel
+    if not any(x in line for x in (' t ', ' W ', ' T ')):

[pasko, 2015/01/28 14:37:02]

I realize it was easy to write this line, but reading it, being done from right
to left, is not handy.

Matching against two regexps seems to be more self-explanatory, and if nothing
matched, just return None.

[Benoit L, 2015/01/28 15:20:41]

Done.

+      return None
+    parts = line.split()
+    if len(parts) == 4:
+      return SymbolInfo(
+          offset=int(parts[0], 16), size=int(parts[1], 16), name=parts[3])
+    elif len(parts) == 3:
+      return SymbolInfo(
+          offset=int(parts[0], 16), size=-1, name=parts[2])
+    else:
+      return None
+
+
+def SymbolInfosFromStream(nm_lines):
+  """Parses the output of nm, and get all the symbols from a binary.
+
+  Args:
+    nm_lines: An iterable of lines
+
+  Returns:
+    The same output as GetSymbolsFromBinary.

[pasko, 2015/01/28 14:37:01]

did you mean SymbolInfosFromBinary? Saying that it's 'a list of SymbolInfo'
would be easier to read (less random seeks, you know)

[Benoit L, 2015/01/28 15:20:41]

Done.

+  """
+  # TODO(lizeb): Consider switching to objdump to simplify parsing.
+  symbol_infos = []
+  for line in nm_lines:
+    if not any(x in line for x in (' t ', ' W ', ' T ')):

[pasko, 2015/01/28 14:37:02]

why checking it again here? the checkign for None two lines below does the job

[Benoit L, 2015/01/28 15:20:41]

Done.

+      continue
+    symbol_info = SymbolInfo.fromNmLine(line)
+    if symbol_info:
+      symbol_infos.append(symbol_info)
+  return symbol_infos
+
+
+def SymbolInfosFromBinary(binary_filename):
+  """Runs nm to get all the symbols from a binary.
+
+  Args:
+    binary_filename: path to the binary.
+
+  Returns:
+    A list of symbols from the binary.
+  """
+  command = (_NM_BINARY, '-S', '-n', binary_filename)
+  p = subprocess.Popen(command, shell=False, stdout=subprocess.PIPE)
+  try:
+    result = SymbolInfosFromStream(p.stdout)
+    return result
+  finally:
+    p.wait()
+
+
+def OffsetToSymbolInfos(symbol_infos):

[pasko, 2015/01/28 14:37:02]

GroupSymbolInfosByOffset? GroupSymbolsByOffset? GroupByOffset?

the name OffsetToSomething suggests that offset would be a parameter, misleading

[Benoit L, 2015/01/28 15:20:41]

Done.

+  """Create a dict {offset: [symbol_info1, ...], ...}.
+
+  As several symbols can be at the same offset, this is a 1-to-many
+  relationship.
+
+  Args:
+    symbol_infos: iterable of SymbolInfo instances
+
+  Returns:
+    a dict {offset: [symbol_info1, ...], ...}
+  """
+  offset_to_symbol_infos = collections.defaultdict(list)
+  for symbol_info in symbol_infos:
+    offset_to_symbol_infos[symbol_info.offset].append(symbol_info)
+  return dict(offset_to_symbol_infos)
+
+
+def NameToSymbolInfo(symbol_infos):

[pasko, 2015/01/28 14:37:01]

CreateNameToSymbolInfo?

[Benoit L, 2015/01/28 15:20:41]

Done.

+  """Create a dict {name: symbol_info, ...}.
+
+  Args:
+    symbol_infos: iterable of SymbolInfo instances
+
+  Returns:
+    a dict {name: symbol_info, ...}
+  """
+  return {symbol_info.name: symbol_info for symbol_info in symbol_infos}