Use absolute pcs and load locations to symbolize on macOS.

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

[erikchen, 2017-02-15 01:43:26]

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

[erikchen, 2017-02-15 01:43:27]

dskiba: Please review.

[DmitrySkiba, 2017-02-15 22:40:38]

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...
tracing/bin/symbolize_trace:347: symfile.region_start_address =
region.start_address
Hmm, this overwrites the whatever value region_start_address previously had.

Does macOS have ASLR for the image itself? The reason we calculate relative_pc
here is because different absolute addresses can end up being single relative
address after translation.

I.e. browser mapped foo() to 0xA000, and renderer mapped it to 0xF000, but it's
relative address ends up being 0x1000 if we take into account load addresses.

With this change we end up with the same relative address 0x1000, but use the
last known start address 0xE000 to later (in _SymbolizeMac) translate 0x1000
back to 0xF000.

What am I missing?

[erikchen, 2017-02-15 22:45:27]

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...
> tracing/bin/symbolize_trace:347: symfile.region_start_address =
> region.start_address
> Hmm, this overwrites the whatever value region_start_address previously had.
> 
> Does macOS have ASLR for the image itself? The reason we calculate relative_pc
> here is because different absolute addresses can end up being single relative
> address after translation.
> 
> I.e. browser mapped foo() to 0xA000, and renderer mapped it to 0xF000, but
it's
> relative address ends up being 0x1000 if we take into account load addresses.
> 
> With this change we end up with the same relative address 0x1000, but use the
> last known start address 0xE000 to later (in _SymbolizeMac) translate 0x1000
> back to 0xF000.
> 
> What am I missing?

A Mach-O image itself has an expected load address e.g. (0x100000000). ASLR will
modify this by a slide, e.g. 0x100120000. Let's say that there's a symbol at
0x100123456. 
atos -l 0x100120000 0x100123456 will return the right symbol, but atos -l 0x0
0x123456 will barf.

[erikchen, 2017-02-15 22:48:58]

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

[erikchen, 2017-02-15 22:48:59]

+mark

Unless we are guaranteed that the image slide is the same for all chrome-related
processes, it looks like we're going to have to symbolize processes separately.

"""
erikchen@erikchen-macpro ~/projects/chromium3/src  (temp72)$ otool -l
./out/gn/Chromium.app/Contents/MacOS/Chromium  | head -n 20
./out/gn/Chromium.app/Contents/MacOS/Chromium:
Load command 0
      cmd LC_SEGMENT_64
  cmdsize 72
  segname __PAGEZERO
   vmaddr 0x0000000000000000
   vmsize 0x0000000100000000
  fileoff 0
 filesize 0
  maxprot 0x00000000
 initprot 0x00000000
   nsects 0
    flags 0x0
Load command 1
      cmd LC_SEGMENT_64
  cmdsize 552
  segname __TEXT
   vmaddr 0x0000000100000000
   vmsize 0x0000000000001000
  fileoff 0

erikchen@erikchen-macpro ~/projects/chromium3/src  (temp72)$ atos -o
./out/gn/Chromium.app/Contents/MacOS/Chromium -l 0x100000000 0x100000bae
main (in Chromium) (chrome_exe_main_mac.c:34)
erikchen@erikchen-macpro ~/projects/chromium3/src  (temp72)$ atos -o
./out/gn/Chromium.app/Contents/MacOS/Chromium -l 0x00000000 0x00000bae
0x00000bae (in Chromium)
"""

[DmitrySkiba, 2017-02-15 22:54:05]

On 2017/02/15 22:48:59, erikchen wrote:
> +mark
> 
> Unless we are guaranteed that the image slide is the same for all
chrome-related
> processes, it looks like we're going to have to symbolize processes
separately.
> 
> """
> erikchen@erikchen-macpro ~/projects/chromium3/src  (temp72)$ otool -l
> ./out/gn/Chromium.app/Contents/MacOS/Chromium  | head -n 20
> ./out/gn/Chromium.app/Contents/MacOS/Chromium:
> Load command 0
>       cmd LC_SEGMENT_64
>   cmdsize 72
>   segname __PAGEZERO
>    vmaddr 0x0000000000000000
>    vmsize 0x0000000100000000
>   fileoff 0
>  filesize 0
>   maxprot 0x00000000
>  initprot 0x00000000
>    nsects 0
>     flags 0x0
> Load command 1
>       cmd LC_SEGMENT_64
>   cmdsize 552
>   segname __TEXT
>    vmaddr 0x0000000100000000
>    vmsize 0x0000000000001000
>   fileoff 0
> 
> erikchen@erikchen-macpro ~/projects/chromium3/src  (temp72)$ atos -o
> ./out/gn/Chromium.app/Contents/MacOS/Chromium -l 0x100000000 0x100000bae
> main (in Chromium) (chrome_exe_main_mac.c:34)
> erikchen@erikchen-macpro ~/projects/chromium3/src  (temp72)$ atos -o
> ./out/gn/Chromium.app/Contents/MacOS/Chromium -l 0x00000000 0x00000bae
> 0x00000bae (in Chromium)
> """

