CallIC and KeyedCallIC not wrapping this.

CallIC and KeyedCallIC are not wrapping receiver for calls to strict mode and builtin functions.

BUG=
TEST=

[Martin Maly, 2011-02-16 04:56:17]

Attempt at CallIC not wrapping "this" for strict mode target functions. Only
CallIC so far, not KeyedCallIC (though they call each other so parts may
actually work).

The idea is to delay ReceiverToObject call until we've had a chance to sniff the
function being called. If non-strict, wrap, if strict, keep value as-is.

I need to poke around some more to see if I caught all cases but am hoping for
early feedback.

Thank you!
Martin

http://codereview.chromium.org/6523052/diff/2001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6523052/diff/2001/src/ic.cc#newcode464
src/ic.cc:464: // TODO(mmaly): One cannot hang number-indexed functions off of
values, right?
I couldn't come up with a way where value could actually be valid here. I may be
wrong though...

[Mads Ager (chromium), 2011-02-16 10:43:54]

As far as I can tell this should work. When you have removed the TODOs this
LGTM. Then you can double check if all the keyed call cases are covered.

http://codereview.chromium.org/6523052/diff/2001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6523052/diff/2001/src/ic.cc#newcode464
src/ic.cc:464: // TODO(mmaly): One cannot hang number-indexed functions off of
values, right?
On 2011/02/16 04:56:17, Martin Maly wrote:
> I couldn't come up with a way where value could actually be valid here. I may
be
> wrong though...

I think that is because we have a bug here. I think you should leave this code
as is. GetElement should deal with prototypes for value objects the same way
that GetProperty does.

The case that we should hit here is:

Number.prototype[0] = function() { print('asdf'); }
(0)["0"]()

We do not hit this and we throw an error. This is inconsistent and we should
change it. Let's file a bug report and deal with that in a separate change.

http://codereview.chromium.org/6523052/diff/2001/src/ic.cc#newcode490
src/ic.cc:490: // TODO(mmaly): Should we do ToObject here?
Nope, there is no change to this code and it makes sense to use the value object
for error reporting. That is what you attempted to call something on.

The ReceiverToObject call only changes the receiver on the stack I believe, so
you have not made any change to behavior here.

[Vitaly Repeshko, 2011-02-16 11:52:12]

Drive by comments.

http://codereview.chromium.org/6523052/diff/2001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6523052/diff/2001/src/ic.cc#newcode511
src/ic.cc:511: ReceiverToObject(object);
1. Since this is now done after UpdateCaches, please fix TryUpdateExtraICState.
It assumes it's invoked with a wrapped receiver.

2. For builtin functions it would be nice to avoid wrapping the receiver. It
could be as simple as checking whether the function is a builtin here. If turns
out to be more involved feel free to ignore this suggestion.

[Lasse Reichstein, 2011-02-16 13:34:29]

Just a comment.

