Skip to content

Commit

Permalink
[MERGE #5456 @sethbrenith] nicer dynamic casts
Browse files Browse the repository at this point in the history
Merge pull request #5456 from sethbrenith:user/sethb/is

I recently came across the following code:

```C++
// JsParseSerialized only accepts ArrayBuffer (incl. ExternalArrayBuffer)
if (!Js::ExternalArrayBuffer::Is(bufferVal))
{
 return JsErrorInvalidArgument;
}
```

I thought the comment was out of date, and was about to update it, when I realized that `ExternalArrayBuffer::Is` actually invokes `ArrayBuffer::Is`, because static methods are inherited and `ExternalArrayBuffer` doesn't provide an `Is` method. I don't want to live in a world where `ExternalArrayBuffer::Is(bufferVal)` can return something other than whether `bufferVal` is an `ExternalArrayBuffer`, so this change is my proposed solution. It introduces a new template method `VarIs` (in RecyclableObject.h) and uses it consistently for all type-checks and conversions of `RecyclableObject` subclasses. Benefits are:

* Avoid the confusing case above (it would be a linker error)
* Less boilerplate code (this is a net removal of about 1500 lines)
* Every type gets by default an optimization for when the compiler knows the input is `RecyclableObject*` (previously only a few types implemented this)

Most of the change is purely mechanical, and anybody who's willing to review it is a hero. However, a few cases are interesting:

* `DynamicObject`, `JavascriptArray`, and `JavascriptGeneratorFunction` had asymmetrical behavior between `Is` and `(Unsafe)?FromVar`. I have attempted to avoid any behavior changes in this review by updating callers to `Is` to use a new uniquely-named method, and making `VarIs` respect the behavior from `(Unsafe)?FromVar`.
* A few calls have been updated to refer to the thing they were actually calling, not some subclass:
    * `JavascriptObject` -> `DynamicObject`
    * `ExternalArrayBuffer` -> `ArrayBuffer`
    * `JavascriptArrayBuffer` -> `ArrayBuffer`
    * `RuntimeFunction` -> `JavascriptFunction`

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/microsoft/chakracore/5456)
<!-- Reviewable:end -->
  • Loading branch information
