NewGC: Don't put tagged null pointers in garbage objects when updating pointers

NewGC: Don't put tagged null pointers in garbage objects when updating pointers
to dead new space objects.  These null pointers can be found later by the
evacuation pointer updater, which will crash.
Committed: http://code.google.com/p/v8/source/detail?r=8917

[Vyacheslav Egorov (Chromium), 2011-08-12 08:39:10]

LGTM!

[Lasse Reichstein, 2011-08-12 08:47:12]

LGTM

http://codereview.chromium.org/7635014/diff/1/src/mark-compact.cc
File src/mark-compact.cc (right):

http://codereview.chromium.org/7635014/diff/1/src/mark-compact.cc#newcode2373
src/mark-compact.cc:2373: // with NULL. In this case we do not need to transfer
this entry to
Does this mean we store NULL in the heap? We should probably call it something
else (Smi::ValueOf(0)) just for precission.

http://codereview.chromium.org/7635014/diff/1/src/mark-compact.cc#newcode2381
src/mark-compact.cc:2381: *p = reinterpret_cast<HeapObject*>(Smi::FromInt(0));
So we are not assuming that the value is HeapObject tagged anywhere? (Quite
possible, since we can't use it as such anyway).
It seems to end up in NewSpace::Contains which just uses address arithmetic, so
it should be safe (but that probably should take a HeapObject and filter out
smis earler). 
Maybe make StoreBuffer::Uniq and similar functions filter out the smis.

[Erik Corry, 2011-08-12 08:56:40]

http://codereview.chromium.org/7635014/diff/1/src/mark-compact.cc
File src/mark-compact.cc (right):

http://codereview.chromium.org/7635014/diff/1/src/mark-compact.cc#newcode2373
src/mark-compact.cc:2373: // with NULL. In this case we do not need to transfer
this entry to
On 2011/08/12 08:47:13, Lasse Reichstein wrote:
> Does this mean we store NULL in the heap? We should probably call it something
> else (Smi::ValueOf(0)) just for precission.

It doesn't really have anything to do with a Smi. The map field of the dead
object has to contain something that is heap object tagged, because otherwise it
looks like a forwarding pointer (can't have Smis in the map field).

http://codereview.chromium.org/7635014/diff/1/src/mark-compact.cc#newcode2381
src/mark-compact.cc:2381: *p = reinterpret_cast<HeapObject*>(Smi::FromInt(0));
On 2011/08/12 08:47:13, Lasse Reichstein wrote:
> So we are not assuming that the value is HeapObject tagged anywhere?

This is the value in the slot that used to point to new space.  The slot is now
part of a dead object pointing to a dead object so we have to put something
there that neither the scan-on-scavenge page traversal nor the evacuation slot
buffer will choke on.  A Smi zero should be fine.  The signature of this
function gets a little in the way, but changing that is likely to cause more
pain.

[Lasse Reichstein, 2011-08-12 09:21:48]

http://codereview.chromium.org/7635014/diff/1/src/mark-compact.cc
File src/mark-compact.cc (right):

http://codereview.chromium.org/7635014/diff/1/src/mark-compact.cc#newcode2373
src/mark-compact.cc:2373: // with NULL. In this case we do not need to transfer
this entry to
So the comment is incorrect and we don't store NULL, but "HeapObject tagged
NULL"?

http://codereview.chromium.org/7635014/diff/1/src/mark-compact.cc#newcode2381
src/mark-compact.cc:2381: *p = reinterpret_cast<HeapObject*>(Smi::FromInt(0));
Ah, so *p is always in the heap, and not in a store-buffer?