Fix Win64 build of Dart VM.

Fix Win64 build of Dart VM.

1. Build script tools/build.py now actually builds X64 target when x64
is passed as an architecture:

- GYP files set 'msvs_configuration_platform': 'x64' for Win_x64
configuration;

- NSS related files are fixed to have correct set of defines for ia32
and x64 builds and include right files. A os_windows.c file added to
work-around inability to have different sets of source files per
configuration. Originally this was controlled through target_arch GYP
define and required regeneration of the solution.

- build.py passes Win32 or x64 platform to devenv as part of build
configuration argument;

2. Dart VM now supports Win64 ABI. ABI description is moved into a
separate class CallingConventions in constants_x64.h.

BUG=http://dartbug.com/7792
R=iposva@google.com, ricow@google.com, srdjan@google.com

Committed: https://code.google.com/p/dart/source/detail?r=37078

[Vyacheslav Egorov (Google), 2014-06-04 19:36:19]

Incorporate fix for http://dartbug.com/19213 into this CL and re-enable
corelib/big_integer_vm_test which was originally disabled.

The following was in a old description and no longer applies to this CL:

3. Temporary mark corelib/big_integer_vm_test as expected failure on Win64.
This test reveals a minor difference in what BigintOperations::toDouble returns
for big integers in the uint64 range when compiled for x64 with MSVC. A separate
bug is filed to track the fix for that, see http://dartbug.com/19213.

[Vyacheslav Egorov (Google), 2014-06-05 14:21:51]

+srdjan, siva, fschneider as reviewers

[ricow1, 2014-06-05 14:41:41]

build.py and gyp changes lgtm

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/net/nss.gyp
File runtime/bin/net/nss.gyp (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/net/nss.gyp#...
runtime/bin/net/nss.gyp:81: },
place 
# Added by Dart.
here as well to easily distinguish what we added and what we got from nss

Also, you could comment that these defines are taken from the original
target_arch section below

[srdjan, 2014-06-05 17:00:46]

