binary_size_tool: fix for ambiguous addr2line output

build/android/pylib/symbols/elf_symbolizer.py

Index: build/android/pylib/symbols/elf_symbolizer.py
diff --git a/build/android/pylib/symbols/elf_symbolizer.py b/build/android/pylib/symbols/elf_symbolizer.py
index b294654f20b1ee3c7f0cdb5c142789f3bc1b5933..70c9c9ab5797a7824c32073d6523fa5a7a25aff9 100644
--- a/build/android/pylib/symbols/elf_symbolizer.py
+++ b/build/android/pylib/symbols/elf_symbolizer.py
@@ -14,7 +14,6 @@ import subprocess
 import sys
 import threading
 
-

[Primiano Tucci (use gerrit), 2014/06/18 15:11:10]

Nit: Keep the extra blankline here. Code styles provides 2 \n between top levels
IIRC

 # addr2line builds a possibly infinite memory cache that can exhaust
 # the computer's memory if allowed to grow for too long. This constant
 # controls how many lookups we do before restarting the process. 4000
@@ -75,7 +74,8 @@ class ELFSymbolizer(object):
   """
 
   def __init__(self, elf_file_path, addr2line_path, callback, inlines=False,
-      max_concurrent_jobs=None, addr2line_timeout=30, max_queue_size=50):
+      max_concurrent_jobs=None, addr2line_timeout=30, max_queue_size=50,
+      source_root_path=None, strip_base_path=None):
     """Args:
       elf_file_path: path of the elf file to be symbolized.
       addr2line_path: path of the toolchain's addr2line binary.
@@ -91,6 +91,16 @@ class ELFSymbolizer(object):
       max_queue_size: Max number of outstanding requests per addr2line instance.
       addr2line_timeout: Max time (in seconds) to wait for a addr2line response.
           After the timeout, the instance will be considered hung and respawned.
+      disambiguate: Whether to run a disambiguation process or not.

[Primiano Tucci (use gerrit), 2014/06/18 15:11:10]

Please update the doc here with the renamed variables.

+          Disambiguation means to resolve ambiguous source_paths, for
+          example turn addr2line output "unicode.cc" into a full and absolute
+          path. In some toolchains only the name of the source file is output,
+          without any path information; disambiguation searches through the
+          source directory specified by 'disambiguate_source_path' argument
+          for files whose name matches. If there are multiple files with the
+          same name, disambiguation will fail.
+      disambiguate_source_path: The path to the directory where the source
+          files are located, used for disambiguating paths.
     """
     assert(os.path.isfile(addr2line_path)), 'Cannot find ' + addr2line_path
     self.elf_file_path = elf_file_path
@@ -104,6 +114,14 @@ class ELFSymbolizer(object):
     self.requests_counter = 0  # For generating monotonic request IDs.
     self._a2l_instances = []  # Up to |max_concurrent_jobs| _Addr2Line inst.
 
+    # If necessary, create disambiguation lookup table
+    self.disambiguate = source_root_path is not None

[Primiano Tucci (use gerrit), 2014/06/18 15:11:10]

You don't seem to make any use of self.disambiguate.
Can you just directly use source_root_path in the check.
Also, typically it's nicer to be concise in python, unless there is a functional
need for it (if foo vs if foo is not None).
In essence, remove 118, and turn line 122 into
if source_root_path:

+    self.lookup_table = {}

[Primiano Tucci (use gerrit), 2014/06/18 15:11:10]

nit: _lookup_table (add _ prefix) as this is a private detail.

+    self.source_root_path = source_root_path
+    self.strip_base_path = strip_base_path
+    if(self.disambiguate):
+      self._CreateDisambiguationTable(source_root_path)

[Primiano Tucci (use gerrit), 2014/06/18 15:11:10]

Ehm, if you store source_root_path as a field into self, passing that as an
argument of _CreateDisambiguationTable feels redundant. Just use
self.source_root_path in _CreateDisambiguationTable, no?

+
     # Create one addr2line instance. More instances will be created on demand
     # (up to |max_concurrent_jobs|) depending on the rate of the requests.
     self._CreateNewA2LInstance()
@@ -161,6 +179,26 @@ class ELFSymbolizer(object):
     self._a2l_instances.append(a2l)
     return a2l
 
+  def _CreateDisambiguationTable(self, src_root_path):

[Primiano Tucci (use gerrit), 2014/06/18 15:11:10]

Is there a reason why you couldn't use those 4 lines of code I pasted in the
previous comment?
This seems a bit redundant and unnecessarily verbose: you populate the table,
keep track of duplicates, and then remove the duplicates from the table

+    """ Creates a table of files used for disambiguation later

