Skip to content

Commit

Permalink
Fix Issue#3064/chakra-core#3423: Cache conflicts in super property ac…
Browse files Browse the repository at this point in the history 
…cess

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 chakra-core#3064, chakra-core#3423
  • Loading branch information
Suwei Chen committed Jul 25, 2017
1 parent 78baecc commit f2d0e2e
Show file tree
Hide file tree
Showing 12 changed files with 169 additions and 80 deletions.
6 changes: 5 additions & 1 deletion lib/Backend/GlobOpt.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3489,7 +3489,11 @@ GlobOpt::OptSrc(IR::Opnd *opnd, IR::Instr * *pInstr, Value **indirIndexValRef, I

if (opnd->IsSymOpnd() && opnd->AsSymOpnd()->IsPropertySymOpnd())
{
TryOptimizeInstrWithFixedDataProperty(&instr);
if (instr->m_opcode != Js::OpCode::LdSuperFld) // TODO: should profile LdSuperFld?
{
TryOptimizeInstrWithFixedDataProperty(&instr);
}

this->FinishOptPropOp(instr, opnd->AsPropertySymOpnd());
}

Expand Down
12 changes: 6 additions & 6 deletions lib/Backend/JnHelperMethodList.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,10 +215,10 @@ HELPERCALL(Op_Memset, Js::JavascriptOperators::OP_Memset, AttrCanThrow)
HELPERCALL(Op_Memcopy, Js::JavascriptOperators::OP_Memcopy, AttrCanThrow)

HELPERCALL(Op_PatchGetValue, ((Js::Var (*)(Js::FunctionBody *const, Js::InlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId))Js::JavascriptOperators::PatchGetValue<true, Js::InlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchGetValueWithThisPtr, ((Js::Var(*)(Js::FunctionBody *const, Js::InlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var))Js::JavascriptOperators::PatchGetValueWithThisPtr<true, Js::InlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchGetSuperValue, ((Js::Var(*)(Js::FunctionBody *const, Js::InlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, const bool))Js::JavascriptOperators::PatchGetSuperValue<true, Js::InlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchGetValueForTypeOf, ((Js::Var(*)(Js::FunctionBody *const, Js::InlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId))Js::JavascriptOperators::PatchGetValueForTypeOf<true, Js::InlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchGetValuePolymorphic, ((Js::Var (*)(Js::FunctionBody *const, Js::PolymorphicInlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId))Js::JavascriptOperators::PatchGetValue<true, Js::PolymorphicInlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchGetValuePolymorphicWithThisPtr, ((Js::Var(*)(Js::FunctionBody *const, Js::PolymorphicInlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var))Js::JavascriptOperators::PatchGetValueWithThisPtr<true, Js::PolymorphicInlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchGetSuperValuePolymorphic, ((Js::Var(*)(Js::FunctionBody *const, Js::PolymorphicInlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, const bool))Js::JavascriptOperators::PatchGetSuperValue<true, Js::PolymorphicInlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchGetValuePolymorphicForTypeOf, ((Js::Var(*)(Js::FunctionBody *const, Js::PolymorphicInlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId))Js::JavascriptOperators::PatchGetValueForTypeOf<true, Js::PolymorphicInlineCache>), AttrCanThrow)

HELPERCALL(Op_PatchGetRootValue, ((Js::Var (*)(Js::FunctionBody *const, Js::InlineCache *const, const Js::InlineCacheIndex, Js::DynamicObject*, Js::PropertyId))Js::JavascriptOperators::PatchGetRootValue<true, Js::InlineCache>), AttrCanThrow)
Expand All @@ -234,16 +234,16 @@ HELPERCALL(Op_PatchInitValue, ((void (*)(Js::FunctionBody *const, Js::InlineCach
HELPERCALL(Op_PatchInitValuePolymorphic, ((void (*)(Js::FunctionBody *const, Js::PolymorphicInlineCache *const, const Js::InlineCacheIndex, Js::RecyclableObject*, Js::PropertyId, Js::Var))Js::JavascriptOperators::PatchInitValue<true, Js::PolymorphicInlineCache>), AttrCanThrow)

HELPERCALL(Op_PatchPutValue, ((void (*)(Js::FunctionBody *const, Js::InlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, Js::PropertyOperationFlags))Js::JavascriptOperators::PatchPutValue<true, Js::InlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchPutValueWithThisPtr, ((void (*)(Js::FunctionBody *const, Js::InlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, Js::Var, Js::PropertyOperationFlags))Js::JavascriptOperators::PatchPutValueWithThisPtr<true, Js::InlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchPutSuperValue, ((void (*)(Js::FunctionBody *const, Js::InlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, Js::Var, Js::PropertyOperationFlags))Js::JavascriptOperators::PatchPutSuperValue<true, Js::InlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchPutValuePolymorphic, ((void (*)(Js::FunctionBody *const, Js::PolymorphicInlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, Js::PropertyOperationFlags))Js::JavascriptOperators::PatchPutValue<true, Js::PolymorphicInlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchPutValueWithThisPtrPolymorphic, ((void (*)(Js::FunctionBody *const, Js::PolymorphicInlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, Js::Var, Js::PropertyOperationFlags))Js::JavascriptOperators::PatchPutValueWithThisPtr<true, Js::PolymorphicInlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchPutSuperValuePolymorphic, ((void (*)(Js::FunctionBody *const, Js::PolymorphicInlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, Js::Var, Js::PropertyOperationFlags))Js::JavascriptOperators::PatchPutSuperValue<true, Js::PolymorphicInlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchPutRootValue, ((void (*)(Js::FunctionBody *const, Js::InlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, Js::PropertyOperationFlags))Js::JavascriptOperators::PatchPutRootValue<true, Js::InlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchPutRootValuePolymorphic, ((void (*)(Js::FunctionBody *const, Js::PolymorphicInlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, Js::PropertyOperationFlags))Js::JavascriptOperators::PatchPutRootValue<true, Js::PolymorphicInlineCache>), AttrCanThrow)

HELPERCALL(Op_PatchPutValueNoLocalFastPath, ((void (*)(Js::FunctionBody *const, Js::InlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, Js::PropertyOperationFlags))Js::JavascriptOperators::PatchPutValueNoLocalFastPath<true, Js::InlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchPutValueWithThisPtrNoLocalFastPath, ((void (*)(Js::FunctionBody *const, Js::InlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, Js::Var, Js::PropertyOperationFlags))Js::JavascriptOperators::PatchPutValueWithThisPtrNoLocalFastPath<true, Js::InlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchPutSuperValueNoLocalFastPath, ((void (*)(Js::FunctionBody *const, Js::InlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, Js::Var, Js::PropertyOperationFlags))Js::JavascriptOperators::PatchPutSuperValueNoLocalFastPath<true, Js::InlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchPutValueNoLocalFastPathPolymorphic, ((void (*)(Js::FunctionBody *const, Js::PolymorphicInlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, Js::PropertyOperationFlags))Js::JavascriptOperators::PatchPutValueNoLocalFastPath<true, Js::PolymorphicInlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchPutValueWithThisPtrNoLocalFastPathPolymorphic, ((void (*)(Js::FunctionBody *const, Js::PolymorphicInlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, Js::Var, Js::PropertyOperationFlags))Js::JavascriptOperators::PatchPutValueWithThisPtrNoLocalFastPath<true, Js::PolymorphicInlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchPutSuperValueNoLocalFastPathPolymorphic, ((void (*)(Js::FunctionBody *const, Js::PolymorphicInlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, Js::Var, Js::PropertyOperationFlags))Js::JavascriptOperators::PatchPutSuperValueNoLocalFastPath<true, Js::PolymorphicInlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchPutRootValueNoLocalFastPath, ((void (*)(Js::FunctionBody *const, Js::InlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, Js::PropertyOperationFlags))Js::JavascriptOperators::PatchPutRootValueNoLocalFastPath<true, Js::InlineCache>), AttrCanThrow)
HELPERCALL(Op_PatchPutRootValueNoLocalFastPathPolymorphic, ((void (*)(Js::FunctionBody *const, Js::PolymorphicInlineCache *const, const Js::InlineCacheIndex, Js::Var, Js::PropertyId, Js::Var, Js::PropertyOperationFlags))Js::JavascriptOperators::PatchPutRootValueNoLocalFastPath<true, Js::PolymorphicInlineCache>), AttrCanThrow)

Expand Down
8 changes: 4 additions & 4 deletions lib/Backend/Lower.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -389,8 +389,8 @@ Lowerer::LowerRange(IR::Instr *instrStart, IR::Instr *instrEnd, bool defaultDoFa
break;

case Js::OpCode::LdSuperFld:
instrPrev = GenerateCompleteLdFld<false>(instr, !noFieldFastPath, IR::HelperOp_PatchGetValueWithThisPtr, IR::HelperOp_PatchGetValuePolymorphicWithThisPtr,
IR::HelperOp_PatchGetValueWithThisPtr, IR::HelperOp_PatchGetValuePolymorphicWithThisPtr);
instrPrev = GenerateCompleteLdFld<false>(instr, !noFieldFastPath, IR::HelperOp_PatchGetSuperValue, IR::HelperOp_PatchGetSuperValuePolymorphic,
IR::HelperOp_PatchGetSuperValue, IR::HelperOp_PatchGetSuperValuePolymorphic);
break;

case Js::OpCode::LdRootFld:
Expand Down Expand Up @@ -508,8 +508,8 @@ Lowerer::LowerRange(IR::Instr *instrStart, IR::Instr *instrEnd, bool defaultDoFa
break;

case Js::OpCode::StSuperFld:
instrPrev = GenerateCompleteStFld(instr, !noFieldFastPath, IR::HelperOp_PatchPutValueWithThisPtrNoLocalFastPath, IR::HelperOp_PatchPutValueWithThisPtrNoLocalFastPathPolymorphic,
IR::HelperOp_PatchPutValueWithThisPtr, IR::HelperOp_PatchPutValueWithThisPtrPolymorphic, true, isStrictMode ? Js::PropertyOperation_StrictMode : Js::PropertyOperation_None);
instrPrev = GenerateCompleteStFld(instr, !noFieldFastPath, IR::HelperOp_PatchPutSuperValueNoLocalFastPath, IR::HelperOp_PatchPutSuperValueNoLocalFastPathPolymorphic,
IR::HelperOp_PatchPutSuperValue, IR::HelperOp_PatchPutSuperValuePolymorphic, true, isStrictMode ? Js::PropertyOperation_StrictMode : Js::PropertyOperation_None);
break;

case Js::OpCode::StRootFld:
Expand Down
12 changes: 8 additions & 4 deletions lib/Runtime/ByteCode/ByteCodeEmitter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7088,15 +7088,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);
byteCodeGenerator->Writer()->PatchablePropertyWithThisPtr(Js::OpCode::StSuperFld, rhsLocation, tmpReg, funcInfo->thisPointerRegister, cacheId);
uint cacheId = funcInfo->FindOrAddInlineCacheId(tmpReg, propertyId, false, true);
byteCodeGenerator->Writer()->PatchableSuperProperty(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 @@ -10888,16 +10889,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 @@ -10906,10 +10908,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);
byteCodeGenerator->Writer()->PatchablePropertyWithThisPtr(Js::OpCode::LdSuperFld, pnode->location, tmpReg, funcInfo->thisPointerRegister, cacheId, isConstructorCall);
uint cacheId = funcInfo->FindOrAddInlineCacheId(tmpReg, propertyId, false, false);
byteCodeGenerator->Writer()->PatchableSuperProperty(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
2 changes: 1 addition & 1 deletion lib/Runtime/ByteCode/ByteCodeWriter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2125,7 +2125,7 @@ namespace Js
return false;
}

void ByteCodeWriter::PatchablePropertyWithThisPtr(OpCode op, RegSlot value, RegSlot instance, RegSlot thisInstance, uint cacheId, bool isCtor, bool registerCacheIdForCall)
void ByteCodeWriter::PatchableSuperProperty(OpCode op, RegSlot value, RegSlot instance, RegSlot thisInstance, uint cacheId, bool isCtor, bool registerCacheIdForCall)
{
CheckOpen();
CheckOp(op, OpLayoutType::ElementC2);
Expand Down
2 changes: 1 addition & 1 deletion lib/Runtime/ByteCode/ByteCodeWriter.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ namespace Js
void ElementPIndexed(OpCode op, RegSlot value, uint cacheId, uint32 scopeIndex);
void PatchableRootProperty(OpCode op, RegSlot value, uint cacheId, bool isLoadMethod, bool isStore, bool registerCacheIdForCall = true);
void PatchableProperty(OpCode op, RegSlot value, RegSlot instance, uint cacheId, bool isCtor = false, bool registerCacheIdForCall = true);
void PatchablePropertyWithThisPtr(OpCode op, RegSlot value, RegSlot instance, RegSlot thisInstance, uint cacheId, bool isCtor = false, bool registerCacheIdForCall = true);
void PatchableSuperProperty(OpCode op, RegSlot value, RegSlot instance, RegSlot thisInstance, uint cacheId, bool isCtor = false, bool registerCacheIdForCall = true);
void ScopedProperty2(OpCode op, RegSlot Value, PropertyIdIndexType propertyIdIndex, RegSlot R2);
void W1(OpCode op, unsigned short C1);
void Reg1Int2(OpCode op, RegSlot R0, int C1, int C2);
Expand Down
10 changes: 6 additions & 4 deletions lib/Runtime/Language/InterpreterStackFrame.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4210,7 +4210,7 @@ namespace Js
}
SetReg(
playout->Value,
JavascriptOperators::PatchGetValueWithThisPtr<false>(
JavascriptOperators::PatchGetSuperValue<false>(
GetFunctionBody(),
GetInlineCache(playout->PropertyIdIndex),
playout->PropertyIdIndex,
Expand Down Expand Up @@ -4267,7 +4267,8 @@ namespace Js
GetInlineCache(playout->PropertyIdIndex),
playout->PropertyIdIndex,
GetFunctionBody(),
GetReg(playout->Value2)));
GetReg(playout->Value2),
true));

}

Expand Down Expand Up @@ -4565,7 +4566,7 @@ namespace Js
#endif
InlineCache *const inlineCache = GetInlineCache(playout->PropertyIdIndex);

JavascriptOperators::PatchPutValueWithThisPtrNoLocalFastPath<false, InlineCache>(
JavascriptOperators::PatchPutSuperValueNoLocalFastPath<false, InlineCache>(
GetFunctionBody(),
inlineCache,
playout->PropertyIdIndex,
Expand Down Expand Up @@ -4617,7 +4618,8 @@ namespace Js
m_functionBody->GetIsStrictMode() ?
(PropertyOperationFlags)(flags | PropertyOperation_StrictMode ) : flags,
GetJavascriptFunction(),
thisInstance);
thisInstance,
true);
}
#endif

Expand Down
Loading

0 comments on commit f2d0e2e

Please sign in to comment.