Update symbolize_trace to work on macOS.

Update symbolize_trace to work on macOS.

This CL hooks 'atos' into symbolize_trace to get symbolization working.

BUG=chromium:677302
Review-Url: https://codereview.chromium.org/2605183002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/d31d896231ad698f50692b012810969c4d03f0a9

[erikchen, 2016-12-29 01:18:56]

Description was changed from

==========
Update symbolize_trace to work on macOS.

This CL hooks 'atos' into symbolize_trace to get symbolization working.

BUG=677302
==========

to

==========
Update symbolize_trace to work on macOS.

This CL hooks 'atos' into symbolize_trace to get symbolization working.

BUG=chromium:677302
==========

[erikchen, 2016-12-29 01:19:15]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 01:19:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-29 01:41:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-29 01:41:17]

Dry run: This issue passed the CQ dry run.

[erikchen, 2016-12-29 17:59:44]

erikchen@chromium.org changed reviewers:
+ primiano@chromium.org

[erikchen, 2016-12-29 17:59:45]

primiano: Please review.

[Primiano Tucci (use gerrit), 2017-01-06 10:47:23]

+dskiba FYI as he wrote the original version.
Enthusiastic LGTM from my viewpoint. Thanksallot!

One question out of curiosity: does this mean that
--enable-heap-profiling=native JustWorks™ on OSX ? Or do you have some other
patch coming?

[DmitrySkiba, 2017-01-10 21:28:08]

dskiba@chromium.org changed reviewers:
+ dskiba@chromium.org

[DmitrySkiba, 2017-01-10 21:28:09]

https://codereview.chromium.org/2605183002/diff/20001/tracing/bin/symbolize_t...
File tracing/bin/symbolize_trace (right):

https://codereview.chromium.org/2605183002/diff/20001/tracing/bin/symbolize_t...
tracing/bin/symbolize_trace:309: if sys.platform == 'darwin':
We have this check in two places - first when we decide what symbolizer_path
should be, the other is here. If looks like symbolizer_path should instead be
'symblizer' object with a single method.

https://codereview.chromium.org/2605183002/diff/20001/tracing/bin/symbolize_t...
tracing/bin/symbolize_trace:346: cmd.extend([hex(int(x)) for x in
symfile.frames_by_address.keys()])
What is the max command line length on macOS? This can easily append thousands
of addresses.

[erikchen, 2017-02-04 02:34:50]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-04 02:34:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2017-02-04 02:34:56]

https://codereview.chromium.org/2605183002/diff/20001/tracing/bin/symbolize_t...
File tracing/bin/symbolize_trace (right):

https://codereview.chromium.org/2605183002/diff/20001/tracing/bin/symbolize_t...
tracing/bin/symbolize_trace:309: if sys.platform == 'darwin':
On 2017/01/10 21:28:09, DmitrySkiba wrote:
> We have this check in two places - first when we decide what symbolizer_path
> should be, the other is here. If looks like symbolizer_path should instead be
> 'symblizer' object with a single method.

Introducing a class to avoid an extra conditional seems dubious to me, but
whatever. Done.

https://codereview.chromium.org/2605183002/diff/20001/tracing/bin/symbolize_t...
tracing/bin/symbolize_trace:346: cmd.extend([hex(int(x)) for x in
symfile.frames_by_address.keys()])
On 2017/01/10 21:28:09, DmitrySkiba wrote:
> What is the max command line length on macOS? This can easily append thousands
> of addresses.

By default, the max length is 260k characters. I've added logic to not exceed
that limit.

[erikchen, 2017-02-04 02:35:07]

On 2017/01/06 10:47:23, Primiano Tucci wrote:
> +dskiba FYI as he wrote the original version.
> Enthusiastic LGTM from my viewpoint. Thanksallot!
> 
> One question out of curiosity: does this mean that
> --enable-heap-profiling=native JustWorks™ on OSX ? Or do you have some other
> patch coming?

Correct, this *just works*

[commit-bot: I haz the power, 2017-02-04 02:55:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-04 02:55:27]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2017-02-06 10:19:47]

On 2017/02/04 02:35:07, erikchen wrote:
> On 2017/01/06 10:47:23, Primiano Tucci wrote:
> > +dskiba FYI as he wrote the original version.
> > Enthusiastic LGTM from my viewpoint. Thanksallot!
> > 
> > One question out of curiosity: does this mean that
> > --enable-heap-profiling=native JustWorks™ on OSX ? Or do you have some other
> > patch coming?
> 
> Correct, this *just works*

\o/

[DmitrySkiba, 2017-02-06 19:28:34]

https://codereview.chromium.org/2605183002/diff/40001/tracing/bin/symbolize_t...
File tracing/bin/symbolize_trace (right):

https://codereview.chromium.org/2605183002/diff/40001/tracing/bin/symbolize_t...
tracing/bin/symbolize_trace:65: def _SymbolizeLinuxAndAndroid(self, sym_file):
Argument should be 'symfile' (without underscore).

https://codereview.chromium.org/2605183002/diff/40001/tracing/bin/symbolize_t...
tracing/bin/symbolize_trace:94: chars_for_other_arguments = 2000
Since symfile doesn't change in this function, we can estimate this number
exactly, by moving initialization of cmd out of the loop.

https://codereview.chromium.org/2605183002/diff/40001/tracing/bin/symbolize_t...
tracing/bin/symbolize_trace:110: for frame in
symfile.frames_by_address.values()[i]:
|i| should include offset (number of keys already processed) to properly address
into values().

https://codereview.chromium.org/2605183002/diff/40001/tracing/bin/symbolize_t...
tracing/bin/symbolize_trace:111: frame.name = output_array[i]
Hmm, according to the docs, atos output can include app and file/line info,
e.g.:

