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..f5f1474ef6dbbf54b04560581a89e0f8c898be15 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. Which is used in

[Primiano Tucci (use gerrit), 2014/06/25 09:02:58]

Nit: s/. Which//

+          the disambiguation process. 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

[Primiano Tucci (use gerrit), 2014/06/25 09:02:59]

Uh, what is the difference between a "full" and a "absolute" path?
Probably you want just one of the two?

+          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.

[Primiano Tucci (use gerrit), 2014/06/25 09:02:58]

Can we reword this a bit and make it a bit more clear and concise? 
What about:

"path to the source tree on the local machine. It has two purposes: 1. turn
relative file names names into full paths (foo.c -> /source/root/path/foo.c) iff
foo.c is not ambiguous (i.e. unique in the |source_root_path|) to workaround
some toolchains limitations; 2. serves as the replacement base for
|strip_base_path| (below)"

+      strip_base_path: If this base path is found in the beginning of any

[Primiano Tucci (use gerrit), 2014/06/25 09:02:58]

"rebases the symbols source paths onto |source_root_path| (i.e.
s/^strip_base_path/source_root_path/)."

+          result, we strip it and replace it with |source_root_path|.
     """
     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.lookup_table = {}

[Primiano Tucci (use gerrit), 2014/06/25 09:02:59]

What about:
self.disambiguation_table = {}  # 'foo.c' -> '/abs/path/foo.c'

"lookup_table" has the same value of saying:
self.array = []
self.number = 3 :) not really explicative.
Variables names should reflect the role they play, not the data structure used
to implement them :)

+    self.source_root_path = source_root_path
+    self.strip_base_path = strip_base_path
+    if(self.disambiguate):
+      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):

[Primiano Tucci (use gerrit), 2014/06/25 09:02:59]

Just add a one line comment please:
"""Non-unique file names will result in None entries"""

+    self.lookup_table = {}
+    source_root_path = os.path.abspath(self.source_root_path)

[Primiano Tucci (use gerrit), 2014/06/25 09:02:59]

I'd do this on line 121 and save one line:
self.source_root_path = os.path.abspath(source_root_path)

+
+    for root, _, filenames in os.walk(source_root_path):
+      for f in filenames:
+        self.lookup_table[f] = os.path.join(root, f) if (f not in
+                                self.lookup_table) else None
+
 
   class Addr2Line(object):
     """A python wrapper around an addr2line instance.
@@ -312,7 +340,22 @@ 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
+        was_ambiguous = False
+        disambiguated = False
+        if self._symbolizer.disambiguate:
+          if source_path and not source_path.startswith('/'):

[Primiano Tucci (use gerrit), 2014/06/25 09:02:58]

Probably you can just use: posixpath.isabs(source_path) here

+            source_path = self._symbolizer.lookup_table.get(source_path)
+            was_ambiguous = True
+            disambiguated = source_path is not None

[Primiano Tucci (use gerrit), 2014/06/25 09:02:59]

Just a doubt: at this point if a2l here returns 'foo.c' and you don't have foo.c
in your disambiguation table, source_path becomes none.
In essence, looks like you're trying to enrich the output here, but if you
happen to fail, you end up reducing the quality of the output ('foo.c' -> None.)
Don't you probably want to do something like:

full_path = self._symbolizer.disambiguation_table.get(source_path)
if full_path:
  source_path = full_path
  disambiguated = True
?

+
+        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)

[Primiano Tucci (use gerrit), 2014/06/25 09:02:58]

Nit: indentation should be 4 spaces here, or inline. i.e.
either

source_path = re.sub('^' + self._symbolizer.strip_base_path,
    self._symbolizer.source_root_path or '', source_path)

or

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,14 +436,18 @@ 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
+    self.source_path = (None if source_path is None else

[Primiano Tucci (use gerrit), 2014/06/25 09:02:58]

why do you need this logic (the abs path) here? Didn't you cover that above? If
the client didn't ask for disambiguation, he's fine to get relative paths.
Also, this is conceptually wrong: if you have 'foo.c' (suppose you didn't chose
disambiguation), abspath('foo.c') = PWD + 'foo.c', which is probably not what
you want.

+                       os.path.abspath(source_path))
     self.source_line = source_line
     # 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]' % (