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

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

First part of the orderfile generating script refactor. The aim here is to make
sure that all scripts parse the object files the same way, using the same
tool. This commit extracts the parsing logic from patch_orderfile.py. It uses nm
from the Android NDK.

BUG=452879
Committed: https://crrev.com/719d8624a8c241085bd5793557cd6601aeca8f0b
Cr-Commit-Position: refs/heads/master@{#313528}

[Benoit L, 2015-01-28 13:21:07]

lizeb@chromium.org changed reviewers:
+ pasko@chromium.org

[Benoit L, 2015-01-28 13:21:08]

Hello,

Please review this refactor. More to come :-)

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

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/patch_o...
File tools/cygprofile/patch_orderfile.py (right):

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/patch_o...
tools/cygprofile/patch_orderfile.py:31: import symbols_extractor
s/symbols_extractor/symbol_extractor/

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/patch_o...
tools/cygprofile/patch_orderfile.py:53: The same output as GetSymbolsFromBinary.
did you mean _GroupSymbolInfosFromBinary?

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/symbols...
File tools/cygprofile/symbols_extractor.py (right):

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/symbols...
tools/cygprofile/symbols_extractor.py:17: # TODO(lizeb): Fix this when the
hardcoded assumptions (ARM 32bits, linux
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.

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/symbols...
tools/cygprofile/symbols_extractor.py:24: __SymbolInfo = collections.namedtuple(
Two blank lines between top-level definitions:
https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Blank...

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/symbols...
tools/cygprofile/symbols_extractor.py:27: class SymbolInfo(__SymbolInfo):
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

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/symbols...
tools/cygprofile/symbols_extractor.py:40: An instance of SymbolInfo if the line
represent a symbol, None otherwise.
s/represent/represents/

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/symbols...
tools/cygprofile/symbols_extractor.py:49: if not any(x in line for x in (' t ',
' W ', ' T ')):
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.

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/symbols...
tools/cygprofile/symbols_extractor.py:69: The same output as
GetSymbolsFromBinary.
did you mean SymbolInfosFromBinary? Saying that it's 'a list of SymbolInfo'
would be easier to read (less random seeks, you know)

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/symbols...
tools/cygprofile/symbols_extractor.py:74: if not any(x in line for x in (' t ',
' W ', ' T ')):
why checking it again here? the checkign for None two lines below does the job

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/symbols...
tools/cygprofile/symbols_extractor.py:100: def
OffsetToSymbolInfos(symbol_infos):
GroupSymbolInfosByOffset? GroupSymbolsByOffset? GroupByOffset?

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

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/symbols...
tools/cygprofile/symbols_extractor.py:118: def NameToSymbolInfo(symbol_infos):
CreateNameToSymbolInfo?

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

Thank you for the review.
All done.

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/patch_o...
File tools/cygprofile/patch_orderfile.py (right):

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/patch_o...
tools/cygprofile/patch_orderfile.py:31: import symbols_extractor
On 2015/01/28 14:37:01, pasko wrote:
> s/symbols_extractor/symbol_extractor/

Done.

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/patch_o...
tools/cygprofile/patch_orderfile.py:53: The same output as GetSymbolsFromBinary.
On 2015/01/28 14:37:01, pasko wrote:
> did you mean _GroupSymbolInfosFromBinary?

Done.

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/symbols...
File tools/cygprofile/symbols_extractor.py (right):

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/symbols...
tools/cygprofile/symbols_extractor.py:17: # TODO(lizeb): Fix this when the
hardcoded assumptions (ARM 32bits, linux
On 2015/01/28 14:37:01, pasko wrote:
> 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.

Thank you for the tip !
Done.

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/symbols...
tools/cygprofile/symbols_extractor.py:24: __SymbolInfo = collections.namedtuple(
On 2015/01/28 14:37:01, pasko wrote:
> Two blank lines between top-level definitions:
>
https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Blank...

Done.

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/symbols...
tools/cygprofile/symbols_extractor.py:27: class SymbolInfo(__SymbolInfo):
On 2015/01/28 14:37:01, pasko wrote:
> 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

Done.

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/symbols...
tools/cygprofile/symbols_extractor.py:40: An instance of SymbolInfo if the line
represent a symbol, None otherwise.
On 2015/01/28 14:37:01, pasko wrote:
> s/represent/represents/

Done.

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/symbols...
tools/cygprofile/symbols_extractor.py:49: if not any(x in line for x in (' t ',
' W ', ' T ')):
On 2015/01/28 14:37:02, pasko wrote:
> 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.

Done.

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/symbols...
tools/cygprofile/symbols_extractor.py:69: The same output as
GetSymbolsFromBinary.
On 2015/01/28 14:37:01, pasko wrote:
> did you mean SymbolInfosFromBinary? Saying that it's 'a list of SymbolInfo'
> would be easier to read (less random seeks, you know)

Done.

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/symbols...
tools/cygprofile/symbols_extractor.py:74: if not any(x in line for x in (' t ',
' W ', ' T ')):
On 2015/01/28 14:37:02, pasko wrote:
> why checking it again here? the checkign for None two lines below does the job

Done.

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/symbols...
tools/cygprofile/symbols_extractor.py:100: def
OffsetToSymbolInfos(symbol_infos):
On 2015/01/28 14:37:02, pasko wrote:
> GroupSymbolInfosByOffset? GroupSymbolsByOffset? GroupByOffset?
> 
> the name OffsetToSomething suggests that offset would be a parameter,
misleading

Done.

https://codereview.chromium.org/884113002/diff/20001/tools/cygprofile/symbols...
tools/cygprofile/symbols_extractor.py:118: def NameToSymbolInfo(symbol_infos):
On 2015/01/28 14:37:01, pasko wrote:
> CreateNameToSymbolInfo?

Done.

[pasko, 2015-01-28 15:57:24]

LGTM, thank you

https://codereview.chromium.org/884113002/diff/40001/tools/cygprofile/symbol_...
File tools/cygprofile/symbol_extractor.py (right):

https://codereview.chromium.org/884113002/diff/40001/tools/cygprofile/symbol_...
tools/cygprofile/symbol_extractor.py:27: def FromNmLine(line):
nit: add an extra newline above

https://codereview.chromium.org/884113002/diff/40001/tools/cygprofile/symbol_...
tools/cygprofile/symbol_extractor.py:43: if not re.search(' (t|W|T) ', line):
I meant rather two regexps:
'([0-9a-f]+)\s+([0-9a-f]+)\s+(t|W|T)\s+(.*)'
'([0-9a-f]+)\s+(t|W|T)\s+(.*)'

hm, but probably your way is clearer, let it be so

[Benoit L, 2015-01-28 16:03:54]

Thank you for the quick and thorough review.

https://codereview.chromium.org/884113002/diff/40001/tools/cygprofile/symbol_...
File tools/cygprofile/symbol_extractor.py (right):

https://codereview.chromium.org/884113002/diff/40001/tools/cygprofile/symbol_...
tools/cygprofile/symbol_extractor.py:27: def FromNmLine(line):
On 2015/01/28 15:57:24, pasko wrote:
> nit: add an extra newline above

Done.

https://codereview.chromium.org/884113002/diff/40001/tools/cygprofile/symbol_...
tools/cygprofile/symbol_extractor.py:43: if not re.search(' (t|W|T) ', line):
On 2015/01/28 15:57:24, pasko wrote:
> I meant rather two regexps:
> '([0-9a-f]+)\s+([0-9a-f]+)\s+(t|W|T)\s+(.*)'
> '([0-9a-f]+)\s+(t|W|T)\s+(.*)'
> 
> hm, but probably your way is clearer, let it be so

Acknowledged.

[Benoit L, 2015-01-28 16:04:01]

The CQ bit was checked by lizeb@chromium.org

[commit-bot: I haz the power, 2015-01-28 16:04:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/884113002/60001

[commit-bot: I haz the power, 2015-01-28 16:49:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-28 16:50:02]

Patchset 4 (id:??) landed as
https://crrev.com/719d8624a8c241085bd5793557cd6601aeca8f0b
Cr-Commit-Position: refs/heads/master@{#313528}