Avoid handle dereference during graph optimization.

Avoid handle dereference during graph optimization.

With parallel recompilation enabled, objects made accessible by handles may
have changed between graph construction and graph optimization. Therefore
we must not assume that information on those objects remain the same between
those two phases. To police this, we forbid handle dereferencing during
graph optimization.
Exceptions to this rule are:
 - Dereferencing the handle to obtain the raw location of the object. This
   is safe since parallel recompilation acquires RelocationLock
 - Some places that dereference the handle for a type check. These are checked
   to be safe on a case-by-case basis.

R=jkummerow@chromium.org
BUG=

Committed: https://code.google.com/p/v8/source/detail?r=13475

[Yang, 2013-01-22 12:08:47]

Please take a look.

[Jakob Kummerow, 2013-01-23 12:09:18]

https://codereview.chromium.org/12049012/diff/2001/src/handles-inl.h
File src/handles-inl.h (right):

https://codereview.chromium.org/12049012/diff/2001/src/handles-inl.h#newcode183
src/handles-inl.h:183: active_ =
!isolate->optimizing_compiler_thread()->IsOptimizerThread();
I'd appreciate a comment here, something along the lines of: "The guard can only
be enabled for all threads at once, so we can only do that when parallel
compilation is disabled. Checking for the current thread is sufficient to detect
that, because this constructor is only ever called from the optimizer thread."
While typing this it occurred to me: why not just hook this off the
FLAG_parallel_recompilation?

https://codereview.chromium.org/12049012/diff/2001/src/handles.h
File src/handles.h (right):

https://codereview.chromium.org/12049012/diff/2001/src/handles.h#newcode68
src/handles.h:68: return *location_ == *other.location();
Why "location_" vs "location()"? Let's be consistent.

https://codereview.chromium.org/12049012/diff/2001/src/handles.h#newcode352
src/handles.h:352: int old_state_;
This is used to store bools, so it should be a bool.

https://codereview.chromium.org/12049012/diff/2001/src/handles.h#newcode367
src/handles.h:367: int old_state_;
Same here.

https://codereview.chromium.org/12049012/diff/2001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/12049012/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:2671: // HConstant(const HConstant&);
what's this?

https://codereview.chromium.org/12049012/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:2701: if (*handle_.location() ==
Object::cast(heap->undefined_value()) ||
"if (a || b) {return true;} return false;" is kind of equivalent to "return (a
|| b);", no?
I don't feel strongly about it; if you think this is more readable feel free to
leave it as it is.

https://codereview.chromium.org/12049012/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:2761: hash =
reinterpret_cast<intptr_t>(*handle_.location());
So, dereferencing the handle is not allowed, but dereferencing its location()
is? Who are we kidding?

[Yang, 2013-01-23 13:34:25]

Addressed comments. Please re-review :)

https://codereview.chromium.org/12049012/diff/2001/src/handles-inl.h
File src/handles-inl.h (right):

https://codereview.chromium.org/12049012/diff/2001/src/handles-inl.h#newcode183
src/handles-inl.h:183: active_ =
!isolate->optimizing_compiler_thread()->IsOptimizerThread();
On 2013/01/23 12:09:18, Jakob wrote:
> I'd appreciate a comment here, something along the lines of: "The guard can
only
> be enabled for all threads at once, so we can only do that when parallel
> compilation is disabled. Checking for the current thread is sufficient to
detect
> that, because this constructor is only ever called from the optimizer thread."
> While typing this it occurred to me: why not just hook this off the
> FLAG_parallel_recompilation?

Done.

https://codereview.chromium.org/12049012/diff/2001/src/handles.h
File src/handles.h (right):

https://codereview.chromium.org/12049012/diff/2001/src/handles.h#newcode68
src/handles.h:68: return *location_ == *other.location();
On 2013/01/23 12:09:18, Jakob wrote:
> Why "location_" vs "location()"? Let's be consistent.

Done.

https://codereview.chromium.org/12049012/diff/2001/src/handles.h#newcode352
src/handles.h:352: int old_state_;
On 2013/01/23 12:09:18, Jakob wrote:
> This is used to store bools, so it should be a bool.

Done.

https://codereview.chromium.org/12049012/diff/2001/src/handles.h#newcode367
src/handles.h:367: int old_state_;
On 2013/01/23 12:09:18, Jakob wrote:
> Same here.

Done.

https://codereview.chromium.org/12049012/diff/2001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/12049012/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:2671: // HConstant(const HConstant&);
On 2013/01/23 12:09:18, Jakob wrote:
> what's this?

stray edit. gone now.

https://codereview.chromium.org/12049012/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:2701: if (*handle_.location() ==
Object::cast(heap->undefined_value()) ||
On 2013/01/23 12:09:18, Jakob wrote:
> "if (a || b) {return true;} return false;" is kind of equivalent to "return (a
> || b);", no?
> I don't feel strongly about it; if you think this is more readable feel free
to
> leave it as it is.

completely overlooked this. done.

https://codereview.chromium.org/12049012/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:2761: hash =
reinterpret_cast<intptr_t>(*handle_.location());
On 2013/01/23 12:09:18, Jakob wrote:
> So, dereferencing the handle is not allowed, but dereferencing its location()
> is? Who are we kidding?

like we discussed offline, I added the guard to location() as well.

[Jakob Kummerow, 2013-01-23 13:49:03]

lgtm