Make syzyasan_rtl compile in 64 bit

Make syzyasan_rtl compile in 64 bit

CQ_INCLUDE_TRYBOTS=master.tryserver.client.syzygy:win_x64_rel_try;master.tryserver.client.syzygy:win_x64_dbg_try

Committed: https://github.com/google/syzygy/commit/9a972cd1732c0f196cf227650e97af6f51728e97

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

Description was changed from

==========
More fixes to the Release_x64 configuration.
==========

to

==========
WIP: More fixes to the Release_x64 configuration.
==========

[Sébastien Marchand, 2016-09-22 21:50:15]

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

[Sébastien Marchand, 2016-09-22 21:52:10]

Hey,

here's the change to fix some of the tests in the Release_x64 config. It's still
a WIP but I wanted you to share this with you.

[Ignat Loskutov, 2016-09-22 22:00:36]

https://codereview.chromium.org/2363733003/diff/1/syzygy/agent/asan/error_inf...
File syzygy/agent/asan/error_info_unittest.cc (left):

https://codereview.chromium.org/2363733003/diff/1/syzygy/agent/asan/error_inf...
syzygy/agent/asan/error_info_unittest.cc:399: "    \"address\": \"0x%08zX\",\n"
What's wrong about %08zX?

[Ignat Loskutov, 2016-09-22 22:05:13]

Actually, MSDN claims that "The hh, j, z, and t length prefixes are not
supported", although it seemed to work for me. It suggests the "I" prefix
instead.

[Sébastien Marchand, 2016-09-28 15:36:48]

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

[Sébastien Marchand, 2016-09-28 15:36:49]

Here's the changes that I had to make to get the unittests to pass.

[Sébastien Marchand, 2016-10-13 18:08:14]

Description was changed from

==========
WIP: More fixes to the Release_x64 configuration.
==========

to

==========
Make syzyasan_rtl compile in 64 bit
==========

[Sébastien Marchand, 2016-10-13 18:08:25]

PTAL.

[Sébastien Marchand, 2016-10-13 18:18:55]

hold on, I had to rebase and some things broke.

[Sébastien Marchand, 2016-10-13 19:32:26]

Patchset #7 (id:120001) has been deleted

[Sébastien Marchand, 2016-10-13 19:36:48]

Ok, PTAL, I had to fix a bug in the ShadowWalker.

[chrisha, 2016-10-13 20:22:49]

https://codereview.chromium.org/2363733003/diff/140001/syzygy/agent/asan/logg...
File syzygy/agent/asan/logger_unittest.cc (right):

https://codereview.chromium.org/2363733003/diff/140001/syzygy/agent/asan/logg...
syzygy/agent/asan/logger_unittest.cc:134: // Crashpad.
Missing TODO(sebmarchand) somewhere here too?

https://codereview.chromium.org/2363733003/diff/140001/syzygy/agent/asan/shad...
File syzygy/agent/asan/shadow.cc (right):

https://codereview.chromium.org/2363733003/diff/140001/syzygy/agent/asan/shad...
syzygy/agent/asan/shadow.cc:899: if
(!AddressIsInCommittedMemory((&shadow_[left])))
This is going to cost a VirtualQuery per value of |left|, which is pretty
expensive. The virtual query gives us bounds of mapped memory, so we should
actually first do a VirtualQuery for the location, get bounds, and then do
normal checking against those bounds. If we hit the bounds and haven't found the
block start marker, then we return false.

(On Win32, the bounds are simply initialized to the entire range of the shadow
memory.)

https://codereview.chromium.org/2363733003/diff/140001/syzygy/agent/asan/shad...
syzygy/agent/asan/shadow.cc:1173: shadow_cursor_ = end_of_region;
Oops! My bad. New unittest for this?

[Sébastien Marchand, 2016-10-13 21:05:45]

https://codereview.chromium.org/2363733003/diff/140001/syzygy/agent/asan/logg...
File syzygy/agent/asan/logger_unittest.cc (right):

https://codereview.chromium.org/2363733003/diff/140001/syzygy/agent/asan/logg...
syzygy/agent/asan/logger_unittest.cc:134: // Crashpad.
On 2016/10/13 20:22:49, chrisha (slow) wrote:
> Missing TODO(sebmarchand) somewhere here too?

