Skip to content

Commit 62af07e

Browse files
isheludkoV8 LUCI CQ
isheludko
authored and
V8 LUCI CQ
committed
[ic] Cleanup AccessorAssembler::CallGetterIfAccessor()
This CL - reorders parameters to make |expected_receiver_mode| a mandatory one and properly computed, - makes sure we don't pass PropertyCell as a holder when JSReceiver is expected. Bug: 450328966 Change-Id: I921dfbd99245d01143600b4f4713fe602c817657 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7036691 Commit-Queue: Igor Sheludko <[email protected]> Reviewed-by: Leszek Swirski <[email protected]> Cr-Commit-Position: refs/heads/main@{#103085}
1 parent 1422aaf commit 62af07e

File tree

4 files changed

+144
-110
lines changed

4 files changed

+144
-110
lines changed

‎src/codegen/code-stub-assembler.cc‎

Lines changed: 101 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -11662,7 +11662,8 @@ void CodeStubAssembler::ForEachEnumerableOwnProperty(
1166211662

1166311663
var_value = CallGetterIfAccessor(
1166411664
value_or_accessor, object, var_details.value(), context,
11665-
object, next_key, &slow_load, kCallJSGetterUseCachedName);
11665+
object, kExpectingJSReceiver, next_key, &slow_load,
11666+
kCallJSGetterUseCachedName);
1166611667
Goto(&value_ready);
1166711668

1166811669
BIND(&slow_load);
@@ -12158,15 +12159,11 @@ template void CodeStubAssembler::LoadPropertyFromDictionary(
1215812159
TNode<SwissNameDictionary> dictionary, TNode<IntPtrT> name_index,
1215912160
TVariable<Uint32T>* var_details, TVariable<Object>* var_value);
1216012161

