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..50933c555269aefdc0827945aed38eecf693f891 100644
--- a/build/android/pylib/symbols/elf_symbolizer.py
+++ b/build/android/pylib/symbols/elf_symbolizer.py
@@ -75,7 +75,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 +92,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.
+      source_root_path: The path to the library source code is used in

[Andrew Hayden (chromium.org), 2014/06/26 09:00:11]

Grammar here still needs to be cleaned up a bit. I think the original
description I provided is sufficient here, so let's move your example to the end
instead of the beginning of this text. How about this?

In some toolchains only the name of the source
file is output, without any path information; disambiguation searches
through the source directory specified by |source_root_path| argument
for files whose name matches, adding the full path information to the
output. For example, if the toolchain outputs "unicode.cc" and there
is a file called "unicode.cc" located under |source_root_path|/foo/bar,
the tool will replace "unicode.cc" with
"|source_root_path|/foo/bar/unicode.cc". If there are multiple files
with the same name, disambiguation will fail because the tool cannot
determine which of the files was the source of the symbol.

+          the disambiguation process. Disambiguation means to resolve ambiguous
+          source paths, for example turn addr2line output "unicode.cc" into a
+          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 |source_root_path| argument
+          for files whose name matches. If there are multiple files with the
+          same name, disambiguation will fail.
+      strip_base_path: Rebases the symbols source paths onto |source_root_path|
+          (i.e. s/^strip_base_path/source_root_path/).

[Andrew Hayden (chromium.org), 2014/06/26 09:00:11]

This is a nice succinct explanation, though the use of search and replace syntax
here may be confusing for folks who don't use Linux/Mac... but since it's an ELF
symbolizer, I guess that's ok :P :P

Still for clarity and accessibility to a wider audience you might want to
provide an example.

[Primiano Tucci (use gerrit), 2014/06/27 08:53:56]

I find the idea that a developer is not familiar with the regex syntax
terrifying, but I'm fine either ways  ;-) 

     """
     assert(os.path.isfile(addr2line_path)), 'Cannot find ' + addr2line_path
     self.elf_file_path = elf_file_path
@@ -104,6 +115,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
+    self.disambiguation_table = {}
+    self.strip_base_path = strip_base_path
+    if(self.disambiguate):
+      self.source_root_path = os.path.abspath(source_root_path)
+      self._CreateDisambiguationTable()
+
     # 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 +180,15 @@ class ELFSymbolizer(object):
     self._a2l_instances.append(a2l)
     return a2l
 
+  def _CreateDisambiguationTable(self):
+    """ Non-unique file names will result in None entries"""
+    self.disambiguation_table = {}
+
+    for root, _, filenames in os.walk(self.source_root_path):
+      for f in filenames:
+        self.disambiguation_table[f] = os.path.join(root, f) if (f not in
+                                       self.disambiguation_table) else None
+
 
   class Addr2Line(object):
     """A python wrapper around an addr2line instance.
@@ -312,7 +340,28 @@ 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

[Andrew Hayden (chromium.org), 2014/06/26 09:00:11]

This loop has gotten a lot nicer. I think it's self-documenting now, which is
cool. Nice job.

[Primiano Tucci (use gerrit), 2014/06/26 09:33:45]

Really? is it?

I think that nobody here is really understanding all the cases where the path is
kept as it is vs. made absolute (and w.r.t of which base dir).
What would be self-documenting, instead, would be code in the unittests which
clearly defines the possible input and the output expectations.

Just to be clear, this still LGTM. I just give up trying to follow  the path
logic here. Similarly, as a reviewer, I will not encourage any developer, who
will happen to touch this code in future, to spend any time trying to reverse
engineer the intended behavior here.

This is the very first Chrome dev principle: there is just code which is covered
by tests and code which is broken.

+        was_ambiguous = False
+        disambiguated = False
+        if self._symbolizer.disambiguate:
+          if source_path and not os.path.isabs(source_path):
+            path = self._symbolizer.disambiguation_table.get(source_path)
+            was_ambiguous = True
+            disambiguated = path is not None
+            source_path = path if disambiguated else source_path
+
+          # Use absolute paths (so that paths are consistent, as disambiguation
+          # uses absolute paths)
+          if source_path and not was_ambiguous:
+            source_path = os.path.abspath(source_path)
+
+        if source_path and self._symbolizer.strip_base_path:
+          # Strip the base path
+          source_path = re.sub('^' + self._symbolizer.strip_base_path,
+              self._symbolizer.source_root_path or '', source_path)
+
+        sym_info = ELFSymbolInfo(name, source_path, source_line, was_ambiguous,
+                                 disambiguated)
         if prev_sym_info:
           prev_sym_info.inlined_by = sym_info
         if not innermost_sym_info:
@@ -393,7 +442,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, was_ambiguous=False,
+               disambiguated=False):
     """All the fields here can be None (if addr2line replies with '??')."""
     self.name = name
     self.source_path = source_path
@@ -401,6 +451,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.was_ambiguous = was_ambiguous
 
   def __str__(self):
     return '%s [%s:%d]' % (