-[SKTGraphicView drawRect:] (in Sketch) (SKTGraphicView.m:445)

I.e. we need to separate parts properly (note that searching for whitespace
won't work in all cases, for example if signature includes 'const char*' or
'unsigned int' types).

[erikchen, 2017-02-11 02:45:23]

https://codereview.chromium.org/2605183002/diff/40001/tracing/bin/symbolize_t...
File tracing/bin/symbolize_trace (right):

https://codereview.chromium.org/2605183002/diff/40001/tracing/bin/symbolize_t...
tracing/bin/symbolize_trace:65: def _SymbolizeLinuxAndAndroid(self, sym_file):
On 2017/02/06 19:28:33, DmitrySkiba wrote:
> Argument should be 'symfile' (without underscore).

Done.

https://codereview.chromium.org/2605183002/diff/40001/tracing/bin/symbolize_t...
tracing/bin/symbolize_trace:94: chars_for_other_arguments = 2000
On 2017/02/06 19:28:34, DmitrySkiba wrote:
> Since symfile doesn't change in this function, we can estimate this number
> exactly, by moving initialization of cmd out of the loop.

Done.

https://codereview.chromium.org/2605183002/diff/40001/tracing/bin/symbolize_t...
tracing/bin/symbolize_trace:110: for frame in
symfile.frames_by_address.values()[i]:
On 2017/02/06 19:28:34, DmitrySkiba wrote:
> |i| should include offset (number of keys already processed) to properly
address
> into values().

Good catch, thanks. Done.

https://codereview.chromium.org/2605183002/diff/40001/tracing/bin/symbolize_t...
tracing/bin/symbolize_trace:111: frame.name = output_array[i]
On 2017/02/06 19:28:34, DmitrySkiba wrote:
> Hmm, according to the docs, atos output can include app and file/line info,
> e.g.:
> 
> -[SKTGraphicView drawRect:] (in Sketch) (SKTGraphicView.m:445)
> 
> I.e. we need to separate parts properly (note that searching for whitespace
> won't work in all cases, for example if signature includes 'const char*' or
> 'unsigned int' types).

what parsing logic are you referring to? Something in the tracing UI?

[DmitrySkiba, 2017-02-14 01:17:02]

On 2017/02/11 02:45:23, erikchen wrote:
>
https://codereview.chromium.org/2605183002/diff/40001/tracing/bin/symbolize_t...
> File tracing/bin/symbolize_trace (right):
> 
>
https://codereview.chromium.org/2605183002/diff/40001/tracing/bin/symbolize_t...
> tracing/bin/symbolize_trace:65: def _SymbolizeLinuxAndAndroid(self, sym_file):
> On 2017/02/06 19:28:33, DmitrySkiba wrote:
> > Argument should be 'symfile' (without underscore).
> 
> Done.
> 
>
https://codereview.chromium.org/2605183002/diff/40001/tracing/bin/symbolize_t...
> tracing/bin/symbolize_trace:94: chars_for_other_arguments = 2000
> On 2017/02/06 19:28:34, DmitrySkiba wrote:
> > Since symfile doesn't change in this function, we can estimate this number
> > exactly, by moving initialization of cmd out of the loop.
> 
> Done.
> 
>
https://codereview.chromium.org/2605183002/diff/40001/tracing/bin/symbolize_t...
> tracing/bin/symbolize_trace:110: for frame in
> symfile.frames_by_address.values()[i]:
> On 2017/02/06 19:28:34, DmitrySkiba wrote:
> > |i| should include offset (number of keys already processed) to properly
> address
> > into values().
> 
> Good catch, thanks. Done.
> 
>
https://codereview.chromium.org/2605183002/diff/40001/tracing/bin/symbolize_t...
> tracing/bin/symbolize_trace:111: frame.name = output_array[i]
> On 2017/02/06 19:28:34, DmitrySkiba wrote:
> > Hmm, according to the docs, atos output can include app and file/line info,
> > e.g.:
> > 
> > -[SKTGraphicView drawRect:] (in Sketch) (SKTGraphicView.m:445)
> > 
> > I.e. we need to separate parts properly (note that searching for whitespace
> > won't work in all cases, for example if signature includes 'const char*' or
> > 'unsigned int' types).
> 
> what parsing logic are you referring to? Something in the tracing UI?

Symbolizer assigns names to frames, and those names are supposed to be just
method / function names. ELFSymbolizer parses addr2line output and returns name,
source_file and source_line as separate things , while in your case whatever
atos returns gets assigned to frames. But it seems that atos also can return
source_path and source_line, and if that's the case, we need to parse atos
output and extract function / method name.

[erikchen, 2017-02-15 00:32:33]

dskiba: PTAL

I added a regex to parse the relevant expressions, as well as tests.

[DmitrySkiba, 2017-02-15 00:43:09]

LGTM!

[erikchen, 2017-02-15 00:44:53]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-02-15 00:44:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2605183002/#ps100001
(title: "Comments from dskiba.")

[commit-bot: I haz the power, 2017-02-15 00:45:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 01:05:57]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487119493684270,
"parent_rev": "7336c9424b01e5500a91b370824c3299e74a3922", "commit_rev":
"d31d896231ad698f50692b012810969c4d03f0a9"}

[commit-bot: I haz the power, 2017-02-15 01:06:01]

Description was changed from

==========
Update symbolize_trace to work on macOS.

This CL hooks 'atos' into symbolize_trace to get symbolization working.

BUG=chromium:677302
==========

to

==========
Update symbolize_trace to work on macOS.

This CL hooks 'atos' into symbolize_trace to get symbolization working.

BUG=chromium:677302

Review-Url: https://codereview.chromium.org/2605183002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-02-15 01:06:03]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...