Add probes for x64.

Add probes for x64.

Committed: https://github.com/google/syzygy/commit/2b686096a86225a6f6a69dee4196d8f6d23b9c87

[Ignat Loskutov, 2016-09-09 20:26:50]

loskutov@google.com changed reviewers:
+ chrisha@chromium.org, sebmarchand@chromium.org

[Ignat Loskutov, 2016-09-09 20:26:59]

PTAL

[Sébastien Marchand, 2016-09-12 19:45:34]

It should be included in a gyp file.

[Ignat Loskutov, 2016-09-12 19:59:28]

On 2016/09/12 19:45:34, Sébastien Marchand wrote:
> It should be included in a gyp file.

Actually it was supposed to be a "WIP" CL to make sure if the code seems to do
the right thing

[Sébastien Marchand, 2016-09-12 20:12:22]

Approach looks good !

https://codereview.chromium.org/2316303008/diff/40001/syzygy/agent/asan/memor...
File syzygy/agent/asan/memory_interceptors_impl_x64.cc (right):

https://codereview.chromium.org/2316303008/diff/40001/syzygy/agent/asan/memor...
syzygy/agent/asan/memory_interceptors_impl_x64.cc:14: 
Please add a global comment describing the content of this file.

https://codereview.chromium.org/2316303008/diff/40001/syzygy/agent/asan/memor...
syzygy/agent/asan/memory_interceptors_impl_x64.cc:26: CONTEXT ctx;
Add " = {}" to initialize this variable.

https://codereview.chromium.org/2316303008/diff/40001/syzygy/agent/asan/memor...
syzygy/agent/asan/memory_interceptors_impl_x64.cc:28: AsanContext asan_ctx;
Ditto.

https://codereview.chromium.org/2316303008/diff/40001/syzygy/agent/asan/memor...
syzygy/agent/asan/memory_interceptors_impl_x64.cc:34: 
There's an extra BL

https://codereview.chromium.org/2316303008/diff/40001/syzygy/agent/asan/memor...
syzygy/agent/asan/memory_interceptors_impl_x64.cc:36: void
asan_check_##access_size##_byte_read_access_no_flags_##suffix(           \
Can you add a comment describing the different variables ? We don't always do
this but in this case the 'heap_size' argument might not be clear to anyone
reading this code.

[Sébastien Marchand, 2016-09-13 15:48:16]

https://codereview.chromium.org/2316303008/diff/60001/syzygy/agent/asan/memor...
File syzygy/agent/asan/memory_interceptors_impl_x64.cc (right):

https://codereview.chromium.org/2316303008/diff/60001/syzygy/agent/asan/memor...
syzygy/agent/asan/memory_interceptors_impl_x64.cc:85: void asan_no_check() {
return; }
I don't think that we want to export the function like this, instead we should
update system_interceptor_generator.py to generate the proper def file depending
on the bitness.

[Sébastien Marchand, 2016-09-13 16:03:09]

https://codereview.chromium.org/2316303008/diff/60001/syzygy/agent/asan/memor...
File syzygy/agent/asan/memory_interceptors_impl_x64.cc (right):

https://codereview.chromium.org/2316303008/diff/60001/syzygy/agent/asan/memor...
syzygy/agent/asan/memory_interceptors_impl_x64.cc:75: #define
EXPORT_INTERCEPTORS_ALL_SIZES(suffix, heap_size) \
One more comment, I don't think that |heap_size| is an appropriate name, maybe
address_space_size ?

[Ignat Loskutov, 2016-09-13 17:59:53]

I've added some new files and fixed the old one, PTAL

[Sébastien Marchand, 2016-09-13 18:23:05]

Nice ! A handful of comments.

https://codereview.chromium.org/2316303008/diff/100001/syzygy/agent/asan/asan...
File syzygy/agent/asan/asan.gyp (right):