sethbrenith committed Oct 2, 2018
2 parents 831e6b2 + fe14f94 commit b9b758c
Show file tree
Hide file tree
Showing 226 changed files with 3,200 additions and 4,654 deletions.
6 changes: 3 additions & 3 deletions lib/Backend/BackendApi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -142,10 +142,10 @@ void CheckIsExecutable(Js::RecyclableObject * function, Js::JavascriptMethod ent
{
Js::ScriptContext * scriptContext = function->GetScriptContext();
// it's easy to call the default entry point from RecyclableObject.
AssertMsg((Js::JavascriptFunction::Is(function) && Js::JavascriptFunction::FromVar(function)->IsExternalFunction())
AssertMsg((Js::VarIs<Js::JavascriptFunction>(function) && Js::VarTo<Js::JavascriptFunction>(function)->IsExternalFunction())
|| Js::CrossSite::IsThunk(entrypoint)
// External object with entrypoint
|| (!Js::JavascriptFunction::Is(function)
|| (!Js::VarIs<Js::JavascriptFunction>(function)
&& function->IsExternal()
&& Js::JavascriptConversion::IsCallable(function))
|| !scriptContext->IsActuallyClosed()
Expand All @@ -160,7 +160,7 @@ void CheckIsExecutable(Js::RecyclableObject * function, Js::JavascriptMethod ent
{
return;
}

Js::TypeId typeId = Js::JavascriptOperators::GetTypeId(function);
if (typeId == Js::TypeIds_HostDispatch)
{
Expand Down
22 changes: 11 additions & 11 deletions lib/Backend/BailOut.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -576,10 +576,10 @@ BailOutRecord::RestoreValues(IR::BailOutKind bailOutKind, Js::JavascriptCallStac
Assert(RegTypes[LinearScanMD::GetRegisterFromSaveIndex(offset)] != TyFloat64);
value = registerSaveSpace[offset - 1];
}
Assert(Js::DynamicObject::Is(value));
Assert(Js::DynamicObject::IsBaseDynamicObject(value));
Assert(ThreadContext::IsOnStack(value));

Js::DynamicObject * obj = Js::DynamicObject::FromVar(value);
Js::DynamicObject * obj = Js::VarTo<Js::DynamicObject>(value);
uint propertyCount = obj->GetPropertyCount();
for (uint j = record.initFldCount; j < propertyCount; j++)
{
Expand Down Expand Up @@ -656,7 +656,7 @@ BailOutRecord::RestoreValues(IR::BailOutKind bailOutKind, Js::JavascriptCallStac
if (branchValueRegSlot != Js::Constants::NoRegister)
{
// Used when a t1 = CmCC is optimize to BrCC, and the branch bails out. T1 needs to be restored
Assert(branchValue && Js::JavascriptBoolean::Is(branchValue));
Assert(branchValue && Js::VarIs<Js::JavascriptBoolean>(branchValue));
Assert(branchValueRegSlot < newInstance->GetJavascriptFunction()->GetFunctionBody()->GetLocalsCount());
newInstance->m_localSlots[branchValueRegSlot] = branchValue;
}
Expand Down Expand Up @@ -1004,7 +1004,7 @@ BailOutRecord::BailOutCommonNoCodeGen(Js::JavascriptCallStackLayout * layout, Ba
BailOutReturnValue * bailOutReturnValue, void * argoutRestoreAddress)
{
Assert(bailOutRecord->parent == nullptr);
Assert(Js::ScriptFunction::Is(layout->functionObject));
Assert(Js::VarIs<Js::ScriptFunction>(layout->functionObject));
Js::ScriptFunction ** functionRef = (Js::ScriptFunction **)&layout->functionObject;
Js::ArgumentReader args(&layout->callInfo, layout->args);
Js::Var result = BailOutHelper(layout, functionRef, args, false, bailOutRecord, bailOutOffset, returnAddress, bailOutKind, registerSaves, bailOutReturnValue, layout->GetArgumentsObjectLocation(), branchValue, argoutRestoreAddress);
Expand All @@ -1031,7 +1031,7 @@ uint32 bailOutOffset, void * returnAddress, IR::BailOutKind bailOutKind, Js::Imp
sizeof(registerSaves));

Js::Var result = BailOutCommonNoCodeGen(layout, bailOutRecord, bailOutOffset, returnAddress, bailOutKind, branchValue, registerSaves, bailOutReturnValue, argoutRestoreAddress);
ScheduleFunctionCodeGen(Js::ScriptFunction::FromVar(layout->functionObject), nullptr, bailOutRecord, bailOutKind, bailOutOffset, savedImplicitCallFlags, returnAddress);
ScheduleFunctionCodeGen(Js::VarTo<Js::ScriptFunction>(layout->functionObject), nullptr, bailOutRecord, bailOutKind, bailOutOffset, savedImplicitCallFlags, returnAddress);
return result;
}

Expand Down Expand Up @@ -1060,7 +1060,7 @@ BailOutRecord::BailOutInlinedCommon(Js::JavascriptCallStackLayout * layout, Bail
}
Js::Var result = BailOutCommonNoCodeGen(layout, currentBailOutRecord, currentBailOutRecord->bailOutOffset, returnAddress, bailOutKind, branchValue,
registerSaves, &bailOutReturnValue);
ScheduleFunctionCodeGen(Js::ScriptFunction::FromVar(layout->functionObject), innerMostInlinee, currentBailOutRecord, bailOutKind, bailOutOffset, savedImplicitCallFlags, returnAddress);
ScheduleFunctionCodeGen(Js::VarTo<Js::ScriptFunction>(layout->functionObject), innerMostInlinee, currentBailOutRecord, bailOutKind, bailOutOffset, savedImplicitCallFlags, returnAddress);
return result;
}

Expand All @@ -1076,7 +1076,7 @@ BailOutRecord::BailOutFromLoopBodyCommon(Js::JavascriptCallStackLayout * layout,
js_memcpy_s(registerSaves, sizeof(registerSaves), (Js::Var *)layout->functionObject->GetScriptContext()->GetThreadContext()->GetBailOutRegisterSaveSpace(),
sizeof(registerSaves));
uint32 result = BailOutFromLoopBodyHelper(layout, bailOutRecord, bailOutOffset, bailOutKind, branchValue, registerSaves);
ScheduleLoopBodyCodeGen(Js::ScriptFunction::FromVar(layout->functionObject), nullptr, bailOutRecord, bailOutKind);
ScheduleLoopBodyCodeGen(Js::VarTo<Js::ScriptFunction>(layout->functionObject), nullptr, bailOutRecord, bailOutKind);
return result;
}

Expand Down Expand Up @@ -1106,7 +1106,7 @@ BailOutRecord::BailOutFromLoopBodyInlinedCommon(Js::JavascriptCallStackLayout *

uint32 result = BailOutFromLoopBodyHelper(layout, currentBailOutRecord, currentBailOutRecord->bailOutOffset,
bailOutKind, nullptr, registerSaves, &bailOutReturnValue);
ScheduleLoopBodyCodeGen(Js::ScriptFunction::FromVar(layout->functionObject), innerMostInlinee, currentBailOutRecord, bailOutKind);
ScheduleLoopBodyCodeGen(Js::VarTo<Js::ScriptFunction>(layout->functionObject), innerMostInlinee, currentBailOutRecord, bailOutKind);
return result;
}

Expand All @@ -1118,7 +1118,7 @@ BailOutRecord::BailOutInlinedHelper(Js::JavascriptCallStackLayout * layout, Bail
BailOutReturnValue * lastBailOutReturnValue = nullptr;
*innerMostInlinee = nullptr;

Js::FunctionBody* functionBody = Js::ScriptFunction::FromVar(layout->functionObject)->GetFunctionBody();
Js::FunctionBody* functionBody = Js::VarTo<Js::ScriptFunction>(layout->functionObject)->GetFunctionBody();

Js::EntryPointInfo *entryPointInfo;
if(isInLoopBody)
Expand Down Expand Up @@ -1162,7 +1162,7 @@ BailOutRecord::BailOutInlinedHelper(Js::JavascriptCallStackLayout * layout, Bail

Js::ScriptFunction ** functionRef = (Js::ScriptFunction **)&(inlinedFrame->function);
AnalysisAssert(*functionRef);
Assert(Js::ScriptFunction::Is(inlinedFrame->function));
Assert(Js::VarIs<Js::ScriptFunction>(inlinedFrame->function));

if (*innerMostInlinee == nullptr)
{
Expand Down Expand Up @@ -1381,7 +1381,7 @@ BailOutRecord::BailOutHelper(Js::JavascriptCallStackLayout * layout, Js::ScriptF
// when resuming a generator and not needed when yielding from a generator, as is occurring
// here.
AssertMsg(args.Info.Count == 2, "Generator ScriptFunctions should only be invoked by generator APIs with the pair of arguments they pass in -- the generator object and a ResumeYieldData pointer");
Js::JavascriptGenerator* generator = Js::JavascriptGenerator::FromVar(args[0]);
Js::JavascriptGenerator* generator = Js::VarTo<Js::JavascriptGenerator>(args[0]);
newInstance = generator->GetFrame();

if (newInstance != nullptr)
Expand Down
8 changes: 4 additions & 4 deletions lib/Backend/FixedFieldInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,16 @@ FixedFieldInfo::PopulateFixedField(_In_opt_ Js::Type * type, _In_opt_ Js::Var va
FixedFieldIDL * rawFF = fixed->GetRaw();
rawFF->fieldValue = var;
rawFF->nextHasSameFixedField = false;
if (var != nullptr && Js::JavascriptFunction::Is(var))
if (var != nullptr && Js::VarIs<Js::JavascriptFunction>(var))
{
Js::JavascriptFunction * funcObj = Js::JavascriptFunction::FromVar(var);
Js::JavascriptFunction * funcObj = Js::VarTo<Js::JavascriptFunction>(var);
rawFF->valueType = ValueType::FromObject(funcObj).GetRawData();
rawFF->funcInfoAddr = (void*)funcObj->GetFunctionInfo();
rawFF->isClassCtor = funcObj->GetFunctionInfo()->IsClassConstructor();
rawFF->localFuncId = funcObj->GetFunctionInfo()->GetLocalFunctionId();
if (Js::ScriptFunction::Is(var))
if (Js::VarIs<Js::ScriptFunction>(var))
{
rawFF->environmentAddr = (void*)Js::ScriptFunction::FromVar(funcObj)->GetEnvironment();
rawFF->environmentAddr = (void*)Js::VarTo<Js::ScriptFunction>(funcObj)->GetEnvironment();
}
}
if (type != nullptr)
Expand Down
10 changes: 5 additions & 5 deletions lib/Backend/GlobOptFields.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ GlobOpt::ProcessFieldKills(IR::Instr *instr, BVSparse<JitArenaAllocator> *bv, bo
case Js::OpCode::InlineeEnd:
Assert(!instr->UsesAllFields());

// Kill all live 'arguments' and 'caller' fields, as 'inlineeFunction.arguments' and 'inlineeFunction.caller'
// Kill all live 'arguments' and 'caller' fields, as 'inlineeFunction.arguments' and 'inlineeFunction.caller'
// cannot be copy-propped across different instances of the same inlined function.
KillLiveFields(argumentsEquivBv, bv);
KillLiveFields(callerEquivBv, bv);
Expand Down Expand Up @@ -493,7 +493,7 @@ GlobOpt::CreateFieldSrcValue(PropertySym * sym, PropertySym * originalSym, IR::O
}

Assert((*ppOpnd)->AsSymOpnd()->m_sym == sym || this->IsLoopPrePass());

// We don't use the sym store to do copy prop on hoisted fields, but create a value
// in case it can be copy prop out of the loop.
return this->NewGenericValue(ValueType::Uninitialized, *ppOpnd);
Expand Down Expand Up @@ -1284,8 +1284,8 @@ GlobOpt::ProcessPropOpInTypeCheckSeq(IR::Instr* instr, IR::PropertySymOpnd *opnd
}
}
else if (valueInfo->GetJsTypeSet() &&
(opnd->IsMono() ?
valueInfo->GetJsTypeSet()->Contains(opnd->GetFirstEquivalentType()) :
(opnd->IsMono() ?
valueInfo->GetJsTypeSet()->Contains(opnd->GetFirstEquivalentType()) :
IsSubsetOf(opndTypeSet, valueInfo->GetJsTypeSet())
)
)
Expand Down Expand Up @@ -1473,7 +1473,7 @@ GlobOpt::OptNewScObject(IR::Instr** instrPtr, Value* srcVal)
instr->m_func->GetConstructorCache(static_cast<Js::ProfileId>(instr->AsProfiledInstr()->u.profileId)) : nullptr;

// TODO: OOP JIT, enable assert
//Assert(ctorCache == nullptr || srcVal->GetValueInfo()->IsVarConstant() && Js::JavascriptFunction::Is(srcVal->GetValueInfo()->AsVarConstant()->VarValue()));
//Assert(ctorCache == nullptr || srcVal->GetValueInfo()->IsVarConstant() && Js::VarIs<Js::JavascriptFunction>(srcVal->GetValueInfo()->AsVarConstant()->VarValue()));
Assert(ctorCache == nullptr || !ctorCache->IsTypeFinal() || ctorCache->CtorHasNoExplicitReturnValue());

if (ctorCache != nullptr && !ctorCache->SkipNewScObject() && (isCtorInlined || ctorCache->IsTypeFinal()))
Expand Down
14 changes: 7 additions & 7 deletions lib/Backend/Inline.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2549,7 +2549,7 @@ IR::Instr * Inline::InlineApplyWithArgumentsObject(IR::Instr * callInstr, IR::In
IR::Instr * argumentsObjArgOut = nullptr;
uint argOutCount = 0;
this->GetArgInstrsForCallAndApply(callInstr, &implicitThisArgOut, &explicitThisArgOut, &argumentsObjArgOut, argOutCount);

Assert(implicitThisArgOut);
Assert(explicitThisArgOut);
Assert(argumentsObjArgOut);
Expand Down Expand Up @@ -2725,7 +2725,7 @@ IR::Instr * Inline::InlineApplyWithoutArrayArgument(IR::Instr *callInstr, const

Assert(implicitThisArgOut);
Assert(explicitThisArgOut);

EmitFixedMethodOrFunctionObjectChecksForBuiltIns(callInstr, callInstr, applyInfo, false /*isPolymorphic*/, true /*isBuiltIn*/, false /*isCtor*/, true /*isInlined*/);

InsertInlineeBuiltInStartEndTags(callInstr, 2); // 2 args (implicit this + explicit this)
Expand Down Expand Up @@ -2898,7 +2898,7 @@ bool Inline::InlineApplyScriptTarget(IR::Instr *callInstr, const FunctionJITTime
{
return false;
}

const FunctionJITTimeInfo * inlineeData = nullptr;
Js::InlineCacheIndex inlineCacheIndex = 0;
IR::Instr * callbackDefInstr = nullptr;
Expand Down Expand Up @@ -2955,7 +2955,7 @@ bool Inline::InlineApplyScriptTarget(IR::Instr *callInstr, const FunctionJITTime
});

// If the arguments object was passed in as the first argument to apply,
// 'arguments' access continues to exist even after apply target inlining
// 'arguments' access continues to exist even after apply target inlining
if (!HasArgumentsAccess(explicitThisArgOut))
{
callInstr->m_func->SetApplyTargetInliningRemovedArgumentsAccess();
Expand Down Expand Up @@ -3023,7 +3023,7 @@ Inline::InlineCallApplyTarget_Shared(IR::Instr *callInstr, bool originalCallTarg
if (isCallback)
{
char16 debugStringBuffer[MAX_FUNCTION_BODY_DEBUG_STRING_SIZE];
INLINE_CALLBACKS_TRACE(_u("INLINING CALLBACK : Inlining callback for call/apply target : \t%s (%s)\n"), inlineeData->GetBody()->GetDisplayName(),
INLINE_CALLBACKS_TRACE(_u("INLINING CALLBACK : Inlining callback for call/apply target : \t%s (%s)\n"), inlineeData->GetBody()->GetDisplayName(),
inlineeData->GetDebugNumberSet(debugStringBuffer));
}
#endif
Expand Down Expand Up @@ -4067,7 +4067,7 @@ void Inline::InlineDOMGetterSetterFunction(IR::Instr *ldFldInstr, const Function
// type-specific optimizations. Otherwise, this optimization to reduce calls into the host will also
// result in relatively more expensive calls in the runtime.
tmpDst->SetValueType(ldFldInstr->GetDst()->GetValueType());

IR::Opnd * callInstrDst = ldFldInstr->UnlinkDst();
ldFldInstr->SetDst(tmpDst);

Expand Down Expand Up @@ -4491,7 +4491,7 @@ Inline::InsertJsFunctionCheck(IR::Instr * callInstr, IR::Instr *insertBeforeInst
void
Inline::InsertFunctionInfoCheck(IR::RegOpnd * funcOpnd, IR::Instr *insertBeforeInstr, IR::Instr* bailoutInstr, const FunctionJITTimeInfo *funcInfo)
{
// if (JavascriptFunction::FromVar(r1)->functionInfo != funcInfo) goto noInlineLabel
// if (VarTo<JavascriptFunction>(r1)->functionInfo != funcInfo) goto noInlineLabel
// BrNeq_I4 noInlineLabel, r1->functionInfo, funcInfo
IR::IndirOpnd* opndFuncInfo = IR::IndirOpnd::New(funcOpnd, Js::JavascriptFunction::GetOffsetOfFunctionInfo(), TyMachPtr, insertBeforeInstr->m_func);
IR::AddrOpnd* inlinedFuncInfo = IR::AddrOpnd::New(funcInfo->GetFunctionInfoAddr(), IR::AddrOpndKindDynamicFunctionInfo, insertBeforeInstr->m_func);
Expand Down
6 changes: 3 additions & 3 deletions lib/Backend/InlineeFrameInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -211,9 +211,9 @@ void InlineeFrameRecord::Restore(Js::FunctionBody* functionBody, InlinedFrameLay
BAILOUT_VERBOSE_TRACE(functionBody, _u("Restore function object: "));
// No deepCopy needed for just the function
Js::Var varFunction = this->Restore(this->functionOffset, /*isFloat64*/ false, /*isInt32*/ false, layout, functionBody, boxValues);
Assert(Js::ScriptFunction::Is(varFunction));
Assert(Js::VarIs<Js::ScriptFunction>(varFunction));

Js::ScriptFunction* function = Js::ScriptFunction::FromVar(varFunction);
Js::ScriptFunction* function = Js::VarTo<Js::ScriptFunction>(varFunction);
BAILOUT_VERBOSE_TRACE(functionBody, _u("Inlinee: %s [%d.%d] \n"), function->GetFunctionBody()->GetDisplayName(), function->GetFunctionBody()->GetSourceContextId(), function->GetFunctionBody()->GetLocalFunctionId());

inlinedFrame->function = function;
Expand All @@ -230,7 +230,7 @@ void InlineeFrameRecord::Restore(Js::FunctionBody* functionBody, InlinedFrameLay
#if DBG
if (boxValues && !Js::TaggedNumber::Is(var))
{
Js::RecyclableObject *const recyclableObject = Js::RecyclableObject::FromVar(var);
Js::RecyclableObject *const recyclableObject = Js::VarTo<Js::RecyclableObject>(var);
Assert(!ThreadContext::IsOnStack(recyclableObject));
}
#endif
Expand Down
Loading

0 comments on commit b9b758c

Please sign in to comment.