12161-
// |value| is the property backing store's contents, which is either a value or
12162-
// an accessor pair, as specified by |details|. |holder| is a JSReceiver or a
12163-
// PropertyCell. Returns either the original value, or the result of the getter
12164-
// call.
1216512162
TNode<Object> CodeStubAssembler::CallGetterIfAccessor(
12166-
TNode<Object> value, TNode<Union<JSReceiver, PropertyCell>> holder,
12163+
TNode<Object> value, std::optional<TNode<JSReceiver>> holder,
1216712164
TNode<Uint32T> details, TNode<Context> context, TNode<JSAny> receiver,
12168-
TNode<Object> name, Label* if_bailout, GetOwnPropertyMode mode,
12169-
ExpectedReceiverMode expected_receiver_mode) {
12165+
ExpectedReceiverMode expected_receiver_mode, TNode<Object> name,
12166+
Label* if_bailout, GetOwnPropertyMode mode) {
1217012167
TVARIABLE(Object, var_value, value);
1217112168
Label done(this), if_accessor_info(this, Label::kDeferred);
1217212169

@@ -12207,44 +12204,51 @@ TNode<Object> CodeStubAssembler::CallGetterIfAccessor(
1220712204

1220812205
BIND(&if_function_template_info);
1220912206
{
12210-
Label use_cached_property(this);
12211-
TNode<HeapObject> cached_property_name = LoadObjectField<HeapObject>(
12212-
getter, FunctionTemplateInfo::kCachedPropertyNameOffset);
12213-
12214-
Label* has_cached_property = mode == kCallJSGetterUseCachedName
12215-
? &use_cached_property
12216-
: if_bailout;
12217-
GotoIfNot(IsTheHole(cached_property_name), has_cached_property);
12218-
12219-
TNode<JSReceiver> js_receiver;
12220-
switch (expected_receiver_mode) {
12221-
case kExpectingJSReceiver:
12222-
js_receiver = CAST(receiver);
12223-
break;
12224-
case kExpectingAnyReceiver:
12225-
// TODO(ishell): in case the function template info has a signature
12226-
// and receiver is not a JSReceiver the signature check in
12227-
// CallFunctionTemplate builtin will fail anyway, so we can short
12228-
// cut it here and throw kIllegalInvocation immediately.
12229-
js_receiver = ToObject_Inline(context, receiver);
12230-
break;
12231-
}
12232-
TNode<JSReceiver> holder_receiver = CAST(holder);
12233-
TNode<NativeContext> creation_context =
12234-
GetCreationContext(holder_receiver, if_bailout);
12235-
TNode<Context> caller_context = context;
12236-
var_value = CallBuiltin(
12237-
Builtin::kCallFunctionTemplate_Generic, creation_context, getter,
12238-
Int32Constant(i::JSParameterCount(0)), caller_context, js_receiver);
12239-
Goto(&done);
12207+
if (holder.has_value()) {
12208+
Label use_cached_property(this);
12209+
TNode<HeapObject> cached_property_name = LoadObjectField<HeapObject>(
12210+
getter, FunctionTemplateInfo::kCachedPropertyNameOffset);
12211+
12212+
Label* has_cached_property = mode == kCallJSGetterUseCachedName
12213+
? &use_cached_property
12214+
: if_bailout;
12215+
GotoIfNot(IsTheHole(cached_property_name), has_cached_property);
12216+
12217+
TNode<JSReceiver> js_receiver;
12218+
switch (expected_receiver_mode) {
12219+
case kExpectingJSReceiver:
12220+
js_receiver = CAST(receiver);
12221+
break;
12222+
case kExpectingAnyReceiver:
12223+
// TODO(ishell): in case the function template info has a
12224+
// signature and receiver is not a JSReceiver the signature check
12225+
// in CallFunctionTemplate builtin will fail anyway, so we can
12226+
// short cut it here and throw kIllegalInvocation immediately.
12227+
js_receiver = ToObject_Inline(context, receiver);
12228+
break;
12229+
}
12230+
TNode<JSReceiver> holder_receiver = *holder;
12231+
TNode<NativeContext> creation_context =
12232+
GetCreationContext(holder_receiver, if_bailout);
12233+
TNode<Context> caller_context = context;
12234+
var_value = CallBuiltin(Builtin::kCallFunctionTemplate_Generic,
12235+
creation_context, getter,
12236+
Int32Constant(i::JSParameterCount(0)),
12237+
caller_context, js_receiver);
12238+
Goto(&done);
1224012239

12241-
if (mode == kCallJSGetterUseCachedName) {
12242-
Bind(&use_cached_property);
12240+
if (mode == kCallJSGetterUseCachedName) {
12241+
Bind(&use_cached_property);
1224312242

12244-
var_value =
12245-
GetProperty(context, holder_receiver, cached_property_name);
12243+
var_value =
12244+
GetProperty(context, holder_receiver, cached_property_name);
1224612245

12247-
Goto(&done);
12246+
Goto(&done);
12247+
}
12248+
} else {
12249+
// |holder| must be available in order to handle lazy AccessorPair
12250+
// case (we need it for computing the function's context).
12251+
Unreachable();
1224812252
}
1224912253
}
1225012254
} else {
@@ -12256,56 +12260,61 @@ TNode<Object> CodeStubAssembler::CallGetterIfAccessor(
1225612260
// AccessorInfo case.
1225712261
BIND(&if_accessor_info);
1225812262
{
12259-
TNode<AccessorInfo> accessor_info = CAST(value);
12260-
Label if_array(this), if_function(this), if_wrapper(this);
12261-
12262-
// Dispatch based on {holder} instance type.
12263-
TNode<Map> holder_map = LoadMap(holder);
12264-
TNode<Uint16T> holder_instance_type = LoadMapInstanceType(holder_map);
12265-
GotoIf(IsJSArrayInstanceType(holder_instance_type), &if_array);
12266-
GotoIf(IsJSFunctionInstanceType(holder_instance_type), &if_function);
12267-
Branch(IsJSPrimitiveWrapperInstanceType(holder_instance_type), &if_wrapper,
12268-
if_bailout);
12269-
12270-
// JSArray AccessorInfo case.
12271-
BIND(&if_array);
12272-
{
12273-
// We only deal with the "length" accessor on JSArray.
12274-
GotoIfNot(IsLengthString(
12275-
LoadObjectField(accessor_info, AccessorInfo::kNameOffset)),
12276-
if_bailout);
12277-
TNode<JSArray> array = CAST(holder);
12278-
var_value = LoadJSArrayLength(array);
12279-
Goto(&done);
12280-
}
12281-
12282-
// JSFunction AccessorInfo case.
12283-
BIND(&if_function);
12284-
{
12285-
// We only deal with the "prototype" accessor on JSFunction here.
12286-
GotoIfNot(IsPrototypeString(
12287-
LoadObjectField(accessor_info, AccessorInfo::kNameOffset)),
12288-
if_bailout);
12263+
if (holder.has_value()) {
12264+
TNode<AccessorInfo> accessor_info = CAST(value);
12265+
Label if_array(this), if_function(this), if_wrapper(this);
12266+
// Dispatch based on {holder} instance type.
12267+
TNode<Map> holder_map = LoadMap(*holder);
12268+
TNode<Uint16T> holder_instance_type = LoadMapInstanceType(holder_map);
12269+
GotoIf(IsJSArrayInstanceType(holder_instance_type), &if_array);
12270+
GotoIf(IsJSFunctionInstanceType(holder_instance_type), &if_function);
12271+
Branch(IsJSPrimitiveWrapperInstanceType(holder_instance_type),
12272+
&if_wrapper, if_bailout);
12273+
12274+
// JSArray AccessorInfo case.
12275+
BIND(&if_array);
12276+
{
12277+
// We only deal with the "length" accessor on JSArray.
12278+
GotoIfNot(IsLengthString(LoadObjectField(accessor_info,
12279+
AccessorInfo::kNameOffset)),
12280+
if_bailout);
12281+
TNode<JSArray> array = CAST(*holder);
12282+
var_value = LoadJSArrayLength(array);
12283+
Goto(&done);
12284+
}
1228912285

12290-
TNode<JSFunction> function = CAST(holder);
12291-
GotoIfPrototypeRequiresRuntimeLookup(function, holder_map, if_bailout);
12292-
var_value = LoadJSFunctionPrototype(function, if_bailout);
12293-
Goto(&done);
12294-
}
12286+
// JSFunction AccessorInfo case.
12287+
BIND(&if_function);
12288+
{
12289+
// We only deal with the "prototype" accessor on JSFunction here.
12290+
GotoIfNot(IsPrototypeString(LoadObjectField(accessor_info,
12291+
AccessorInfo::kNameOffset)),
12292+
if_bailout);
12293+
12294+
TNode<JSFunction> function = CAST(*holder);
12295+
GotoIfPrototypeRequiresRuntimeLookup(function, holder_map, if_bailout);
12296+
var_value = LoadJSFunctionPrototype(function, if_bailout);
12297+
Goto(&done);
12298+
}
1229512299

12296-
// JSPrimitiveWrapper AccessorInfo case.
12297-
BIND(&if_wrapper);
12298-
{
12299-
// We only deal with the "length" accessor on JSPrimitiveWrapper string
12300-
// wrappers.
12301-
GotoIfNot(IsLengthString(
12302-
LoadObjectField(accessor_info, AccessorInfo::kNameOffset)),
12303-
if_bailout);
12304-
TNode<Object> holder_value = LoadJSPrimitiveWrapperValue(CAST(holder));
12305-
GotoIfNot(TaggedIsNotSmi(holder_value), if_bailout);
12306-
GotoIfNot(IsString(CAST(holder_value)), if_bailout);
12307-
var_value = LoadStringLengthAsSmi(CAST(holder_value));
12308-
Goto(&done);
12300+
// JSPrimitiveWrapper AccessorInfo case.
12301+
BIND(&if_wrapper);
12302+
{
12303+
// We only deal with the "length" accessor on JSPrimitiveWrapper string
12304+
// wrappers.
12305+
GotoIfNot(IsLengthString(LoadObjectField(accessor_info,
12306+
AccessorInfo::kNameOffset)),
12307+
if_bailout);
12308+
TNode<Object> holder_value = LoadJSPrimitiveWrapperValue(CAST(*holder));
12309+
GotoIfNot(TaggedIsNotSmi(holder_value), if_bailout);
12310+
GotoIfNot(IsString(CAST(holder_value)), if_bailout);
12311+
var_value = LoadStringLengthAsSmi(CAST(holder_value));
12312+
Goto(&done);
12313+
}
12314+
} else {
12315+
// |holder| must be available in order to handle AccessorInfo case (we
12316+
// need to pass it to the callback).
12317+
Unreachable();
1230912318
}
1231012319
}
1231112320

@@ -12390,7 +12399,7 @@ void CodeStubAssembler::TryGetOwnProperty(
1239012399
}
1239112400
TNode<Object> value = CallGetterIfAccessor(
1239212401
var_value->value(), object, var_details->value(), context, receiver,
12393-
unique_name, if_bailout, mode, expected_receiver_mode);
12402+
expected_receiver_mode, unique_name, if_bailout, mode);
1239412403
*var_value = value;
1239512404
Goto(if_found_value);
1239612405
}

‎src/codegen/code-stub-assembler.h‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4646,12 +4646,16 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
46464646
const ForEachKeyValueFunction& body,
46474647
Label* bailout);
46484648

4649+
// |value| is the property backing store's contents, which is either a value
4650+
// or an accessor pair, as specified by |details|. |holder| is a JSReceiver
4651+
// or empty std::nullopt if holder is not available.
4652+
// Returns either the original value, or the result of the getter call.
46494653
TNode<Object> CallGetterIfAccessor(
4650-
TNode<Object> value, TNode<Union<JSReceiver, PropertyCell>> holder,
4654+
TNode<Object> value, std::optional<TNode<JSReceiver>> holder,
46514655
TNode<Uint32T> details, TNode<Context> context, TNode<JSAny> receiver,
4652-
TNode<Object> name, Label* if_bailout,
4653-
GetOwnPropertyMode mode = kCallJSGetterDontUseCachedName,
4654-
ExpectedReceiverMode expected_receiver_mode = kExpectingJSReceiver);
4656+
ExpectedReceiverMode expected_receiver_mode, TNode<Object> name,
4657+
Label* if_bailout,
4658+
GetOwnPropertyMode mode = kCallJSGetterDontUseCachedName);
46554659

46564660
TNode<IntPtrT> TryToIntptr(TNode<Object> key, Label* if_not_intptr,
46574661
TVariable<Int32T>* var_instance_type = nullptr);

‎src/ic/accessor-assembler.cc‎

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -845,9 +845,13 @@ void AccessorAssembler::HandleLoadICSmiHandlerLoadNamedCase(
845845
TVARIABLE(Object, var_value);
846846
LoadPropertyFromDictionary<PropertyDictionary>(
847847
properties, var_name_index.value(), &var_details, &var_value);
848+
849+
ExpectedReceiverMode expected_receiver_mode =
850+
p->IsLoadSuperIC() ? kExpectingAnyReceiver : kExpectingJSReceiver;
851+
848852
TNode<Object> value = CallGetterIfAccessor(
849853
var_value.value(), CAST(holder), var_details.value(), p->context(),
850-
p->receiver(), p->name(), miss);
854+
p->receiver(), expected_receiver_mode, p->name(), miss);
851855
exit_point->Return(value);
852856
}
853857
}
@@ -925,17 +929,18 @@ void AccessorAssembler::HandleLoadICSmiHandlerLoadNamedCase(
925929

926930
BIND(&global);
927931
{
928-
CSA_DCHECK(this, IsPropertyCell(CAST(holder)));
929932
// Ensure the property cell doesn't contain the hole.
930-
TNode<Object> value =
931-
LoadObjectField(CAST(holder), PropertyCell::kValueOffset);
933+
TNode<Object> value = LoadPropertyCellValue(CAST(holder));
934+
GotoIf(IsPropertyCellHole(value), miss);
932935
TNode<Uint32T> details = Unsigned(LoadAndUntagToWord32ObjectField(
933936
CAST(holder), PropertyCell::kPropertyDetailsRawOffset));
934-
GotoIf(IsPropertyCellHole(value), miss);
935937

936-
exit_point->Return(CallGetterIfAccessor(value, CAST(holder), details,
937-
p->context(), p->receiver(),
938-
p->name(), miss));
938+
ExpectedReceiverMode expected_receiver_mode =
939+
p->IsLoadSuperIC() ? kExpectingAnyReceiver : kExpectingJSReceiver;
940+
941+
exit_point->Return(CallGetterIfAccessor(
942+
value, std::nullopt, details, p->context(), p->receiver(),
943+
expected_receiver_mode, p->name(), miss));
939944
}
940945

941946
BIND(&interceptor);
@@ -1221,9 +1226,14 @@ void AccessorAssembler::HandleLoadICProtoHandler(
12211226
TVARIABLE(Object, var_value);
12221227
LoadPropertyFromDictionary<PropertyDictionary>(
12231228
properties, name_index, &var_details, &var_value);
1229+
1230+
ExpectedReceiverMode expected_receiver_mode =
1231+
p->IsLoadSuperIC() ? kExpectingAnyReceiver : kExpectingJSReceiver;
1232+
12241233
TNode<Object> value = CallGetterIfAccessor(
12251234
var_value.value(), CAST(var_holder->value()), var_details.value(),
1226-
p->context(), p->receiver(), p->name(), miss);
1235+
p->context(), p->receiver(), expected_receiver_mode, p->name(),
1236+
miss);
12271237
exit_point->Return(value);
12281238
}
12291239
},
@@ -2960,9 +2970,12 @@ void AccessorAssembler::GenericPropertyLoad(
29602970

29612971
BIND(&if_found_on_lookup_start_object);
29622972
{
2973+
ExpectedReceiverMode expected_receiver_mode =
2974+
p->IsLoadSuperIC() ? kExpectingAnyReceiver : kExpectingJSReceiver;
2975+
29632976
TNode<Object> value = CallGetterIfAccessor(
29642977
var_value.value(), CAST(lookup_start_object), var_details.value(),
2965-
p->context(), p->receiver(), p->name(), slow);
2978+
p->context(), p->receiver(), expected_receiver_mode, p->name(), slow);
29662979
Return(value);
29672980
}
29682981

