[1.7>master] [1.6>1.7] [MERGE chakra-core#3336 @suwc] Fix Issue#3064/chakra-core#3423: Cache conflicts in super property access

Merge pull request chakra-core#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 chakra-core#3064, Fixes chakra-core#3423

Author: Suwei Chen

lib/Backend/IRBuilder.cpp

@@ -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;
@@ -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;

lib/Runtime/ByteCode/ByteCodeEmitter.cpp

@@ -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);
  }
@@ -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);
  }
  }
@@ -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);
  }
  }

lib/Runtime/Language/JavascriptOperators.cpp

@@ -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))
@@ -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))
@@ -7282,7 +7282,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;
  }
@@ -7938,55 +7938,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);

lib/Runtime/Language/ProfilingHelpers.cpp

@@ -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,
@@ -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,

test/es6/classes_bugfixes.js

@@ -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 () {