Done.

https://codereview.chromium.org/2363733003/diff/140001/syzygy/agent/asan/shad...
File syzygy/agent/asan/shadow.cc (right):

https://codereview.chromium.org/2363733003/diff/140001/syzygy/agent/asan/shad...
syzygy/agent/asan/shadow.cc:899: if
(!AddressIsInCommittedMemory((&shadow_[left])))
On 2016/10/13 20:22:49, chrisha (slow) wrote:
> This is going to cost a VirtualQuery per value of |left|, which is pretty
> expensive. The virtual query gives us bounds of mapped memory, so we should
> actually first do a VirtualQuery for the location, get bounds, and then do
> normal checking against those bounds. If we hit the bounds and haven't found
the
> block start marker, then we return false.
> 
> (On Win32, the bounds are simply initialized to the entire range of the shadow
> memory.)

No, in the loop I'm evaluating the following condition:
    if (&shadow_[left] < page_begin &&
        !AddressIsInCommittedMemory((&shadow_[left]))) {

so we will call AddressIsInCommittedMemory only once per page, not for every
value of |left|. But I agree that using the bound returned by VirtualQuery is
probably a better solution, on it.

[Sébastien Marchand, 2016-10-14 15:35:42]

Patchset #9 (id:180001) has been deleted

[Sébastien Marchand, 2016-10-14 15:35:59]

Patchset #9 (id:200001) has been deleted

[Sébastien Marchand, 2016-10-14 19:04:14]

Thanks, PTAnL.

https://codereview.chromium.org/2363733003/diff/140001/syzygy/agent/asan/shad...
File syzygy/agent/asan/shadow.cc (right):

https://codereview.chromium.org/2363733003/diff/140001/syzygy/agent/asan/shad...
syzygy/agent/asan/shadow.cc:899: if
(!AddressIsInCommittedMemory((&shadow_[left])))
On 2016/10/13 21:05:45, Sébastien Marchand wrote:
> On 2016/10/13 20:22:49, chrisha (slow) wrote:
> > This is going to cost a VirtualQuery per value of |left|, which is pretty
> > expensive. The virtual query gives us bounds of mapped memory, so we should
> > actually first do a VirtualQuery for the location, get bounds, and then do
> > normal checking against those bounds. If we hit the bounds and haven't found
> the
> > block start marker, then we return false.
> > 
> > (On Win32, the bounds are simply initialized to the entire range of the
shadow
> > memory.)
> 
> No, in the loop I'm evaluating the following condition:
>     if (&shadow_[left] < page_begin &&
>         !AddressIsInCommittedMemory((&shadow_[left]))) {
> 
> so we will call AddressIsInCommittedMemory only once per page, not for every
> value of |left|. But I agree that using the bound returned by VirtualQuery is
> probably a better solution, on it.

Ha, didn't realized that VirtualQuery return the information about the whole
segment (not just the page), nice.

https://codereview.chromium.org/2363733003/diff/140001/syzygy/agent/asan/shad...
syzygy/agent/asan/shadow.cc:1173: shadow_cursor_ = end_of_region;
On 2016/10/13 20:22:49, chrisha (slow) wrote:
> Oops! My bad. New unittest for this?

Addressing this in https://codereview.chromium.org/2421523003.

[chrisha, 2016-10-14 19:53:13]

lgtm!

https://codereview.chromium.org/2363733003/diff/240001/syzygy/agent/asan/logg...
File syzygy/agent/asan/logger_unittest.cc (right):

https://codereview.chromium.org/2363733003/diff/240001/syzygy/agent/asan/logg...
syzygy/agent/asan/logger_unittest.cc:131: // The 64 bit version of the logger
doesn't support writting minidumps yet
writing*

https://codereview.chromium.org/2363733003/diff/240001/syzygy/agent/asan/logg...
syzygy/agent/asan/logger_unittest.cc:135: // TODO(sebmarchand): Add support for
generating minidump to the 64 bit
minidumps* ... 64-bit*

[Sébastien Marchand, 2016-10-14 20:03:13]

Description was changed from

==========
Make syzyasan_rtl compile in 64 bit
==========

to

==========
Make syzyasan_rtl compile in 64 bit

CQ_INCLUDE_TRYBOTS=master.tryserver.tryserver.client.syzygy:win_x64_rel_try,
master.tryserver.tryserver.client.syzygy:win_x64_dbg_try
==========

[Sébastien Marchand, 2016-10-14 20:03:30]

The CQ bit was checked by sebmarchand@chromium.org

[Sébastien Marchand, 2016-10-14 20:03:31]

The patchset sent to the CQ was uploaded after l-g-t-m from chrisha@chromium.org
Link to the patchset: https://codereview.chromium.org/2363733003/#ps260001
(title: "Nits.")

[commit-bot: I haz the power, 2016-10-14 20:03:37]

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-10-14 20:03:37]

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

[commit-bot: I haz the power, 2016-10-14 20:03:38]

Your CL can not be processed by CQ because of:
 * Failed to parse additional trybots

[Sébastien Marchand, 2016-10-14 20:04:03]

Thanks, committing.

https://codereview.chromium.org/2363733003/diff/240001/syzygy/agent/asan/logg...
File syzygy/agent/asan/logger_unittest.cc (right):

https://codereview.chromium.org/2363733003/diff/240001/syzygy/agent/asan/logg...
syzygy/agent/asan/logger_unittest.cc:131: // The 64 bit version of the logger
doesn't support writting minidumps yet
On 2016/10/14 19:53:13, chrisha (slow) wrote:
> writing*

Done.

https://codereview.chromium.org/2363733003/diff/240001/syzygy/agent/asan/logg...
syzygy/agent/asan/logger_unittest.cc:135: // TODO(sebmarchand): Add support for
generating minidump to the 64 bit
On 2016/10/14 19:53:13, chrisha (slow) wrote:
> minidumps* ... 64-bit*

Done.

[Sébastien Marchand, 2016-10-14 20:05:30]

Description was changed from

==========
Make syzyasan_rtl compile in 64 bit

CQ_INCLUDE_TRYBOTS=master.tryserver.tryserver.client.syzygy:win_x64_rel_try,
master.tryserver.tryserver.client.syzygy:win_x64_dbg_try
==========

to

==========
Make syzyasan_rtl compile in 64 bit

CQ_INCLUDE_TRYBOTS=master.tryserver.client.syzygy:win_x64_rel_try,
master.tryserver.client.syzygy:win_x64_dbg_try
==========

[Sébastien Marchand, 2016-10-14 20:05:35]

The CQ bit was checked by sebmarchand@chromium.org

[commit-bot: I haz the power, 2016-10-14 20:05:37]

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-10-14 20:05:38]

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

