X64: Omitted creating native builtins and using them in test-heap.

X64: Omitted creating native builtins and using them in test-heap.
This allows test-heap to run successfully.

[Lasse Reichstein, 2009-05-22 11:51:01]

Review please

[William Hesse, 2009-05-22 12:21:13]

LGTM.  We don't want to have an explosion of #ifdef X64 exclusions in the main
V8 code, but these seem unavoidable.

http://codereview.chromium.org/115706/diff/1/2
File src/bootstrapper.cc (right):

http://codereview.chromium.org/115706/diff/1/2#newcode1536
Line 1536: #ifndef V8_HOST_ARCH_64_BIT
This should be marked with a TODO to remove it when the compiler is working.

http://codereview.chromium.org/115706/diff/1/7
File test/cctest/test-heap.cc (right):

http://codereview.chromium.org/115706/diff/1/7#newcode191
Line 191: ASSERT_EQ(request, OBJECT_SIZE_ALIGN(request));
Shouldn't this be CHECK_EQ?  Otherwise, you could have check failures later in
release mode, because of a misaligned request.

[Kevin Millikin (Chromium), 2009-05-22 12:47:48]

I wonder if we should be stubbing out function calls or function implementations
instead (or you could argue that you have stubbed out (part of) an
implementation....).

http://codereview.chromium.org/115706/diff/1/2
File src/bootstrapper.cc (right):

http://codereview.chromium.org/115706/diff/1/2#newcode1537
Line 1537: if (!InstallNatives()) return;
There may be a general documentation advantage to moving the #ifdef (with a
TODO) inside Bootstrapper::InstallNatives.  It more directly marks that function
(rather than this call site) as one that needs to be looked at.

The cost is that it probably has to become an #ifndef ... #else ... #endif.

http://codereview.chromium.org/115706/diff/1/4
File src/heap.h (right):

http://codereview.chromium.org/115706/diff/1/4#newcode1052
Line 1052: ASSERT(Page::IsRSetSet(reinterpret_cast<Address>(current), 0));
Same comment---Page::IsRSetSet could return true in V8_HOST_ARCH_64_BIT.

[Lasse Reichstein, 2009-05-22 13:10:10]

Good comments.

http://codereview.chromium.org/115706/diff/1/2
File src/bootstrapper.cc (right):

http://codereview.chromium.org/115706/diff/1/2#newcode1537
Line 1537: if (!InstallNatives()) return;
I have moved the ifdef inside the function, and even restricted it to dropping
only some of the function body (from where it starts compiling stuff).

http://codereview.chromium.org/115706/diff/1/4
File src/heap.h (right):

http://codereview.chromium.org/115706/diff/1/4#newcode1052
Line 1052: ASSERT(Page::IsRSetSet(reinterpret_cast<Address>(current), 0));
Done.
Returning true should be a safe approximation (unless someone is asserting that
it returns false somewhere).

http://codereview.chromium.org/115706/diff/1/7
File test/cctest/test-heap.cc (right):

http://codereview.chromium.org/115706/diff/1/7#newcode191
Line 191: ASSERT_EQ(request, OBJECT_SIZE_ALIGN(request));
I couldn't decide. It's not a check in the same sense as the following ones.
They check that something works as intended.  This is more like a sanity check
for the test itself. I.e., if this fails, it's not a bug in the code being
tested, but in the test itself - equivalent to ASSERTs in the main code.