Fixed a flaky unittest that shows only on Release 64 bit.

Fixed a flaky unittest that shows only on Release 64 bit.

BUG=https://github.com/google/syzygy/issues/55

[njanevsk, 2017-06-05 19:00:31]

Description was changed from

==========
Fixed a flaky unittest that shows only on Release 64 bit.

BUG=
==========

to

==========
Fixed a flaky unittest that shows only on Release 64 bit.

BUG=
==========

[njanevsk, 2017-06-05 19:00:32]

njanevsk@google.com changed reviewers:
+ etienne.bergeron@gmail.com, sebmarchand@chromium.org

[njanevsk, 2017-06-06 17:49:43]

PTAL

[njanevsk, 2017-06-06 18:14:34]

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

[commit-bot: I haz the power, 2017-06-06 18:14:37]

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, 2017-06-06 18:14:39]

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

[commit-bot: I haz the power, 2017-06-06 18:14:40]

Dry run: No L-G-T-M from a valid reviewer yet.
CQ run can only be started 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-syzygy-committers".
Note that this has nothing to do with OWNERS files.

[Sébastien Marchand, 2017-06-06 18:30:23]

sebmarchand@chromium.org changed reviewers:
+ chrisha@chromium.org

[Sébastien Marchand, 2017-06-06 18:30:23]

That's not really a fix, you're just making this problem less likely to happen
by assuming that we're always running on a 64-bit system that always support
128TB of memory. 

Currently these tests work by using an array of (Clang)InterceptFunction[1] that
contain all the asan_check_* probes, in 32-bit it look like this:
{
  {"asan_load4_2gb, 4},
  {"asan_load4_4gb, 4},
  {"asan_load8_2gb, 8},
  {"asan_load8_4gb, 8},
  ...
}

And in 64-bit it's the same thing but with the 8tb/128tb suffixes. (And in
32-bit you also have the SyzyAsan variants, "asan_check_4_byte_read_access"
etc).

The problem with this test is that the probes assume that the address that get
passed to them doesn't exceed the address space. Our dev machine have a 128tb
address space (for the 64-bit processes), so an allocation can easily have an
address superior to 8tb and testing the asan_..._8tb probes will fail for this
address.

It also mean that the 32-bit unittests could fail the same way (e.g. something
get allocated in the > 2GB range when testing the 2gb probes).

I think that the proper way to fix this would be to modify the
(clang_)intercept_functions* arrays in memory_interceptors_unittests, instead of
being arrays of "InterceptFunction" you'll define a new structure that'll look
like this:

struct CoolNewStruct {
#ifndef _WIN64
  InterceptFunction probe_2g;
  InterceptFunction probe_4g;
#else
  InterceptFunction probe_8tb;
  InterceptFunction probe_128tb;
#endif
};

And then you'll choose the appropriate probe at runtime depending on the machine
configuration. You'll need to modify TestMemoryInterceptors::TestValidAccess etc
to use the appropriate probe. You can check the size of the address space by
calling ::GlobalMemoryStatusEx (we do this in Shadow::RequiredLength and
syzyasan_rtl.cc:SelectMemoryAccessorMode)

This isn't a simple fix, and I'm not sure if it's worth for you to spend time
fixing this, Chris, wdyt?

Also, make sure to use everyone's @chromium address (you used Etienne's Gmail
account here).

[1]
https://github.com/google/syzygy/blob/master/syzygy/agent/asan/unittest_util....

[njanevsk, 2017-06-06 18:45:27]

njanevsk@google.com changed reviewers:
+ etienneb@chromium.org
- etienne.bergeron@gmail.com

[Sébastien Marchand, 2017-06-06 22:15:17]

Also, most of the trybots that you see in the "Choose trybots" panel are for
Chrome, only use the one under master.tryserver.client.syzygy

[chrisha, 2017-06-13 20:19:39]

Potentially we could just extend ClangInterceptFunction to have a third field
"AddressSpaceType"? And then specify 2GB, 4GB, 8TB, 128TB as appropriate in this
field? And then at runtime skip tests that don't match the bitness /
address-space size of the machine?

[chrisha, 2017-06-15 21:06:32]

We should close this CL, given that it's the wrong way to fix this.

[njanevsk, 2017-06-16 14:55:19]

On 2017/06/15 21:06:32, chrisha wrote:
> We should close this CL, given that it's the wrong way to fix this.

Will close it. Do you have a tool for creating bugs and assigning them?
If so I can create a bug for this assigned it to myself and work on it when I
have time since we decided it is a low priority issue.

[Sébastien Marchand, 2017-06-16 14:57:00]

You can use https://github.com/google/syzygy/issues

[njanevsk, 2017-06-16 15:05:29]

Description was changed from

==========
Fixed a flaky unittest that shows only on Release 64 bit.

BUG=
==========

to

==========
Fixed a flaky unittest that shows only on Release 64 bit.

BUG=https://github.com/google/syzygy/issues/55
==========

[njanevsk, 2017-06-16 15:06:33]

On 2017/06/16 14:57:00, Sébastien Marchand wrote:
> You can use https://github.com/google/syzygy/issues

OK. Created it: https://github.com/google/syzygy/issues/55

Don't know how to assign it.

Closed this CL.