https://codereview.chromium.org/2316303008/diff/100001/syzygy/agent/asan/asan...
syzygy/agent/asan/asan.gyp:218: ['target_arch == "x64"', {
Can you try to keep the same logic across the different files ? i.e. Sometime
you to:
if (target_arch == x64) {
...
} else {
...
}

but sometime you check for 'target_arch == ia32' first. I'd prefer to be
consistent and to always do
if (target_arch == ia32) {
...
} else {
...
}

https://codereview.chromium.org/2316303008/diff/100001/syzygy/agent/asan/asan...
syzygy/agent/asan/asan.gyp:223: 'reporters/kasko_reporter_unittest.cc',  # kasko
is to be removed
Move the comment on the previous line, for consistency.

https://codereview.chromium.org/2316303008/diff/100001/syzygy/agent/asan/asan...
syzygy/agent/asan/asan.gyp:227: 'memory_interceptors_unittest.cc',  # relies on
full set of probes
Ditto , here and below

https://codereview.chromium.org/2316303008/diff/100001/syzygy/agent/asan/memo...
File syzygy/agent/asan/memory_interceptors.h (right):

https://codereview.chromium.org/2316303008/diff/100001/syzygy/agent/asan/memo...
syzygy/agent/asan/memory_interceptors.h:158: #endif
This endif and the following ifndef seem unnecessary ?

https://codereview.chromium.org/2316303008/diff/100001/syzygy/agent/asan/memo...
File syzygy/agent/asan/memory_interceptors_impl_x64.cc (right):

https://codereview.chromium.org/2316303008/diff/100001/syzygy/agent/asan/memo...
syzygy/agent/asan/memory_interceptors_impl_x64.cc:26: // @param access_size
Access size in bytes.
nit, for the template parameters we annotate them with '@tparam' (notice the
extra 't')

https://codereview.chromium.org/2316303008/diff/100001/syzygy/agent/asan/memo...
syzygy/agent/asan/memory_interceptors_impl_x64.cc:35: using namespace
agent::asan;
We never use 'using ...' inside a function, do this globally instead.

https://codereview.chromium.org/2316303008/diff/100001/syzygy/agent/asan/memo...
syzygy/agent/asan/memory_interceptors_impl_x64.cc:61: void
asan_check_##access_size##_byte_write_access_no_flags_##suffix(           \
You don't seem to be using the address_space_size argument in the function name
?

https://codereview.chromium.org/2316303008/diff/100001/syzygy/agent/asan/memo...
syzygy/agent/asan/memory_interceptors_impl_x64.cc:88:
EXPORT_INTERCEPTORS_ALL_SIZES(8tb,   static_cast<size_t>(8)   << 40)
The '40' isn't obvious, can you move it to a constant describe what it means ?

https://codereview.chromium.org/2316303008/diff/100001/syzygy/agent/asan/memo...
syzygy/agent/asan/memory_interceptors_impl_x64.cc:88:
EXPORT_INTERCEPTORS_ALL_SIZES(8tb,   static_cast<size_t>(8)   << 40)
There's some extra spaces in front of the static_cast.

https://codereview.chromium.org/2316303008/diff/100001/syzygy/agent/asan/syst...
File syzygy/agent/asan/system_interceptors_x64.def (right):

https://codereview.chromium.org/2316303008/diff/100001/syzygy/agent/asan/syst...
syzygy/agent/asan/system_interceptors_x64.def:17: ; This file is generated by
system_interceptor_generator.py, DO NOT MODIFY.
This isn't true anymore :), update the comment.

[Ignat Loskutov, 2016-09-13 19:05:59]

On 2016/09/13 18:23:05, Sébastien Marchand wrote:
>
https://codereview.chromium.org/2316303008/diff/100001/syzygy/agent/asan/memo...
> syzygy/agent/asan/memory_interceptors_impl_x64.cc:61: void
> asan_check_##access_size##_byte_write_access_no_flags_##suffix(           \
> You don't seem to be using the address_space_size argument in the function
name
> ?
That's true, as |suffix| alters the function name, and |address_space_size|
alters its behavior. They're supposed to express the same thing, one as a string
("128tb"), the other one—as a number (2^47).

The rest is fixed.

[Sébastien Marchand, 2016-09-13 21:11:34]

lgtm % a few comments.

https://codereview.chromium.org/2316303008/diff/140001/syzygy/agent/asan/asan...
File syzygy/agent/asan/asan.gyp (right):

https://codereview.chromium.org/2316303008/diff/140001/syzygy/agent/asan/asan...
syzygy/agent/asan/asan.gyp:227: # Static shadow doesn't work for large address
spaces.
Please remove the BL between each items, we rarely use BL in Gyp files.

https://codereview.chromium.org/2316303008/diff/140001/syzygy/agent/asan/asan...
syzygy/agent/asan/asan.gyp:243: # PE lib for win64 is sort of stub, so there's
nothing to test there
Missing period (I know that it means adding a new line...).

https://codereview.chromium.org/2316303008/diff/140001/syzygy/agent/asan/asan...
syzygy/agent/asan/asan.gyp:246: # IsHeapCorrupt fails (TODO: fix it).
Usually our TODO go on a separate line.

# Disabled because IsHeapCorrupt fails.
# TODO(loskutov): Fit it.

https://codereview.chromium.org/2316303008/diff/140001/syzygy/agent/asan/memo...
File syzygy/agent/asan/memory_interceptors_impl_x64.cc (right):

https://codereview.chromium.org/2316303008/diff/140001/syzygy/agent/asan/memo...
syzygy/agent/asan/memory_interceptors_impl_x64.cc:85: const size_t tb =
static_cast<size_t>(1) << 40;
const size_t ONE_TB = ...

[Ignat Loskutov, 2016-09-13 21:30:24]

The CQ bit was checked by loskutov@google.com

[Ignat Loskutov, 2016-09-13 21:30:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
sebmarchand@chromium.org
Link to the patchset: https://codereview.chromium.org/2316303008/#ps160001
(title: "Style edits.")

[commit-bot: I haz the power, 2016-09-13 21:30:33]

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-09-13 21:36:50]

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

[commit-bot: I haz the power, 2016-09-13 21:36:51]

Try jobs failed on following builders:
  win_rel_try on master.tryserver.client.syzygy (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.syzygy/builders/win_rel_try/bui...)

[Ignat Loskutov, 2016-09-13 21:51:34]

The CQ bit was checked by loskutov@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 21:51:39]

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-09-13 21:58:50]

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