‎src/ic/accessor-assembler.h‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -138,9 +138,7 @@ class V8_EXPORT_PRIVATE AccessorAssembler : public CodeStubAssembler {
138138
TNode<Object> name() const { return name_; }
139139
TNode<TaggedIndex> slot() const { return slot_; }
140140
TNode<HeapObject> vector() const { return vector_; }
141-
TNode<JSAny> lookup_start_object() const {
142-
return lookup_start_object_.value();
143-
}
141+
TNode<JSAny> lookup_start_object() const { return lookup_start_object_; }
144142
TNode<Smi> enum_index() const { return *enum_index_; }
145143
TNode<Object> cache_type() const { return *cache_type_; }
146144

@@ -152,6 +150,11 @@ class V8_EXPORT_PRIVATE AccessorAssembler : public CodeStubAssembler {
152150
return receiver_;
153151
}
154152

153+
// This is useful for figuring out whether we know anything about receiver
154+
// type. If |receiver| and |lookup_start_object| are different TNodes
155+
// then this ICParameters object belongs to LoadSuperIC.
156+
bool IsLoadSuperIC() const { return lookup_start_object_ != receiver_; }
157+
155158
bool IsEnumeratedKeyedLoad() const { return enum_index_ != std::nullopt; }
156159

157160
private:
@@ -160,7 +163,7 @@ class V8_EXPORT_PRIVATE AccessorAssembler : public CodeStubAssembler {
160163
TNode<Object> name_;
161164
TNode<TaggedIndex> slot_;
162165
TNode<HeapObject> vector_;
163-
std::optional<TNode<JSAny>> lookup_start_object_;
166+
TNode<JSAny> lookup_start_object_;
164167
std::optional<TNode<Smi>> enum_index_;
165168
std::optional<TNode<Object>> cache_type_;
166169
};
@@ -202,6 +205,11 @@ class V8_EXPORT_PRIVATE AccessorAssembler : public CodeStubAssembler {
202205
return receiver_;
203206
}
204207

208+
// This is useful for figuring out whether we know anything about receiver
209+
// type. If |receiver| and |lookup_start_object| are different TNodes
210+
// then this ICParameters object belongs to LoadSuperIC.
211+
bool IsLoadSuperIC() const { return lookup_start_object_ != receiver_; }
212+
205213
private:
206214
LazyNode<Context> context_;
207215
TNode<JSAny> receiver_;

0 commit comments

Comments
 (0)