Fix a bunch of issues blocking 64-bit orderfile generation.

Fix a bunch of issues blocking 64-bit orderfile generation.

Fix a cast that didn't play nice in 64-bit.

Modify patch_orderfile.py and cyglog_to_orderfile to accept a --target_arch parameter.

Pass architecture to symbol_extractor.

BUG=448054
Committed: https://crrev.com/9ba50ebc258a881904788c999c36dafd444d96db
Cr-Commit-Position: refs/heads/master@{#314802}

[azarchs, 2015-02-04 17:02:25]

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

[azarchs, 2015-02-04 17:02:25]

PTAL

[Benoit L, 2015-02-04 17:12:57]

lgtm, only nits.

https://codereview.chromium.org/902633002/diff/1/tools/cygprofile/cyglog_to_o...
File tools/cygprofile/cyglog_to_orderfile.py (right):

https://codereview.chromium.org/902633002/diff/1/tools/cygprofile/cyglog_to_o...
tools/cygprofile/cyglog_to_orderfile.py:224: parser.add_option('--target_arch',
action='store', type='string',
nit: maybe use "choice" here to limit the values to what we know works.

https://codereview.chromium.org/902633002/diff/1/tools/cygprofile/cygprofile_...
File tools/cygprofile/cygprofile_unittest.cc (right):

https://codereview.chromium.org/902633002/diff/1/tools/cygprofile/cygprofile_...
tools/cygprofile/cygprofile_unittest.cc:49:
ASSERT_EQ(reinterpret_cast<long>(first_entry.address), 2);
nit: uintptr_t is probably the most appropriate type here. It might not be
defined everywhere, but I believe that we are fine with our compilers.

[azarchs, 2015-02-04 17:48:27]

New patchsets have been uploaded after l-g-t-m from lizeb@chromium.org

[azarchs, 2015-02-04 17:49:54]

https://codereview.chromium.org/902633002/diff/1/tools/cygprofile/cyglog_to_o...
File tools/cygprofile/cyglog_to_orderfile.py (right):

https://codereview.chromium.org/902633002/diff/1/tools/cygprofile/cyglog_to_o...
tools/cygprofile/cyglog_to_orderfile.py:224: parser.add_option('--target_arch',
action='store', type='string',
On 2015/02/04 17:12:57, lizeb wrote:
> nit: maybe use "choice" here to limit the values to what we know works.

Done.

https://codereview.chromium.org/902633002/diff/1/tools/cygprofile/cygprofile_...
File tools/cygprofile/cygprofile_unittest.cc (right):

https://codereview.chromium.org/902633002/diff/1/tools/cygprofile/cygprofile_...
tools/cygprofile/cygprofile_unittest.cc:49:
ASSERT_EQ(reinterpret_cast<long>(first_entry.address), 2);
On 2015/02/04 17:12:57, lizeb wrote:
> nit: uintptr_t is probably the most appropriate type here. It might not be
> defined everywhere, but I believe that we are fine with our compilers.

Done.

[pasko, 2015-02-04 19:08:26]

Did you confirm any speedups? I am wondering whether implicit int() -> long
conversions in, say, mergetraces.py are working as we intend. I'm somehow not
confident :/

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

https://codereview.chromium.org/902633002/diff/20001/tools/cygprofile/patch_o...
tools/cygprofile/patch_orderfile.py:209: parser.add_option('--target_arch',
action='store', dest='arch',
please use --target-arch

https://codereview.chromium.org/902633002/diff/20001/tools/cygprofile/patch_o...
tools/cygprofile/patch_orderfile.py:215: print 'Usage: %s <unpatched_orderfile>
<libchrome.so>' % argv[0]
This has now changed, it would be better to use parser.print_help(), see:
https://docs.python.org/2/library/optparse.html#generating-help

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

https://codereview.chromium.org/902633002/diff/20001/tools/cygprofile/symbol_...
tools/cygprofile/symbol_extractor.py:29: global _OBJDUMP_BINARY
let's not increase global state. How about _GetObjdumpBinary()?

[azarchs, 2015-02-05 09:40:02]

I can't yet test the performance impact, as I haven't yet made the changes to
make it build with the orderfile in arm64.  This is just to make one.  On
inspection, the output orderfile looks reasonable, however.

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

https://codereview.chromium.org/902633002/diff/20001/tools/cygprofile/patch_o...
tools/cygprofile/patch_orderfile.py:209: parser.add_option('--target_arch',
action='store', dest='arch',
On 2015/02/04 19:08:26, pasko wrote:
> please use --target-arch

Done.

https://codereview.chromium.org/902633002/diff/20001/tools/cygprofile/patch_o...
tools/cygprofile/patch_orderfile.py:215: print 'Usage: %s <unpatched_orderfile>
<libchrome.so>' % argv[0]
On 2015/02/04 19:08:26, pasko wrote:
> This has now changed, it would be better to use parser.print_help(), see:
> https://docs.python.org/2/library/optparse.html#generating-help

Done.

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

https://codereview.chromium.org/902633002/diff/20001/tools/cygprofile/symbol_...
tools/cygprofile/symbol_extractor.py:29: global _OBJDUMP_BINARY
On 2015/02/04 19:08:26, pasko wrote:
> let's not increase global state. How about _GetObjdumpBinary()?

This isn't increasing global state - just changing the content of a variable
that is already global.  Do we want to do make that variable not global?  It
sounds like a good idea, but would be a more extensive change as we'd need to
turn this whole file into a class instead of a bunch of global methods.

[pasko, 2015-02-05 10:24:50]

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

https://codereview.chromium.org/902633002/diff/20001/tools/cygprofile/symbol_...
tools/cygprofile/symbol_extractor.py:29: global _OBJDUMP_BINARY
On 2015/02/05 09:40:02, azarchs wrote:
> On 2015/02/04 19:08:26, pasko wrote:
> > let's not increase global state. How about _GetObjdumpBinary()?
> 
> This isn't increasing global state - just changing the content of a variable
> that is already global.  Do we want to do make that variable not global?  It
> sounds like a good idea, but would be a more extensive change as we'd need to
> turn this whole file into a class instead of a bunch of global methods.

Let's eliminate the _OBJDUMP_BINARY (referenced only once in this file), and
always call symbol.ToolPath('objdump').

The SetArchitecture(arch) would stay and be a one liner to modify the
symbol.ARCH in an obvious way.

WDYT?

[azarchs, 2015-02-05 10:31:01]

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

https://codereview.chromium.org/902633002/diff/20001/tools/cygprofile/symbol_...
tools/cygprofile/symbol_extractor.py:29: global _OBJDUMP_BINARY
On 2015/02/05 10:24:50, pasko wrote:
> On 2015/02/05 09:40:02, azarchs wrote:
> > On 2015/02/04 19:08:26, pasko wrote:
> > > let's not increase global state. How about _GetObjdumpBinary()?
> > 
> > This isn't increasing global state - just changing the content of a variable
> > that is already global.  Do we want to do make that variable not global?  It
> > sounds like a good idea, but would be a more extensive change as we'd need
to
> > turn this whole file into a class instead of a bunch of global methods.
> 
> Let's eliminate the _OBJDUMP_BINARY (referenced only once in this file), and
> always call symbol.ToolPath('objdump').
> 
> The SetArchitecture(arch) would stay and be a one liner to modify the
> symbol.ARCH in an obvious way.
> 
> WDYT?

Done.

[pasko, 2015-02-05 10:47:29]

lgtm, thank you!

[azarchs, 2015-02-05 12:02:17]

The CQ bit was checked by azarchs@chromium.org

[commit-bot: I haz the power, 2015-02-05 12:03:03]

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

[commit-bot: I haz the power, 2015-02-05 14:01:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-05 14:03:23]

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