[commit-bot: I haz the power, 2016-10-14 20:05:40]

Your CL can not be processed by CQ because of:
 * Failed to parse additional trybots

[Sébastien Marchand, 2016-10-14 20:07:30]

Description was changed from

==========
Make syzyasan_rtl compile in 64 bit

CQ_INCLUDE_TRYBOTS=master.tryserver.client.syzygy:win_x64_rel_try,
master.tryserver.client.syzygy:win_x64_dbg_try
==========

to

==========
Make syzyasan_rtl compile in 64 bit

CQ_INCLUDE_TRYBOTS=master.tryserver.client.syzygy:win_x64_rel_try;master.tryserver.client.syzygy:win_x64_dbg_try
==========

[Sébastien Marchand, 2016-10-14 20:07:36]

The CQ bit was checked by sebmarchand@chromium.org

[commit-bot: I haz the power, 2016-10-14 20:07:48]

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-10-14 20:43:04]

Description was changed from

==========
Make syzyasan_rtl compile in 64 bit

CQ_INCLUDE_TRYBOTS=master.tryserver.client.syzygy:win_x64_rel_try;master.tryserver.client.syzygy:win_x64_dbg_try
==========

to

==========
Make syzyasan_rtl compile in 64 bit

CQ_INCLUDE_TRYBOTS=master.tryserver.client.syzygy:win_x64_rel_try;master.tryserver.client.syzygy:win_x64_dbg_try

Committed:
https://github.com/google/syzygy/commit/9a972cd1732c0f196cf227650e97af6f51728e97
==========

[commit-bot: I haz the power, 2016-10-14 20:43:05]

Committed patchset #11 (id:260001) as
https://github.com/google/syzygy/commit/9a972cd1732c0f196cf227650e97af6f51728e97