Ensure double alignment when deserializing.

Ensure double alignment when deserializing.

R=rmcilroy@chromium.org
BUG=chromium:436510
LOG=N
TEST=compile with V8_OOL_CONSTANT_POOL=1, run d8 with --verify-heap --test

[rmcilroy, 2014-11-26 17:44:40]

lgtm, with some comments.  Thanks for doing this Yang!

https://codereview.chromium.org/759823006/diff/40001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/759823006/diff/40001/src/heap/heap.cc#newcode...
src/heap/heap.cc:1833: return EnsureDoubleAligned(this, object, size);
Could you add an ASSERT(object->NeedsToEnsureDoubleAlignment()) in
EnsureDoubleAligned to ensure that the list in NeedsToEnsureDoubleAlignment
keeps in sync with objects which call EnsureDoubleAligned during allocation.

https://codereview.chromium.org/759823006/diff/40001/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/759823006/diff/40001/src/serialize.cc#newcode817
src/serialize.cc:817: address = Allocate(space_number, size + kPointerSize);
You should only need to add kPointerSize if not a 64bit architecture (this is
what the Heap::AllocateXXX calls do).

[Yang, 2014-11-26 18:59:49]

https://codereview.chromium.org/759823006/diff/40001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/759823006/diff/40001/src/heap/heap.cc#newcode...
src/heap/heap.cc:1833: return EnsureDoubleAligned(this, object, size);
On 2014/11/26 17:44:40, rmcilroy wrote:
> Could you add an ASSERT(object->NeedsToEnsureDoubleAlignment()) in
> EnsureDoubleAligned to ensure that the list in NeedsToEnsureDoubleAlignment
> keeps in sync with objects which call EnsureDoubleAligned during allocation.

Unfortunately, at this point, the object is no more than an allocation. It
doesn't have a map yet, so we can't check for its object type.

I could add a DCHECK(object->NeedsToEnsureDoubleAlignment) at the call sites 
(AllocateFixedDoubleArray etc.) after the map is set, though.

https://codereview.chromium.org/759823006/diff/40001/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/759823006/diff/40001/src/serialize.cc#newcode817
src/serialize.cc:817: address = Allocate(space_number, size + kPointerSize);
On 2014/11/26 17:44:40, rmcilroy wrote:
> You should only need to add kPointerSize if not a 64bit architecture (this is
> what the Heap::AllocateXXX calls do).

I think this entire if-branch can be omitted for 64bit archs?

[rmcilroy, 2014-11-26 19:24:46]

https://codereview.chromium.org/759823006/diff/40001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/759823006/diff/40001/src/heap/heap.cc#newcode...
src/heap/heap.cc:1833: return EnsureDoubleAligned(this, object, size);
On 2014/11/26 18:59:49, Yang wrote:
> On 2014/11/26 17:44:40, rmcilroy wrote:
> > Could you add an ASSERT(object->NeedsToEnsureDoubleAlignment()) in
> > EnsureDoubleAligned to ensure that the list in NeedsToEnsureDoubleAlignment
> > keeps in sync with objects which call EnsureDoubleAligned during allocation.
> 
> Unfortunately, at this point, the object is no more than an allocation. It
> doesn't have a map yet, so we can't check for its object type.
> 
> I could add a DCHECK(object->NeedsToEnsureDoubleAlignment) at the call sites 
> (AllocateFixedDoubleArray etc.) after the map is set, though.

Right enough.  Your suggestion stgt. It doesn't quite capture the check, but
would hopefully help someone realise they should add to this list if adding a
new double aligned object.

https://codereview.chromium.org/759823006/diff/40001/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/759823006/diff/40001/src/serialize.cc#newcode817
src/serialize.cc:817: address = Allocate(space_number, size + kPointerSize);
On 2014/11/26 18:59:49, Yang wrote:
> On 2014/11/26 17:44:40, rmcilroy wrote:
> > You should only need to add kPointerSize if not a 64bit architecture (this
is
> > what the Heap::AllocateXXX calls do).
> 
> I think this entire if-branch can be omitted for 64bit archs?

agreed.

[Yang, 2014-11-27 08:53:17]

On 2014/11/26 19:24:52, rmcilroy wrote:

Turns out adding the assertion is not straightforward either:
- In case of FixedDoubleArrays, the caller doesn't set the map. The caller of
the caller does.
- NeedsToEnsureDoubleAlignment always returns false for 64 bit systems.
Asserting that would simply fail. I made it return false so that it can be
folded away.

So I guess I'll just leave it the way it is.

I uploaded a new patch set that changes the encoding scheme a bit.

[Yang, 2014-11-27 08:53:20]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2014-11-27 08:54:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/759823006/60001

[commit-bot: I haz the power, 2014-11-27 09:22:18]

Committed patchset #4 (id:60001)