Hmm, so actually we don't really care about region.start_address, we just need
to read load address from a binary, and use that to offset symbols we pass to
atos?

[erikchen, 2017-02-15 22:57:00]

> Hmm, so actually we don't really care about region.start_address, we just need
> to read load address from a binary, and use that to offset symbols we pass to
> atos?

No, region.start_address includes the slide [ASLR], which is relevant.

[erikchen, 2017-02-15 22:59:45]

On 2017/02/15 22:57:00, erikchen wrote:
> > Hmm, so actually we don't really care about region.start_address, we just
need
> > to read load address from a binary, and use that to offset symbols we pass
to
> > atos?
> 
> No, region.start_address includes the slide [ASLR], which is relevant.

theoretically, you could compute the relative address, and then find the load
address from the binary, and then "pretend" that the slide is 0. This requires
parsing the binary, which isn't that hard, but also seems like more work than
just using the actual addresses/load address. I think this mostly depends on
whether the slide is guaranteed to be the same across all chrome-binaries, in
which case the current code is both fastest and easyest.

[erikchen, 2017-02-15 22:59:47]

On 2017/02/15 22:57:00, erikchen wrote:
> > Hmm, so actually we don't really care about region.start_address, we just
need
> > to read load address from a binary, and use that to offset symbols we pass
to
> > atos?
> 
> No, region.start_address includes the slide [ASLR], which is relevant.

theoretically, you could compute the relative address, and then find the load
address from the binary, and then "pretend" that the slide is 0. This requires
parsing the binary, which isn't that hard, but also seems like more work than
just using the actual addresses/load address. I think this mostly depends on
whether the slide is guaranteed to be the same across all chrome-binaries, in
which case the current code is both fastest and easyest.

[Mark Mentovai, 2017-02-16 00:59:19]

erikchen wrote:
> Unless we are guaranteed that the image slide is the same for all
chrome-related
> processes, it looks like we're going to have to symbolize processes
separately.

Slide will definitely vary between processes. See 10.12.3
xnu-3789.41.3/bsd/kern/mach_loader.c load_machfile(), computation of aslr_offset
and dyld_aslr_offset. load_machfile() is called once for each execve() or
posix_spawn().

[erikchen, 2017-02-16 21:03:09]

Description was changed from

==========
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.

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

to

==========
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
==========

[erikchen, 2017-02-16 21:03:24]

dskiba, mark: Please review.

[DmitrySkiba, 2017-02-16 21:19:46]

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

