Skip to content

Commit

Permalink
[MERGE #3336 @suwc] Fix Issue#3064/#3423: Cache conflicts in super pr…
Browse files Browse the repository at this point in the history 
…operty access

Merge pull request #3336 from suwc:build/suwc/Issue3064

Accesses to super properties are cached on 'this' object (vs. the
"super" object), causing conflict between e.g. super.x and this.x.
Similar conflicts cause Issue#3423 for GetProperty cases.
Fix by adding 'isSuper' flag to use appropriate object for caching.

Fixes #3064, Fixes #3423
  • Loading branch information
Suwei Chen committed Aug 11, 2017
2 parents 63034a0 + d2a1cae commit 85ff17d
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 58 deletions.
4 changes: 2 additions & 2 deletions lib/Backend/IRBuilder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4651,7 +4651,7 @@ IRBuilder::BuildElementC2(Js::OpCode newOpcode, uint32 offset, Js::RegSlot insta
value2Opnd = this->BuildDstOpnd(value2Slot);
regOpnd = this->BuildDstOpnd(regSlot);

instr = IR::Instr::New(newOpcode, regOpnd, fieldSymOpnd, value2Opnd, m_func);
instr = IR::ProfiledInstr::New(newOpcode, regOpnd, fieldSymOpnd, value2Opnd, m_func);
this->AddInstr(instr, offset);
}
break;
Expand All @@ -4672,7 +4672,7 @@ IRBuilder::BuildElementC2(Js::OpCode newOpcode, uint32 offset, Js::RegSlot insta
regOpnd = this->BuildSrcOpnd(regSlot);
value2Opnd = this->BuildSrcOpnd(value2Slot);

instr = IR::Instr::New(newOpcode, fieldSymOpnd, regOpnd, value2Opnd, m_func);
instr = IR::ProfiledInstr::New(newOpcode, fieldSymOpnd, regOpnd, value2Opnd, m_func);

this->AddInstr(instr, offset);
break;
Expand Down
8 changes: 6 additions & 2 deletions lib/Runtime/ByteCode/ByteCodeEmitter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7121,15 +7121,16 @@ void EmitAssignment(
// PutValue(x, "y", rhs)
Js::PropertyId propertyId = lhs->sxBin.pnode2->sxPid.PropertyIdFromNameNode();

uint cacheId = funcInfo->FindOrAddInlineCacheId(lhs->sxBin.pnode1->location, propertyId, false, true);
EmitSuperMethodBegin(lhs, byteCodeGenerator, funcInfo);
if (lhs->sxBin.pnode1->nop == knopSuper)
{
Js::RegSlot tmpReg = byteCodeGenerator->EmitLdObjProto(Js::OpCode::LdHomeObjProto, funcInfo->superRegister, funcInfo);
uint cacheId = funcInfo->FindOrAddInlineCacheId(tmpReg, propertyId, false, true);
byteCodeGenerator->Writer()->PatchablePropertyWithThisPtr(Js::OpCode::StSuperFld, rhsLocation, tmpReg, funcInfo->thisPointerRegister, cacheId);
}
else
{
uint cacheId = funcInfo->FindOrAddInlineCacheId(lhs->sxBin.pnode1->location, propertyId, false, true);
byteCodeGenerator->Writer()->PatchableProperty(
ByteCodeGenerator::GetStFldOpCode(funcInfo, false, false, false, false), rhsLocation, lhs->sxBin.pnode1->location, cacheId);
}
Expand Down Expand Up @@ -10911,16 +10912,17 @@ void Emit(ParseNode *pnode, ByteCodeGenerator *byteCodeGenerator, FuncInfo *func
Js::RegSlot protoLocation = callObjLocation;
EmitSuperMethodBegin(pnode, byteCodeGenerator, funcInfo);

uint cacheId = funcInfo->FindOrAddInlineCacheId(callObjLocation, propertyId, false, false);
if (pnode->IsCallApplyTargetLoad())
{
if (pnode->sxBin.pnode1->nop == knopSuper)
{
Js::RegSlot tmpReg = byteCodeGenerator->EmitLdObjProto(Js::OpCode::LdHomeObjProto, funcInfo->superRegister, funcInfo);
uint cacheId = funcInfo->FindOrAddInlineCacheId(tmpReg, propertyId, false, false);
byteCodeGenerator->Writer()->PatchableProperty(Js::OpCode::LdFldForCallApplyTarget, pnode->location, tmpReg, cacheId);
}
else
{
uint cacheId = funcInfo->FindOrAddInlineCacheId(callObjLocation, propertyId, false, false);
byteCodeGenerator->Writer()->PatchableProperty(Js::OpCode::LdFldForCallApplyTarget, pnode->location, protoLocation, cacheId);
}
}
Expand All @@ -10929,10 +10931,12 @@ void Emit(ParseNode *pnode, ByteCodeGenerator *byteCodeGenerator, FuncInfo *func
if (pnode->sxBin.pnode1->nop == knopSuper)
{
Js::RegSlot tmpReg = byteCodeGenerator->EmitLdObjProto(Js::OpCode::LdHomeObjProto, funcInfo->superRegister, funcInfo);
uint cacheId = funcInfo->FindOrAddInlineCacheId(tmpReg, propertyId, false, false);
byteCodeGenerator->Writer()->PatchablePropertyWithThisPtr(Js::OpCode::LdSuperFld, pnode->location, tmpReg, funcInfo->thisPointerRegister, cacheId, isConstructorCall);
}
else
{
uint cacheId = funcInfo->FindOrAddInlineCacheId(callObjLocation, propertyId, false, false);
byteCodeGenerator->Writer()->PatchableProperty(Js::OpCode::LdFld, pnode->location, callObjLocation, cacheId, isConstructorCall);
}
}
Expand Down
56 changes: 4 additions & 52 deletions lib/Runtime/Language/JavascriptOperators.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1739,7 +1739,7 @@ namespace Js
// Also we might want to throw here instead of checking it again in the caller
if (value && !requestContext->IsUndeclBlockVar(*value) && !WithScopeObject::Is(object))
{
CacheOperators::CachePropertyRead(instance, object, isRoot, propertyId, false, info, requestContext);
CacheOperators::CachePropertyRead(propertyObject, object, isRoot, propertyId, false, info, requestContext);
}
#ifdef TELEMETRY_JSO
if (TELEMETRY_PROPERTY_OPCODE_FILTER(propertyId))
Expand Down Expand Up @@ -1779,7 +1779,7 @@ namespace Js
}

PropertyValueInfo::Set(info, requestContext->GetLibrary()->GetMissingPropertyHolder(), 0);
CacheOperators::CachePropertyRead(instance, requestContext->GetLibrary()->GetMissingPropertyHolder(), isRoot, propertyId, true, info, requestContext);
CacheOperators::CachePropertyRead(propertyObject, requestContext->GetLibrary()->GetMissingPropertyHolder(), isRoot, propertyId, true, info, requestContext);
}
#if defined(TELEMETRY_JSO) || defined(TELEMETRY_AddToCache) // enabled for `TELEMETRY_AddToCache`, because this is the property-not-found codepath where the normal TELEMETRY_AddToCache code wouldn't be executed.
if (TELEMETRY_PROPERTY_OPCODE_FILTER(propertyId))
Expand Down Expand Up @@ -7274,7 +7274,7 @@ namespace Js
PropertyValueInfo::SetCacheInfo(&info, functionBody, inlineCache, inlineCacheIndex, !IsFromFullJit);
Var value;
if (CacheOperators::TryGetProperty<true, true, true, true, true, true, !TInlineCache::IsPolymorphic, TInlineCache::IsPolymorphic, false>(
thisInstance, false, object, propertyId, &value, scriptContext, nullptr, &info))
instance, false, object, propertyId, &value, scriptContext, nullptr, &info))
{
return value;
}
Expand Down Expand Up @@ -7930,55 +7930,7 @@ namespace Js
template <bool IsFromFullJit, class TInlineCache>
inline void JavascriptOperators::PatchPutValueNoLocalFastPath(FunctionBody *const functionBody, TInlineCache *const inlineCache, const InlineCacheIndex inlineCacheIndex, Var instance, PropertyId propertyId, Var newValue, PropertyOperationFlags flags)
{
ScriptContext *const scriptContext = functionBody->GetScriptContext();

if (TaggedNumber::Is(instance))
{
JavascriptOperators::SetPropertyOnTaggedNumber(instance,
nullptr,
propertyId,
newValue,
scriptContext,
flags);
return;
}
#if ENABLE_COPYONACCESS_ARRAY
JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(instance);
#endif
RecyclableObject *object = RecyclableObject::FromVar(instance);

PropertyValueInfo info;
PropertyValueInfo::SetCacheInfo(&info, functionBody, inlineCache, inlineCacheIndex, !IsFromFullJit);
if (CacheOperators::TrySetProperty<!TInlineCache::IsPolymorphic, true, true, true, true, !TInlineCache::IsPolymorphic, TInlineCache::IsPolymorphic, false>(
object, false, propertyId, newValue, scriptContext, flags, nullptr, &info))
{
return;
}

#if DBG_DUMP
if (PHASE_VERBOSE_TRACE1(Js::InlineCachePhase))
{
CacheOperators::TraceCache(inlineCache, _u("PatchPutValueNoLocalFastPath"), propertyId, scriptContext, object);
}
#endif

ImplicitCallFlags prevImplicitCallFlags = ImplicitCall_None;
ImplicitCallFlags currImplicitCallFlags = ImplicitCall_None;
bool hasThisOnlyStatements = functionBody->GetHasOnlyThisStmts();
if (hasThisOnlyStatements)
{
prevImplicitCallFlags = CacheAndClearImplicitBit(scriptContext);
}
if (!JavascriptOperators::OP_SetProperty(instance, propertyId, newValue, scriptContext, &info, flags))
{
// Add implicit call flags, to bail out if field copy prop may propagate the wrong value.
scriptContext->GetThreadContext()->AddImplicitCallFlags(ImplicitCall_NoOpSet);
}
if (hasThisOnlyStatements)
{
currImplicitCallFlags = CheckAndUpdateFunctionBodyWithImplicitFlag(functionBody);
RestoreImplicitFlag(scriptContext, prevImplicitCallFlags, currImplicitCallFlags);
}
PatchPutValueWithThisPtrNoLocalFastPath<IsFromFullJit, TInlineCache>(functionBody, inlineCache, inlineCacheIndex, instance, propertyId, newValue, instance, flags);
}
template void JavascriptOperators::PatchPutValueNoLocalFastPath<false, InlineCache>(FunctionBody *const functionBody, InlineCache *const inlineCache, const InlineCacheIndex inlineCacheIndex, Var instance, PropertyId propertyId, Var newValue, PropertyOperationFlags flags);
template void JavascriptOperators::PatchPutValueNoLocalFastPath<true, InlineCache>(FunctionBody *const functionBody, InlineCache *const inlineCache, const InlineCacheIndex inlineCacheIndex, Var instance, PropertyId propertyId, Var newValue, PropertyOperationFlags flags);
Expand Down
4 changes: 2 additions & 2 deletions lib/Runtime/Language/ProfilingHelpers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -857,7 +857,7 @@ namespace Js
PropertyValueInfo propertyValueInfo;
PropertyValueInfo::SetCacheInfo(&propertyValueInfo, functionBody, inlineCache, inlineCacheIndex, true);
if (!CacheOperators::TryGetProperty<true, true, true, !Root && !Method, true, !Root, true, false, true>(
thisObject,
object,
Root,
object,
propertyId,
Expand Down Expand Up @@ -1102,7 +1102,7 @@ namespace Js
PropertyValueInfo propertyValueInfo;
PropertyValueInfo::SetCacheInfo(&propertyValueInfo, functionBody, inlineCache, inlineCacheIndex, true);
if(!CacheOperators::TrySetProperty<true, true, true, true, !Root, true, false, true>(
thisObject,
object,
Root,
propertyId,
value,
Expand Down
49 changes: 49 additions & 0 deletions test/es6/classes_bugfixes.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,55 @@ var tests = [
assert.areEqual('abc', result, "result == 'abc'");
}
},
{
name: "Issue3064: Caching conflict of super property access",
body: function () {
function Base() { }
Base.prototype = {
x: 15,
f() { return this.x; },
};

function Derived() {}
Derived.prototype = {
__proto__: Base.prototype,
x: 27,
f() {
var a = super.x;
var b = eval("super.x");
return this.x;
}
};

assert.areEqual(15, new Base().f());
assert.areEqual(27, new Derived().f());
}
},
{
name: "Issue3423: Repeated calls to class setter triggers assertion",
body: function () {
var result = "";
class B {
set x(v) { result += "Bset;"; this._x = v; }
get x() { result += "Bget;"; return this._x; }
}

class A extends B {
set x(v) { result += "Aset;"; super.x = v + 100; }
get x() { result += "Aget;"; return super.x; }
}

var a = new A();
a.x = 100;
assert.areEqual(200, a.x);
assert.areEqual("Aset;Bset;Aget;Bget;", result);

var a1 = new A();
a1.x = -100;
assert.areEqual(0, a1.x);
assert.areEqual("Aset;Bset;Aget;Bget;Aset;Bset;Aget;Bget;", result);
}
},
{
name: "Issue3217: Reflect.construct permanently corrupts the invoked constructor",
body: function () {
Expand Down

0 comments on commit 85ff17d

Please sign in to comment.