[Primiano Tucci (use gerrit), 2014/06/18 15:11:10]

I think this comment is pleonastic.
It tells that _CreateDisambiguationTable creates a table of files used for
disambiguation.
The name is explicative enough itself.
Also, doesn't matter if you write that this piece of code is andrewhayden's. If
something goes wrong people will git blame and come back to you anyways, not
Andrew :)
In essence, remove this """ comment here. It makes the file longer without
providing any useful information.

+        adopted from andrewhaydens implementation in earlier commits """
+    duplicates = set()
+    self.lookup_table = {}
+    src_root_path = os.path.abspath(src_root_path)
+
+    for root, _, filenames in os.walk(src_root_path):
+      for f in filenames:
+        base = os.path.basename(f)  # Just in case
+        if self.lookup_table.get(base) is None:
+          self.lookup_table[base] = os.path.join(root, f)
+        else:
+          duplicates.add(base)
+
+    # Duplicates can not be used for disambiguation, as we can not determine
+    # the true source if we have more than one to choose from
+    for d in duplicates:
+      del self.lookup_table[d]
+
 
   class Addr2Line(object):
     """A python wrapper around an addr2line instance.
@@ -312,7 +350,27 @@ class ELFSymbolizer(object):
         else:
           logging.warning('Got invalid symbol path from addr2line: %s' % line2)
 
-        sym_info = ELFSymbolInfo(name, source_path, source_line)
+        # In case disambiguation is on, and needed
+        disambiguated = False

[Primiano Tucci (use gerrit), 2014/06/18 15:11:11]

I think that Andrew suggested that the two bools are unnecessary?

+        failed_disambiguation = False
+        if self._symbolizer.disambiguate:
+          if source_path and not source_path.startswith('/'):

[Primiano Tucci (use gerrit), 2014/06/18 15:11:10]

I think this line is completely unnecessary.
Just look if you have the entry in the table. If you have it in the table, the
path must have been necessarily != None and a relative path.
I.s., this should just be
source_path = self._symbolizer.lookup_table.get(source_path) or source_path

+            source_path = self._symbolizer.lookup_table.get(source_path)
+            failed_disambiguation = source_path is None

[Primiano Tucci (use gerrit), 2014/06/18 15:11:10]

I'm not sure that you need at all to keep failed_disambiguation and the
failed_disambiguated argument.
What you really might want to do, on the caller side, is checking how many file
paths that you got from here exists in the filesystem (which means that you
either disambiguated or stripped succesfully)

+            disambiguated = not failed_disambiguation
+
+        if source_path is not None:

[Primiano Tucci (use gerrit), 2014/06/18 15:11:10]

nit: if source_path:

+          # Strip the base path

[Primiano Tucci (use gerrit), 2014/06/18 15:11:10]

I think that lines 363-368 can be expressed  with a more concise:
source_path = re.sub('^' + self._symbolizer.strip_base_path,
self._symbolizer.source_root_path or '', source_path)

which given the nice names of the variable is wonderfully concise and
self-explicative.

+          strip = self._symbolizer.strip_base_path
+          if strip is not None and source_path.startswith(strip):
+            source_path = source_path[len(strip):] # Remove strip
+            source_path = os.path.join(self._symbolizer.source_root_path,
+                                       source_path)
+          # Use the absolute path
+          source_path = os.path.abspath(source_path)

[Primiano Tucci (use gerrit), 2014/06/18 15:11:11]

Good catch but move to line 120  in __init__ where you assign the var.

+
+        sym_info = ELFSymbolInfo(name, source_path, source_line, disambiguated,
+                                 failed_disambiguation)
         if prev_sym_info:
           prev_sym_info.inlined_by = sym_info
         if not innermost_sym_info:
@@ -393,7 +451,8 @@ class ELFSymbolizer(object):
 class ELFSymbolInfo(object):
   """The result of the symbolization passed as first arg. of each callback."""
 
-  def __init__(self, name, source_path, source_line):
+  def __init__(self, name, source_path, source_line, disambiguated=False,
+               failed_disambiguation=False):
     """All the fields here can be None (if addr2line replies with '??')."""
     self.name = name
     self.source_path = source_path
@@ -401,6 +460,8 @@ class ELFSymbolInfo(object):
     # In the case of |inlines|=True, the |inlined_by| points to the outer
     # function inlining the current one (and so on, to form a chain).
     self.inlined_by = None
+    self.disambiguated = disambiguated
+    self.failed_disambiguation = failed_disambiguation
 
   def __str__(self):
     return '%s [%s:%d]' % (