[commit-bot: I haz the power, 2016-09-13 21:58:51]

Dry run: Try jobs failed on following builders:
  win_dbg_try on master.tryserver.client.syzygy (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.syzygy/builders/win_dbg_try/bui...)

[Sébastien Marchand, 2016-09-14 14:21:43]

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

[commit-bot: I haz the power, 2016-09-14 14:21:55]

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-09-14 14:26:44]

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

[commit-bot: I haz the power, 2016-09-14 14:26:45]

Dry run: Try jobs failed on following builders:
  win_rel_try on master.tryserver.client.syzygy (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.syzygy/builders/win_rel_try/bui...)

[Sébastien Marchand, 2016-09-14 14:45:30]

I think that you need to rebase this CL in order to commit it.

[Ignat Loskutov, 2016-09-14 15:12:23]

The CQ bit was checked by loskutov@google.com

[Ignat Loskutov, 2016-09-14 15:12:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
sebmarchand@chromium.org
Link to the patchset: https://codereview.chromium.org/2316303008/#ps180001
(title: "Rebase on the current master.")

[commit-bot: I haz the power, 2016-09-14 15:12:35]

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-09-14 15:53:37]

Description was changed from

==========
Add probes for x64.
==========

to

==========
Add probes for x64.

Committed:
https://github.com/google/syzygy/commit/2b686096a86225a6f6a69dee4196d8f6d23b9c87
==========

[commit-bot: I haz the power, 2016-09-14 15:53:38]

Committed patchset #10 (id:180001) as
https://github.com/google/syzygy/commit/2b686096a86225a6f6a69dee4196d8f6d23b9c87