Port some more code to x64

Port some more code to x64

Committed: https://github.com/google/syzygy/commit/7e7fbfbb43d76faca0cc9ea6f5de7255345fc4c7

[Ignat Loskutov, 2016-08-02 22:10:56]

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

[Ignat Loskutov, 2016-08-02 22:11:04]

PTAL

[chrisha, 2016-08-03 17:40:06]

How about moving stack_walker_x86.h to stack_walker.h, and simply precompiler
hiding things that aren't generic?

And then having a stack_walker_x86.cc and stack_walker_x64.cc that implements
the function, conditionally brought in via the build system?

https://codereview.chromium.org/2194383007/diff/1/syzygy/agent/common/agent.cc
File syzygy/agent/common/agent.cc (right):

https://codereview.chromium.org/2194383007/diff/1/syzygy/agent/common/agent.c...
syzygy/agent/common/agent.cc:34: // cannot be disabled.
Add to this comment:

The 64-bit version of the runtime is intended to run with LLVM/Clang provided
instrumentation, and doesn't suffer to same problem as the Syzygy
instrumentation.

https://codereview.chromium.org/2194383007/diff/1/syzygy/agent/common/stack_w...
File syzygy/agent/common/stack_walker.cc (right):

https://codereview.chromium.org/2194383007/diff/1/syzygy/agent/common/stack_w...
syzygy/agent/common/stack_walker.cc:30: }
Call this stack_walker_x64.cc?

https://codereview.chromium.org/2194383007/diff/1/syzygy/agent/common/stack_w...
File syzygy/agent/common/stack_walker.h (right):

https://codereview.chromium.org/2194383007/diff/1/syzygy/agent/common/stack_w...
syzygy/agent/common/stack_walker.h:41: StackId* absolute_stack_id);
This definition is identical to the 32 and 64 bit implementations. Simply remove
the redundant definition from stack_walker_x86.h?

[Sébastien Marchand, 2016-08-03 17:58:28]

In think that I'd prefer having only a pair of stack_walker.h/cc files with some
ifdef _WIN64 in them. I'm not entirely sure of this but I think that having
different set of source files depending on the bitness might cause some issues
in VS ? (does it reload the projects when you change the target platform ?).

It's also probably less efficient for crash bucketing, but this is not really an
issue for us I think.

[Ignat Loskutov, 2016-08-03 18:51:50]

Fixed.

[Sébastien Marchand, 2016-08-04 13:30:52]

https://codereview.chromium.org/2194383007/diff/40001/syzygy/agent/common/sta...
File syzygy/agent/common/stack_walker.cc (right):

https://codereview.chromium.org/2194383007/diff/40001/syzygy/agent/common/sta...
syzygy/agent/common/stack_walker.cc:102: size_t __declspec(noinline)
WalkStack(size_t bottom_frames_to_skip,
You need to update this signature as well (uint32 -> size_t), I'll also move the
#ifdef block inside of it.

[Ignat Loskutov, 2016-08-04 14:58:02]

Fixed.

[Sébastien Marchand, 2016-08-04 18:26:32]

lgtm with one nit.

https://codereview.chromium.org/2194383007/diff/60001/third_party/pcre/pcre.gyp
File third_party/pcre/pcre.gyp (right):

https://codereview.chromium.org/2194383007/diff/60001/third_party/pcre/pcre.g...
third_party/pcre/pcre.gyp:100: 'AdditionalOptions': ['/wd4267', '/wd4130',
'/wd4018', '/wd4189', '/wd4996'],
Can you split this on multiple lines (to avoid exceeding 80 chars):
'AdditionalOptions': [
  '/wd...',
  '/wd...',
],


I think that you can use the 'msvs_disabled_warnings' flag instead of this, see
an example here:
https://github.com/google/syzygy/blob/master/syzygy/crashdata/crashdata.gyp#L51

[Ignat Loskutov, 2016-08-04 18:56:56]

The CQ bit was checked by loskutov@google.com

[Ignat Loskutov, 2016-08-04 18:56:56]

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/2194383007/#ps80001
(title: "Fix MSVS disabled warnings list.")

[commit-bot: I haz the power, 2016-08-04 18:57:01]

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-08-04 19:08:12]

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

[commit-bot: I haz the power, 2016-08-04 19:08:13]

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

[Ignat Loskutov, 2016-08-04 19:48:49]

Fixed the compilation error. The name of stack_walker_x86_unittest.cc looks
inconsistent with stack_walker.cc, but it can actually test the x86
implementation only, so I don't know if it's a good idea to rename it.

[Sébastien Marchand, 2016-08-04 19:50:33]

Is there some unittests that can be shared between the 2 versions ? If not could
you add some tests for the x64 one ?

[Ignat Loskutov, 2016-08-04 20:03:54]

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

[commit-bot: I haz the power, 2016-08-04 20:04:00]

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-08-04 20:50:51]

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

[commit-bot: I haz the power, 2016-08-04 20:50:53]

Dry run: This issue passed the CQ dry run.

[Ignat Loskutov, 2016-08-04 21:13:43]

Shared a unittest (which actually compares the implementation with itself for
x64, still helped to find an off-by-one error) and renamed the class.

[Sébastien Marchand, 2016-08-04 21:15:24]

lgtm, it'll help with the win64 coverage.

[Ignat Loskutov, 2016-08-04 21:17:28]

The CQ bit was checked by loskutov@google.com

[commit-bot: I haz the power, 2016-08-04 21:17:30]

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-08-04 22:04:09]

Description was changed from

==========
Port some more code to x64
==========

to

==========
Port some more code to x64

Committed:
https://github.com/google/syzygy/commit/7e7fbfbb43d76faca0cc9ea6f5de7255345fc4c7
==========

[commit-bot: I haz the power, 2016-08-04 22:04:11]

Committed patchset #7 (id:120001) as
https://github.com/google/syzygy/commit/7e7fbfbb43d76faca0cc9ea6f5de7255345fc4c7