Switch from nm to objdump for the cygprofile tools.

tools/cygprofile/symbol_extractor.py

Index: tools/cygprofile/symbol_extractor.py
diff --git a/tools/cygprofile/symbol_extractor.py b/tools/cygprofile/symbol_extractor.py
index d3a2ee06fc2b100a5735075c0791b43f96edf205..09b6fdeeea58cf8ddedc5fdcae4ef21e63651d77 100755
--- a/tools/cygprofile/symbol_extractor.py
+++ b/tools/cygprofile/symbol_extractor.py
@@ -19,61 +19,55 @@ import symbol
 
 # TODO(lizeb): Change symbol.ARCH to the proper value when "arm" is no longer
 # the only possible value.
-_NM_BINARY = symbol.ToolPath('nm')
+_OBJDUMP_BINARY = symbol.ToolPath('objdump')

[pasko, 2015/01/28 18:55:37]

The phrase 'This is required, as nm doesn't provide the section name' confuses
me.

The 'section' value provided by _FromObjdumpLine() would be either '.text' or
'*UND*', so this CL won't change the orderfile. I am guessing this is needed to
be able to use the same SymbolInfo when we parse the lines from the objects (*.o
files)? If we really want to share the 8 lines of code for objdump parsing,
let's create an argument, say, extract_section_name=True|False.

Though it's not clear what the plan is. I imagined we would make a global
'symbol to section map', just like we do today, and use it to update every
SymbolInfo we parsed from the DSO (libchrome.so). Updating symbol.name would be
just fine for these purposes.

Please clarify.

[Benoit L, 2015/01/29 10:02:01]

Yes, the goal is to have only one way to parse object files, and the purpose of
this CL is to make the parsing sufficiently rich for symbolize.py.

The section indeed matters only for the symbolize.py use, that is parsing object
files. Since it is not a problem elsewhere, let's kep only one code path, and
less arguments.

What do you think?

[pasko, 2015/01/29 12:24:42]

Sounds good, but please either remove the 'required' part from the CL
description or enhance it to say that it is required to share the parsing logic
with symbolize.py

 
 
-SymbolInfo = collections.namedtuple('SymbolInfo', ('name', 'offset', 'size'))
+SymbolInfo = collections.namedtuple('SymbolInfo', ('name', 'offset', 'size',
+                                                   'section'))
 
 
-def FromNmLine(line):
-  """Create a SymbolInfo by parsing a properly formatted nm output line.
+def _FromObjdumpLine(line):
+  """Create a SymbolInfo by parsing a properly formatted objdump output line.
 
   Args:
-    line: line from nm
+    line: line from objdump
 
   Returns:
     An instance of SymbolInfo if the line represents a symbol, None otherwise.
   """
-  # 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 re.search(' (t|W|T) ', line):
-    return None
+  # All of the symbol lines we care about are in the form
+  # 0000000000  g    F   .text.foo     000000000 [.hidden] foo
+  # where g (global) might also be l (local) or w (weak).
   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:
+  if len(parts) < 6 or parts[2] != 'F':
     return None
+  offset = int(parts[0], 16)

[pasko, 2015/01/29 12:24:42]

To prevent loosing information without knowing let's put some assertions here:
assert(len(parts) == 5 or (len(parts) == 5 and parts[5] == '.hidden'))
accepted_scopes = set(['g', 'l', 'w'])
assert(parts[1] in accepted_scopes)

also, it would be nice to check that the symbol and section name are in
[:alnum:] + underscores.

btw, did you compare your orderfile patching locally before/after the
refactoring?

+  section = parts[3]
+  size = int(parts[4], 16)
+  name = parts[-1]
+  return SymbolInfo(name=name, offset=offset, section=section, size=size)
 
 
-def SymbolInfosFromStream(nm_lines):
-  """Parses the output of nm, and get all the symbols from a binary.
+def _SymbolInfosFromStream(objdump_lines):
+  """Parses the output of objdump, and get all the symbols from a binary.
 
   Args:
-    nm_lines: An iterable of lines
+    objdump_lines: An iterable of lines
 
   Returns:
     A list of SymbolInfo.
   """
   # TODO(lizeb): Consider switching to objdump to simplify parsing.

[pasko, 2015/01/28 18:55:37]

this change is implementing the TODO (!)

[Benoit L, 2015/01/29 10:02:01]

Arg, indeed. It is so rare to remove TODOs, it would be bad to forget one that
is done.
Done.

[pasko, 2015/01/29 12:24:42]

the blame is on me for nagging to add this TODO, thanks for reminding about
that, I almost forgot, but no, things like that must not be forgotten ..

I frankly did not expect it to arrive that fast, it's the finding about the 'nm'
tool taken from an incorrect place boosted the interest in revamping tool usage.

Thanks for doing!

   symbol_infos = []
-  for line in nm_lines:
-    symbol_info = FromNmLine(line)
+  for line in objdump_lines:
+    symbol_info = _FromObjdumpLine(line)
     if symbol_info is not None:
       symbol_infos.append(symbol_info)
   return symbol_infos
 
 
 def SymbolInfosFromBinary(binary_filename):
-  """Runs nm to get all the symbols from a binary.
+  """Runs objdump to get all the symbols from a binary.
 
   Args:
     binary_filename: path to the binary.
@@ -81,10 +75,10 @@ def SymbolInfosFromBinary(binary_filename):
   Returns:
     A list of SymbolInfo from the binary.
   """
-  command = (_NM_BINARY, '-S', '-n', binary_filename)
+  command = (_OBJDUMP_BINARY, '-t', '-w', binary_filename)
   p = subprocess.Popen(command, shell=False, stdout=subprocess.PIPE)
   try:
-    result = SymbolInfosFromStream(p.stdout)
+    result = _SymbolInfosFromStream(p.stdout)
     return result
   finally:
     p.wait()