Chromium Code Reviews
Help | Chromium Project | Sign in
(5)

Issue 2698543003: Use absolute pcs and load locations to symbolize on macOS. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 2 weeks ago by erikchen
Modified:
2 months, 1 week ago
CC:
catapult-reviews_chromium.org, tracing-review_chromium.org
Target Ref:
refs/heads/master
Project:
catapult
Visibility:
Public.

Description

Use absolute pcs and load locations to symbolize on macOS. atos gets confused if it's passed a relative address that's lower than the load location of the TEXT segment of a Mach-O file. This CL adds a utility to read the load address of a Mach-O file, and then computes an absolute PC/load location based on the relative PC. BUG=chromium:677302 Review-Url: https://codereview.chromium.org/2698543003 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/5252580c24aa6c4bc322a19c9ad03cd2c94f1382

Patch Set 1 #

Total comments: 1

Patch Set 2 : Extract load address of a binary from otool. #

Total comments: 9

Patch Set 3 : comments from dskiba. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+65 lines, -3 lines) Patch
M tracing/bin/symbolize_trace View 1 2 3 chunks +11 lines, -3 lines 0 comments Download
A tracing/bin/symbolize_trace_macho_reader.py View 1 1 chunk +26 lines, -0 lines 0 comments Download
A tracing/bin/symbolize_trace_macho_reader_unittest.py View 1 1 chunk +28 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 31 (9 generated)
erikchen
dskiba: Please review.
2 months, 2 weeks ago (2017-02-15 01:43:27 UTC) #2
DmitrySkiba
https://codereview.chromium.org/2698543003/diff/1/tracing/bin/symbolize_trace File tracing/bin/symbolize_trace (right): https://codereview.chromium.org/2698543003/diff/1/tracing/bin/symbolize_trace#newcode347 tracing/bin/symbolize_trace:347: symfile.region_start_address = region.start_address Hmm, this overwrites the whatever value ...
2 months, 2 weeks ago (2017-02-15 22:40:38 UTC) #3
erikchen
On 2017/02/15 22:40:38, DmitrySkiba wrote: > https://codereview.chromium.org/2698543003/diff/1/tracing/bin/symbolize_trace > File tracing/bin/symbolize_trace (right): > > https://codereview.chromium.org/2698543003/diff/1/tracing/bin/symbolize_trace#newcode347 > ...
2 months, 2 weeks ago (2017-02-15 22:45:27 UTC) #4
erikchen
+mark Unless we are guaranteed that the image slide is the same for all chrome-related ...
2 months, 2 weeks ago (2017-02-15 22:48:59 UTC) #6
DmitrySkiba
On 2017/02/15 22:48:59, erikchen wrote: > +mark > > Unless we are guaranteed that the ...
2 months, 2 weeks ago (2017-02-15 22:54:05 UTC) #7
erikchen
> Hmm, so actually we don't really care about region.start_address, we just need > to ...
2 months, 2 weeks ago (2017-02-15 22:57:00 UTC) #8
erikchen
On 2017/02/15 22:57:00, erikchen wrote: > > Hmm, so actually we don't really care about ...
2 months, 2 weeks ago (2017-02-15 22:59:45 UTC) #9
erikchen
On 2017/02/15 22:57:00, erikchen wrote: > > Hmm, so actually we don't really care about ...
2 months, 2 weeks ago (2017-02-15 22:59:47 UTC) #10
Mark Mentovai
erikchen wrote: > Unless we are guaranteed that the image slide is the same for ...
2 months, 1 week ago (2017-02-16 00:59:19 UTC) #11
erikchen
dskiba, mark: Please review.
2 months, 1 week ago (2017-02-16 21:03:24 UTC) #13
DmitrySkiba
https://codereview.chromium.org/2698543003/diff/20001/tracing/bin/symbolize_trace File tracing/bin/symbolize_trace (right): https://codereview.chromium.org/2698543003/diff/20001/tracing/bin/symbolize_trace#newcode99 tracing/bin/symbolize_trace:99: region_start_address = (symbolize_trace_macho_reader. Let's call this load_address ? https://codereview.chromium.org/2698543003/diff/20001/tracing/bin/symbolize_trace#newcode119 ...
2 months, 1 week ago (2017-02-16 21:19:46 UTC) #14
erikchen
https://codereview.chromium.org/2698543003/diff/20001/tracing/bin/symbolize_trace File tracing/bin/symbolize_trace (right): https://codereview.chromium.org/2698543003/diff/20001/tracing/bin/symbolize_trace#newcode99 tracing/bin/symbolize_trace:99: region_start_address = (symbolize_trace_macho_reader. On 2017/02/16 21:19:45, DmitrySkiba wrote: > ...
2 months, 1 week ago (2017-02-16 21:32:18 UTC) #15
DmitrySkiba
https://codereview.chromium.org/2698543003/diff/20001/tracing/bin/symbolize_trace File tracing/bin/symbolize_trace (right): https://codereview.chromium.org/2698543003/diff/20001/tracing/bin/symbolize_trace#newcode119 tracing/bin/symbolize_trace:119: cmd.extend([hex(int(x) + region_start_address) On 2017/02/16 21:32:18, erikchen wrote: > ...
2 months, 1 week ago (2017-02-16 21:38:48 UTC) #16
erikchen
https://codereview.chromium.org/2698543003/diff/20001/tracing/bin/symbolize_trace File tracing/bin/symbolize_trace (right): https://codereview.chromium.org/2698543003/diff/20001/tracing/bin/symbolize_trace#newcode119 tracing/bin/symbolize_trace:119: cmd.extend([hex(int(x) + region_start_address) On 2017/02/16 21:38:48, DmitrySkiba wrote: > ...
2 months, 1 week ago (2017-02-16 21:43:22 UTC) #17
DmitrySkiba
LGTM
2 months, 1 week ago (2017-02-16 22:03:30 UTC) #18
Mark Mentovai
LGTM
2 months, 1 week ago (2017-02-17 18:12:26 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698543003/40001
2 months, 1 week ago (2017-02-17 18:28:36 UTC) #21
commit-bot: I haz the power
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full ...
2 months, 1 week ago (2017-02-17 18:28:39 UTC) #23
erikchen
nednguyen: OWNER stamp please.
2 months, 1 week ago (2017-02-17 19:04:59 UTC) #25
nednguyen
lgtm
2 months, 1 week ago (2017-02-17 19:06:22 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698543003/40001
2 months, 1 week ago (2017-02-17 19:10:24 UTC) #28
commit-bot: I haz the power
2 months, 1 week ago (2017-02-17 19:33:53 UTC) #31
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46