LGTM

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/net/nss.gyp
File runtime/bin/net/nss.gyp (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/net/nss.gyp#...
runtime/bin/net/nss.gyp:455: # ],
Is the above dead code that should be removed?

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/net/nss.gyp#...
runtime/bin/net/nss.gyp:1356: # ],
ditto?

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/socket_win.cc
File runtime/bin/socket_win.cc (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/socket_win.c...
runtime/bin/socket_win.cc:73: int Socket::Read(intptr_t fd, void* buffer,
intptr_t num_bytes) {
Why is this necessary?

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/assembler_x64.cc
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:2880: // TODO(vegorov): avoid saving XMM0, it is
used only as scratch in out code.
Maybe connect the comment with the definition by asserting FpuTMP == XMM0 ?

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:2901: // TODO(vegorov): avoid saving XMM0, it is
used only as scratch in out code.
ditto

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/constants_x64.h
File runtime/vm/constants_x64.h (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/constants_x64...
runtime/vm/constants_x64.h:153: R(RAX) | R(RCX) | R(RDX) | R(R8) | R(R9) |
R(R10) | R(R11);
4 spaces indent: here and below.

[siva, 2014-06-05 17:21:18]

DBC

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/platform_win.cc
File runtime/bin/platform_win.cc (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/platform_win...
runtime/bin/platform_win.cc:63: for (int i = 0; i < count; i++) free(env[i]);
should this also be for (intptr_t i = 0, ...)

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/socket_win.cc
File runtime/bin/socket_win.cc (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/socket_win.c...
runtime/bin/socket_win.cc:73: int Socket::Read(intptr_t fd, void* buffer,
intptr_t num_bytes) {
Not sure I understand the divergence, num_bytes is an intptr_t and Read returns
the number of bytes read which in most cases would be num_bytes unless there is
a short read, why change the return value type then?

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/constants_x64.h
File runtime/vm/constants_x64.h (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/constants_x64...
runtime/vm/constants_x64.h:141: #define R(reg) (1 << (reg))
R seems cryptic why not REG?

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/stack_frame_x...
File runtime/vm/stack_frame_x64.h (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/stack_frame_x...
runtime/vm/stack_frame_x64.h:56: #endif
#endif  // defined(_WIN64)

[Ivan Posva, 2014-06-05 17:42:11]

LGTM

-Ivan

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/platform_win.cc
File runtime/bin/platform_win.cc (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/platform_win...
runtime/bin/platform_win.cc:63: for (int i = 0; i < count; i++) free(env[i]);
"intptr_t i" in that case too?

And {} please...

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/socket_win.cc
File runtime/bin/socket_win.cc (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/socket_win.c...
runtime/bin/socket_win.cc:73: int Socket::Read(intptr_t fd, void* buffer,
intptr_t num_bytes) {
On 2014/06/05 17:00:46, srdjan wrote:
> Why is this necessary?

I see why this is necessary, but the real problem is that Socket::Read is
defined to return an int and not an intptr_t, which would be the correct thing
to do.

Should be fixed with this CL: https://codereview.chromium.org/312343002/

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/bigint_operat...
File runtime/vm/bigint_operations.cc (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/bigint_operat...
runtime/vm/bigint_operations.cc:491: #if _WIN64
Alternatively we could avoid the ifdef and just do the more involved
instructions always, with the explanation on why we are doing it this way.

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/constants_x64.h
File runtime/vm/constants_x64.h (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/constants_x64...
runtime/vm/constants_x64.h:141: #define R(reg) (1 << (reg))
On 2014/06/05 17:21:18, siva wrote:
> R seems cryptic why not REG?

I don't find R such a bad use here, especially as it is only defined in this one
local section and undefined a could of lines below.

[Vyacheslav Egorov (Google), 2014-06-05 18:05:55]

Comments addressed. I will land as soon as I am in the stable internet
connection zone.

Right now I am on the train and connection is extremely patchy.

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/net/nss.gyp
File runtime/bin/net/nss.gyp (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/net/nss.gyp#...
runtime/bin/net/nss.gyp:81: },
On 2014/06/05 14:41:42, ricow1 wrote:
> place 
> # Added by Dart.
> here as well to easily distinguish what we added and what we got from nss
> 
> Also, you could comment that these defines are taken from the original
> target_arch section below

Done.

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/net/nss.gyp#...
runtime/bin/net/nss.gyp:455: # ],
On 2014/06/05 17:00:46, srdjan wrote:
> Is the above dead code that should be removed?

Apparently we only comment things, but don't remove them to make it easier to
merge nss.gyp from upsteam

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/net/nss.gyp#...
runtime/bin/net/nss.gyp:1356: # ],
On 2014/06/05 17:00:46, srdjan wrote:
> ditto?

see the reasoning above.

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/platform_win.cc
File runtime/bin/platform_win.cc (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/platform_win...
runtime/bin/platform_win.cc:63: for (int i = 0; i < count; i++) free(env[i]);
On 2014/06/05 17:21:18, siva wrote:
> should this also be for (intptr_t i = 0, ...)

Done.

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/platform_win...
runtime/bin/platform_win.cc:63: for (int i = 0; i < count; i++) free(env[i]);
On 2014/06/05 17:42:11, Ivan Posva wrote:
> "intptr_t i" in that case too?
> 
> And {} please...

Done.

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/socket_win.cc
File runtime/bin/socket_win.cc (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/socket_win.c...
runtime/bin/socket_win.cc:73: int Socket::Read(intptr_t fd, void* buffer,
intptr_t num_bytes) {
On 2014/06/05 17:42:11, Ivan Posva wrote:
> On 2014/06/05 17:00:46, srdjan wrote:
> > Why is this necessary?
> 
> I see why this is necessary, but the real problem is that Socket::Read is
> defined to return an int and not an intptr_t, which would be the correct thing
> to do.
> 
> Should be fixed with this CL: https://codereview.chromium.org/312343002/

Thanks. I will revert my changes to this file.

Handle::Read is defined to returned int still though even after your CL.

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/assembler_x64.cc
File runtime/vm/assembler_x64.cc (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:2880: // TODO(vegorov): avoid saving XMM0, it is
used only as scratch in out code.
On 2014/06/05 17:00:46, srdjan wrote:
> Maybe connect the comment with the definition by asserting FpuTMP == XMM0 ?

Well, that would not guarantee that it does not need saving though because it's
all in the hands of register allocator which has code to block FpuTMP for
allocation which is hard to assert from here. I changed the comment though
s/XMM0/FpuTMP/

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/assembler_x64...
runtime/vm/assembler_x64.cc:2901: // TODO(vegorov): avoid saving XMM0, it is
used only as scratch in out code.
On 2014/06/05 17:00:46, srdjan wrote:
> ditto

Done.

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/bigint_operat...
File runtime/vm/bigint_operations.cc (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/bigint_operat...
runtime/vm/bigint_operations.cc:491: #if _WIN64
On 2014/06/05 17:42:11, Ivan Posva wrote:
> Alternatively we could avoid the ifdef and just do the more involved
> instructions always, with the explanation on why we are doing it this way.

I will ifdef for now because right now all other platform agree on the way of
rounding. If we discover more inconsistencies we can switch. Though instead of
using GCC pattern, I would actually use simpler MSVC one (TBH that's how I would
have generated those conversions myself in the first place).

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/constants_x64.h
File runtime/vm/constants_x64.h (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/constants_x64...
runtime/vm/constants_x64.h:141: #define R(reg) (1 << (reg))
On 2014/06/05 17:42:11, Ivan Posva wrote:
> On 2014/06/05 17:21:18, siva wrote:
> > R seems cryptic why not REG?
> 
> I don't find R such a bad use here, especially as it is only defined in this
one
> local section and undefined a could of lines below.

REG is too long I think. I will keep it R() for now, it is just as Ivan says:
very local thing.

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/constants_x64...
runtime/vm/constants_x64.h:153: R(RAX) | R(RCX) | R(RDX) | R(R8) | R(R9) |
R(R10) | R(R11);
On 2014/06/05 17:00:46, srdjan wrote:
> 4 spaces indent: here and below.

Done.

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/stack_frame_x...
File runtime/vm/stack_frame_x64.h (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/vm/stack_frame_x...
runtime/vm/stack_frame_x64.h:56: #endif
On 2014/06/05 17:21:18, siva wrote:
> #endif  // defined(_WIN64)

Done.

[Ivan Posva, 2014-06-05 18:13:39]

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/socket_win.cc
File runtime/bin/socket_win.cc (right):

https://codereview.chromium.org/317773002/diff/20001/runtime/bin/socket_win.c...
runtime/bin/socket_win.cc:73: int Socket::Read(intptr_t fd, void* buffer,
intptr_t num_bytes) {
On 2014/06/05 18:05:55, Vyacheslav Egorov (Google) wrote:
> On 2014/06/05 17:42:11, Ivan Posva wrote:
> > On 2014/06/05 17:00:46, srdjan wrote:
> > > Why is this necessary?
> > 
> > I see why this is necessary, but the real problem is that Socket::Read is
> > defined to return an int and not an intptr_t, which would be the correct
thing
> > to do.
> > 
> > Should be fixed with this CL: https://codereview.chromium.org/312343002/
> 
> Thanks. I will revert my changes to this file.
> 
> Handle::Read is defined to returned int still though even after your CL.

Please file a bug against IO and check with Anders what the lower layers of
Windows provide on 64-bit machines.

[Vyacheslav Egorov (Google), 2014-06-06 12:14:34]

Committed patchset #3 manually as r37078 (presubmit successful).