http://codereview.chromium.org/6523052/diff/2001/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/6523052/diff/2001/src/arm/stub-cache-arm.cc#ne...
src/arm/stub-cache-arm.cc:2335: if (!function->IsBuiltin() &&
!function_info->strict_mode()) {
It would be great if we could just make all builtin functions strict, and drop
the IsBuiltin() check.

[Martin Maly, 2011-02-17 05:25:55]

Implemented KeyedCallIC, incorporated CR feedback (except for marking all
builtins strict - will investigate this part later), and adding more tests to
cover keyed call ICs and using strict/non-strict getters.

Thank you!
Martin

http://codereview.chromium.org/6523052/diff/2001/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/6523052/diff/2001/src/arm/stub-cache-arm.cc#ne...
src/arm/stub-cache-arm.cc:2335: if (!function->IsBuiltin() &&
!function_info->strict_mode()) {
On 2011/02/16 13:34:30, Lasse Reichstein wrote:
> It would be great if we could just make all builtin functions strict, and drop
> the IsBuiltin() check.

Good idea, Lasse. If it is ok, I'll put this on my todo list rather than include
here. I looked for some place where all builtin functions are going through when
they are materialized (all of C, Asm and JS builtins) but am still looking for
the right spot where to flip the bit on the function's shared info.

http://codereview.chromium.org/6523052/diff/2001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6523052/diff/2001/src/ic.cc#newcode464
src/ic.cc:464: // TODO(mmaly): One cannot hang number-indexed functions off of
values, right?
On 2011/02/16 10:43:54, Mads Ager wrote:
> On 2011/02/16 04:56:17, Martin Maly wrote:
> > I couldn't come up with a way where value could actually be valid here. I
may
> be
> > wrong though...
> 
> I think that is because we have a bug here. I think you should leave this code
> as is. GetElement should deal with prototypes for value objects the same way
> that GetProperty does.
> 
> The case that we should hit here is:
> 
> Number.prototype[0] = function() { print('asdf'); }
> (0)["0"]()
> 
> We do not hit this and we throw an error. This is inconsistent and we should
> change it. Let's file a bug report and deal with that in a separate change.
> 

Done.

http://codereview.chromium.org/6523052/diff/2001/src/ic.cc#newcode490
src/ic.cc:490: // TODO(mmaly): Should we do ToObject here?
On 2011/02/16 10:43:54, Mads Ager wrote:
> Nope, there is no change to this code and it makes sense to use the value
object
> for error reporting. That is what you attempted to call something on.
> 
> The ReceiverToObject call only changes the receiver on the stack I believe, so
> you have not made any change to behavior here.

Done.

http://codereview.chromium.org/6523052/diff/2001/src/ic.cc#newcode511
src/ic.cc:511: ReceiverToObject(object);
On 2011/02/16 11:52:12, Vitaly wrote:
> 1. Since this is now done after UpdateCaches, please fix
TryUpdateExtraICState.
> It assumes it's invoked with a wrapped receiver.
> 
> 2. For builtin functions it would be nice to avoid wrapping the receiver. It
> could be as simple as checking whether the function is a builtin here. If
turns
> out to be more involved feel free to ignore this suggestion.

Done and done. for the builtin change I ran full test battery in debug mode, all
clean, so I suppose it was as easy as checking for builtin function here. Thanks
for the suggestion.

http://codereview.chromium.org/6523052/diff/11001/src/ic.cc#newcode437
src/ic.cc:437: 
Since the logic now includes checking the function, and it is used in 2 places,
I lifted it into renamed ReceiverToobject...

http://codereview.chromium.org/6523052/diff/11001/src/ic.cc#newcode582
src/ic.cc:582: index = Smi::cast(args[1])->value();
Good catch, Vitaly, this assert did trigger in debug mode tests as it did depend
on wrapping.

http://codereview.chromium.org/6523052/diff/11001/src/ic.h
File src/ic.h (right):

http://codereview.chromium.org/6523052/diff/11001/src/ic.h#newcode227
src/ic.h:227: void ReceiverToObjectIfRequired(Object* callee, Handle<Object>
object);
I am very open to suggestions on a better name.

[Mads Ager (chromium), 2011-02-17 09:52:51]

LGTM

Are there no tests in the es5conform test suite that we start passing because of
this change?

http://codereview.chromium.org/6523052/diff/10002/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6523052/diff/10002/src/ic.cc#newcode442
src/ic.cc:442: if (function->shared()->strict_mode() ||
Does this fit on one line?

http://codereview.chromium.org/6523052/diff/10002/src/ic.cc#newcode519
src/ic.cc:519: // functions do not wrap the receiver, non-strict functions do.
non-strict functions and objects called as functions do?

http://codereview.chromium.org/6523052/diff/10002/src/ic.cc#newcode813
src/ic.cc:813: // functions do not wrap the receiver, non-strict functions do.
Ditto.

[Martin Maly, 2011-02-17 17:01:03]

Apparently, no es5conform strict mode test exercise this. They all use
Function.call and Function.apply as far as I could see.

http://codereview.chromium.org/6523052/diff/10002/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6523052/diff/10002/src/ic.cc#newcode442
src/ic.cc:442: if (function->shared()->strict_mode() ||
On 2011/02/17 09:52:51, Mads Ager wrote:
> Does this fit on one line?

Done.

http://codereview.chromium.org/6523052/diff/10002/src/ic.cc#newcode519
src/ic.cc:519: // functions do not wrap the receiver, non-strict functions do.
On 2011/02/17 09:52:51, Mads Ager wrote:
> non-strict functions and objects called as functions do?

Done.

http://codereview.chromium.org/6523052/diff/10002/src/ic.cc#newcode813
src/ic.cc:813: // functions do not wrap the receiver, non-strict functions do.
On 2011/02/17 09:52:51, Mads Ager wrote:
> Ditto.

Done.

[Mads Ager (chromium), 2011-02-17 21:57:01]

http://codereview.chromium.org/6523052/diff/12004/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6523052/diff/12004/src/ic.cc#newcode815
src/ic.cc:815: ReceiverToObjectIfRequired(result, object);
I should have caught this earlier: result is a raw pointer and you are using it
after this call. This call can allocate which can cause a gc which can move the
result. We need the result handlified before calling ReceiverToObjectIfRequired.

The same is the case above.

[Martin Maly, 2011-02-18 00:55:38]

Thanks for the additional info, Mads.
with this change I am a bit hazy on proper use of handles, returning handle
values (EscapeFrom vs. "return *handle").
Hopefully this version is close to correct.

Thanks!
Martin

http://codereview.chromium.org/6523052/diff/15001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode449
src/ic.cc:449: if (object->IsString() || object->IsNumber() ||
object->IsBoolean()) {
I don't understand why the object was double-wrapped in a handle. Based on my
understanding it is not necessary but if it was necessary I wonder why the
Handle<Object> object which arrives straight from the IC. I am assuming GC will
discover that object by walking the stack.

http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode541
src/ic.cc:541: Handle<JSFunction> function(JSFunction::cast(*result));
No HandleScope needed here, I believe ... the Handle will simply use the one
above (line 508).

http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode552
src/ic.cc:552: if (result->IsJSFunction()) return *result;
I am a bit hazy on using
"return handle.EscapeFrom(&scope)"
vs.
"return *handle;"
I saw both in the codebase...

http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode809
src/ic.cc:809: result = Handle<Object>(object_result);
Is there an established V8 pattern to export objects out of the nested scope and
Handle-ify it?

http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode821
src/ic.cc:821: return TypeError("property_not_function", object, key);
This sequence seemed cleaner, get rid of "answer" altogether.

[Mads Ager (chromium), 2011-02-18 07:22:48]

A couple of suggestions to make the code clearer. Once those are addressed we
are ready to go.

http://codereview.chromium.org/6523052/diff/15001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode449
src/ic.cc:449: if (object->IsString() || object->IsNumber() ||
object->IsBoolean()) {
On 2011/02/18 00:55:38, Martin Maly wrote:
> I don't understand why the object was double-wrapped in a handle. Based on my
> understanding it is not necessary but if it was necessary I wonder why the
> Handle<Object> object which arrives straight from the IC. I am assuming GC
will
> discover that object by walking the stack.

Yes, there is no reason to rewrap the incoming objects.

http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode509
src/ic.cc:509: Handle<Object> result;
I would move the object_result out here (maybe call it result and call the
handle result_handle.) Then only create the HandleScope after the GetProperty
and ToObject part.

Then you can add a comment that you wrap the result in a handle because
ReceiverToObjectIfRequired can cause a GC.

It would be nice to just use a handlified version of GetProperty but that would
redo work that you already did, so I would probably leave the raw pointers stuff
in here.

http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode522
src/ic.cc:522: if (lookup.type() == INTERCEPTOR) {
It looks like you can move the wrapping below this exception throwing step as
well. If you throw an exception from here, there is no reason to wrap the
receiver. However, the more important part is that LookupResults are not GC
safe. I believe it is safe for the things you are using here (not accessing any
heap object pointers stored on the LookupResult). In general you have to be
careful with LookupResults. To avoid having to think about it I would move the
wrapping to after the final checks on lookup.

http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode533
src/ic.cc:533: ASSERT(*result != Heap::the_hole_value());
ASSERT(!result->IsTheHole());

http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode541
src/ic.cc:541: Handle<JSFunction> function(JSFunction::cast(*result));
On 2011/02/18 00:55:38, Martin Maly wrote:
> No HandleScope needed here, I believe ... the Handle will simply use the one
> above (line 508).

Yes, that's fine.

http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode552
src/ic.cc:552: if (result->IsJSFunction()) return *result;
On 2011/02/18 00:55:38, Martin Maly wrote:
> I am a bit hazy on using
> "return handle.EscapeFrom(&scope)"
> vs.
> "return *handle;"
> I saw both in the codebase...

This method returns a MaybeObject which is a raw object pointer. It can either
be a Smi, an Object pointer or a failure. The caller will deal with failures
(either throwing exceptions or garbage collecting or just returning the result).
So here, you just have to extract the raw Object* from the handle and return
that. This is done with "return *handle".

handle.EscapeFrom(&scope) is used rarely. It is used in situations where you
have a local handle scope and you want to return one of those handles to the
caller (who has his own handle scope). If you just return the handle you will
get into trouble because that handle belongs to your local handle scope. Once
the local handle scope gets destructed as part of returning, the GC no longer
knows about the value in your handle and you are no better off than with a raw
pointer. handle.EscapeFrom(&scope) moves the handle from the current scope to
the outer scope which makes it safe to return it. The outer scope is still
active after returning and the GC will therefore update the pointer in the
handle.

http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode805
src/ic.cc:805: Handle<Object> result;
This case can be rewritten to just use handles. You don't need any of the
maybe_object stuff below and you can just call the handlified version of
GetObjectProperty (defined in handles.[h,cc]:

Handle<Object> result = GetObjectProperty(object, key);

http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode821
src/ic.cc:821: return TypeError("property_not_function", object, key);
On 2011/02/18 00:55:38, Martin Maly wrote:
> This sequence seemed cleaner, get rid of "answer" altogether.

Yes, thanks for cleaning that up! The original code was very strange. :)

[Martin Maly, 2011-02-19 01:26:13]

Fixed up. Thanks for the details on handles and handle scopes.

I'll wait for an OK before I actually commit this.

Martin

http://codereview.chromium.org/6523052/diff/15001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode509
src/ic.cc:509: Handle<Object> result;
On 2011/02/18 07:22:48, Mads Ager wrote:
> I would move the object_result out here (maybe call it result and call the
> handle result_handle.) Then only create the HandleScope after the GetProperty
> and ToObject part.
> 
> Then you can add a comment that you wrap the result in a handle because
> ReceiverToObjectIfRequired can cause a GC.
> 
> It would be nice to just use a handlified version of GetProperty but that
would
> redo work that you already did, so I would probably leave the raw pointers
stuff
> in here.

Done.

http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode522
src/ic.cc:522: if (lookup.type() == INTERCEPTOR) {
On 2011/02/18 07:22:48, Mads Ager wrote:
> It looks like you can move the wrapping below this exception throwing step as
> well. If you throw an exception from here, there is no reason to wrap the
> receiver. However, the more important part is that LookupResults are not GC
> safe. I believe it is safe for the things you are using here (not accessing
any
> heap object pointers stored on the LookupResult). In general you have to be
> careful with LookupResults. To avoid having to think about it I would move the
> wrapping to after the final checks on lookup.

Done.

http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode533
src/ic.cc:533: ASSERT(*result != Heap::the_hole_value());
On 2011/02/18 07:22:48, Mads Ager wrote:
> ASSERT(!result->IsTheHole());

Done.

http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode805
src/ic.cc:805: Handle<Object> result;
On 2011/02/18 07:22:48, Mads Ager wrote:
> This case can be rewritten to just use handles. You don't need any of the
> maybe_object stuff below and you can just call the handlified version of
> GetObjectProperty (defined in handles.[h,cc]:
> 
> Handle<Object> result = GetObjectProperty(object, key);

Done.

[Mads Ager (chromium), 2011-02-21 07:45:11]

LGTM!

http://codereview.chromium.org/6523052/diff/7018/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6523052/diff/7018/src/ic.cc#newcode441
src/ic.cc:441: JSFunction* function = JSFunction::cast(*callee);
Let's keep it all in handles here to not mix handlified and non-handlified code
when we can easily avoid it.

Handle<JSFuntion> function = Handle<JSFunction>::cast(callee);

[Martin Maly, 2011-02-22 00:40:35]

Done. Thanks!

http://codereview.chromium.org/6523052/diff/7018/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6523052/diff/7018/src/ic.cc#newcode441
src/ic.cc:441: JSFunction* function = JSFunction::cast(*callee);
On 2011/02/21 07:45:11, Mads Ager wrote:
> Let's keep it all in handles here to not mix handlified and non-handlified
code
> when we can easily avoid it.
> 
> Handle<JSFuntion> function = Handle<JSFunction>::cast(callee);

Done.