https://codereview.chromium.org/2698543003/diff/20001/tracing/bin/symbolize_t...
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_t...
tracing/bin/symbolize_trace:119: cmd.extend([hex(int(x) + region_start_address)
This change seems to be noop, why are you making it? (Also, the one below.)

https://codereview.chromium.org/2698543003/diff/20001/tracing/bin/symbolize_t...
File tracing/bin/symbolize_trace_macho_reader.py (right):

https://codereview.chromium.org/2698543003/diff/20001/tracing/bin/symbolize_t...
tracing/bin/symbolize_trace_macho_reader.py:25: return int(result.group(1), 16)
Will int be always enough?

[erikchen, 2017-02-16 21:32:18]

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

https://codereview.chromium.org/2698543003/diff/20001/tracing/bin/symbolize_t...
tracing/bin/symbolize_trace:99: region_start_address =
(symbolize_trace_macho_reader.
On 2017/02/16 21:19:45, DmitrySkiba wrote:
> Let's call this load_address ?

Done.

https://codereview.chromium.org/2698543003/diff/20001/tracing/bin/symbolize_t...
tracing/bin/symbolize_trace:119: cmd.extend([hex(int(x) + region_start_address)
On 2017/02/16 21:19:45, DmitrySkiba wrote:
> This change seems to be noop, why are you making it? (Also, the one below.)

I don't understand your question.

old code:
hex(int(x))

new code:

hex(int(x) + region_start_address)

https://codereview.chromium.org/2698543003/diff/20001/tracing/bin/symbolize_t...
File tracing/bin/symbolize_trace_macho_reader.py (right):

https://codereview.chromium.org/2698543003/diff/20001/tracing/bin/symbolize_t...
tracing/bin/symbolize_trace_macho_reader.py:25: return int(result.group(1), 16)
On 2017/02/16 21:19:46, DmitrySkiba wrote:
> Will int be always enough?

"""
 In Python 2, Integers will automatically switch to longs when they grow beyond
their limit:
"""

[DmitrySkiba, 2017-02-16 21:38:48]

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

https://codereview.chromium.org/2698543003/diff/20001/tracing/bin/symbolize_t...
tracing/bin/symbolize_trace:119: cmd.extend([hex(int(x) + region_start_address)
On 2017/02/16 21:32:18, erikchen wrote:
> On 2017/02/16 21:19:45, DmitrySkiba wrote:
> > This change seems to be noop, why are you making it? (Also, the one below.)
> 
> I don't understand your question.
> 
> old code:
> hex(int(x))
> 
> new code:
> 
> hex(int(x) + region_start_address)

Oops, sorry, this line is fine.

But the next line changed from

for frame in symfile.frames_by_address.values()[i + processed_keys_count]:

to

for frame in (symfile.frames_by_address.values()
    [i + processed_keys_count]):

and I wonder why?

https://codereview.chromium.org/2698543003/diff/20001/tracing/bin/symbolize_t...
File tracing/bin/symbolize_trace_macho_reader.py (right):

https://codereview.chromium.org/2698543003/diff/20001/tracing/bin/symbolize_t...
tracing/bin/symbolize_trace_macho_reader.py:25: return int(result.group(1), 16)
On 2017/02/16 21:32:18, erikchen wrote:
> On 2017/02/16 21:19:46, DmitrySkiba wrote:
> > Will int be always enough?
> 
> """
>  In Python 2, Integers will automatically switch to longs when they grow
beyond
> their limit:
> """

Acknowledged.

[erikchen, 2017-02-16 21:43:22]

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

https://codereview.chromium.org/2698543003/diff/20001/tracing/bin/symbolize_t...
tracing/bin/symbolize_trace:119: cmd.extend([hex(int(x) + region_start_address)
On 2017/02/16 21:38:48, DmitrySkiba wrote:
> On 2017/02/16 21:32:18, erikchen wrote:
> > On 2017/02/16 21:19:45, DmitrySkiba wrote:
> > > This change seems to be noop, why are you making it? (Also, the one
below.)
> > 
> > I don't understand your question.
> > 
> > old code:
> > hex(int(x))
> > 
> > new code:
> > 
> > hex(int(x) + region_start_address)
> 
> Oops, sorry, this line is fine.
> 
> But the next line changed from
> 
> for frame in symfile.frames_by_address.values()[i + processed_keys_count]:
> 
> to
> 
> for frame in (symfile.frames_by_address.values()
>     [i + processed_keys_count]):
> 
> and I wonder why?

previous version exceeded 80 column length

[DmitrySkiba, 2017-02-16 22:03:30]

LGTM

[Mark Mentovai, 2017-02-17 18:12:26]

LGTM

[erikchen, 2017-02-17 18:28:23]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2017-02-17 18:28:36]

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-17 18:28:37]

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

[commit-bot: I haz the power, 2017-02-17 18:28:39]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-catapult-committers".
Note that this has nothing to do with OWNERS files.

[erikchen, 2017-02-17 19:04:58]

erikchen@chromium.org changed reviewers:
+ nednguyen@google.com

[erikchen, 2017-02-17 19:04:59]

nednguyen: OWNER stamp please.

[nednguyen, 2017-02-17 19:06:22]

lgtm

[erikchen, 2017-02-17 19:10:12]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2017-02-17 19:10:24]

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-17 19:33:47]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487358612017320,
"parent_rev": "0aecec904fe463d9ea5e3f5e30692bab81f3fe80", "commit_rev":
"5252580c24aa6c4bc322a19c9ad03cd2c94f1382"}

[commit-bot: I haz the power, 2017-02-17 19:33:52]

Description was changed from

==========
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
==========

to

==========
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/catapu...
==========

[commit-bot: I haz the power, 2017-02-17 19:33:53]

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