Skip to content

Commit b9b758c

Browse files
committed
[MERGE #5456 @sethbrenith] nicer dynamic casts
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 -->
2 parents 831e6b2 + fe14f94 commit b9b758c

File tree

226 files changed

+3200
-4654
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

226 files changed

+3200
-4654
lines changed

lib/Backend/BackendApi.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -142,10 +142,10 @@ void CheckIsExecutable(Js::RecyclableObject * function, Js::JavascriptMethod ent
142142
{
143143
Js::ScriptContext * scriptContext = function->GetScriptContext();
144144
// it's easy to call the default entry point from RecyclableObject.
145-
AssertMsg((Js::JavascriptFunction::Is(function) && Js::JavascriptFunction::FromVar(function)->IsExternalFunction())
145+
AssertMsg((Js::VarIs<Js::JavascriptFunction>(function) && Js::VarTo<Js::JavascriptFunction>(function)->IsExternalFunction())
146146
|| Js::CrossSite::IsThunk(entrypoint)
147147
// External object with entrypoint
148-
|| (!Js::JavascriptFunction::Is(function)
148+
|| (!Js::VarIs<Js::JavascriptFunction>(function)
149149
&& function->IsExternal()
150150
&& Js::JavascriptConversion::IsCallable(function))
151151
|| !scriptContext->IsActuallyClosed()
@@ -160,7 +160,7 @@ void CheckIsExecutable(Js::RecyclableObject * function, Js::JavascriptMethod ent
160160
{
161161
return;
162162
}
163-
163+
164164
Js::TypeId typeId = Js::JavascriptOperators::GetTypeId(function);
165165
if (typeId == Js::TypeIds_HostDispatch)
166166
{

lib/Backend/BailOut.cpp

+11-11
Original file line numberDiff line numberDiff line change
@@ -576,10 +576,10 @@ BailOutRecord::RestoreValues(IR::BailOutKind bailOutKind, Js::JavascriptCallStac
576576
Assert(RegTypes[LinearScanMD::GetRegisterFromSaveIndex(offset)] != TyFloat64);
577577
value = registerSaveSpace[offset - 1];
578578
}
579-
Assert(Js::DynamicObject::Is(value));
579+
Assert(Js::DynamicObject::IsBaseDynamicObject(value));
580580
Assert(ThreadContext::IsOnStack(value));
581581

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

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

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

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

@@ -1106,7 +1106,7 @@ BailOutRecord::BailOutFromLoopBodyInlinedCommon(Js::JavascriptCallStackLayout *
11061106

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

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

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

11231123
Js::EntryPointInfo *entryPointInfo;
11241124
if(isInLoopBody)
@@ -1162,7 +1162,7 @@ BailOutRecord::BailOutInlinedHelper(Js::JavascriptCallStackLayout * layout, Bail
11621162

11631163
Js::ScriptFunction ** functionRef = (Js::ScriptFunction **)&(inlinedFrame->function);
11641164
AnalysisAssert(*functionRef);
1165-
Assert(Js::ScriptFunction::Is(inlinedFrame->function));
1165+
Assert(Js::VarIs<Js::ScriptFunction>(inlinedFrame->function));
11661166

11671167
if (*innerMostInlinee == nullptr)
11681168
{
@@ -1381,7 +1381,7 @@ BailOutRecord::BailOutHelper(Js::JavascriptCallStackLayout * layout, Js::ScriptF
13811381
// when resuming a generator and not needed when yielding from a generator, as is occurring
13821382
// here.
13831383
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");
1384-
Js::JavascriptGenerator* generator = Js::JavascriptGenerator::FromVar(args[0]);
1384+
Js::JavascriptGenerator* generator = Js::VarTo<Js::JavascriptGenerator>(args[0]);
13851385
newInstance = generator->GetFrame();
13861386

13871387
if (newInstance != nullptr)

lib/Backend/FixedFieldInfo.cpp

+4-4
Original file line numberDiff line numberDiff line change
@@ -14,16 +14,16 @@ FixedFieldInfo::PopulateFixedField(_In_opt_ Js::Type * type, _In_opt_ Js::Var va
1414
FixedFieldIDL * rawFF = fixed->GetRaw();
1515
rawFF->fieldValue = var;
1616
rawFF->nextHasSameFixedField = false;
17-
if (var != nullptr && Js::JavascriptFunction::Is(var))
17+
if (var != nullptr && Js::VarIs<Js::JavascriptFunction>(var))
1818
{
19-
Js::JavascriptFunction * funcObj = Js::JavascriptFunction::FromVar(var);
19+
Js::JavascriptFunction * funcObj = Js::VarTo<Js::JavascriptFunction>(var);
2020
rawFF->valueType = ValueType::FromObject(funcObj).GetRawData();
2121
rawFF->funcInfoAddr = (void*)funcObj->GetFunctionInfo();
2222
rawFF->isClassCtor = funcObj->GetFunctionInfo()->IsClassConstructor();
2323
rawFF->localFuncId = funcObj->GetFunctionInfo()->GetLocalFunctionId();
24-
if (Js::ScriptFunction::Is(var))
24+
if (Js::VarIs<Js::ScriptFunction>(var))
2525
{
26-
rawFF->environmentAddr = (void*)Js::ScriptFunction::FromVar(funcObj)->GetEnvironment();
26+
rawFF->environmentAddr = (void*)Js::VarTo<Js::ScriptFunction>(funcObj)->GetEnvironment();
2727
}
2828
}
2929
if (type != nullptr)

lib/Backend/GlobOptFields.cpp

+5-5
Original file line numberDiff line numberDiff line change
@@ -400,7 +400,7 @@ GlobOpt::ProcessFieldKills(IR::Instr *instr, BVSparse<JitArenaAllocator> *bv, bo
400400
case Js::OpCode::InlineeEnd:
401401
Assert(!instr->UsesAllFields());
402402

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

495495
Assert((*ppOpnd)->AsSymOpnd()->m_sym == sym || this->IsLoopPrePass());
496-
496+
497497
// We don't use the sym store to do copy prop on hoisted fields, but create a value
498498
// in case it can be copy prop out of the loop.
499499
return this->NewGenericValue(ValueType::Uninitialized, *ppOpnd);
@@ -1284,8 +1284,8 @@ GlobOpt::ProcessPropOpInTypeCheckSeq(IR::Instr* instr, IR::PropertySymOpnd *opnd
12841284
}
12851285
}
12861286
else if (valueInfo->GetJsTypeSet() &&
1287-
(opnd->IsMono() ?
1288-
valueInfo->GetJsTypeSet()->Contains(opnd->GetFirstEquivalentType()) :
1287+
(opnd->IsMono() ?
1288+
valueInfo->GetJsTypeSet()->Contains(opnd->GetFirstEquivalentType()) :
12891289
IsSubsetOf(opndTypeSet, valueInfo->GetJsTypeSet())
12901290
)
12911291
)
@@ -1473,7 +1473,7 @@ GlobOpt::OptNewScObject(IR::Instr** instrPtr, Value* srcVal)
14731473
instr->m_func->GetConstructorCache(static_cast<Js::ProfileId>(instr->AsProfiledInstr()->u.profileId)) : nullptr;
14741474

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

14791479
if (ctorCache != nullptr && !ctorCache->SkipNewScObject() && (isCtorInlined || ctorCache->IsTypeFinal()))

lib/Backend/Inline.cpp

+7-7
Original file line numberDiff line numberDiff line change
@@ -2549,7 +2549,7 @@ IR::Instr * Inline::InlineApplyWithArgumentsObject(IR::Instr * callInstr, IR::In
25492549
IR::Instr * argumentsObjArgOut = nullptr;
25502550
uint argOutCount = 0;
25512551
this->GetArgInstrsForCallAndApply(callInstr, &implicitThisArgOut, &explicitThisArgOut, &argumentsObjArgOut, argOutCount);
2552-
2552+
25532553
Assert(implicitThisArgOut);
25542554
Assert(explicitThisArgOut);
25552555
Assert(argumentsObjArgOut);
@@ -2725,7 +2725,7 @@ IR::Instr * Inline::InlineApplyWithoutArrayArgument(IR::Instr *callInstr, const
27252725

27262726
Assert(implicitThisArgOut);
27272727
Assert(explicitThisArgOut);
2728-
2728+
27292729
EmitFixedMethodOrFunctionObjectChecksForBuiltIns(callInstr, callInstr, applyInfo, false /*isPolymorphic*/, true /*isBuiltIn*/, false /*isCtor*/, true /*isInlined*/);
27302730

27312731
InsertInlineeBuiltInStartEndTags(callInstr, 2); // 2 args (implicit this + explicit this)
@@ -2898,7 +2898,7 @@ bool Inline::InlineApplyScriptTarget(IR::Instr *callInstr, const FunctionJITTime
28982898
{
28992899
return false;
29002900
}
2901-
2901+
29022902
const FunctionJITTimeInfo * inlineeData = nullptr;
29032903
Js::InlineCacheIndex inlineCacheIndex = 0;
29042904
IR::Instr * callbackDefInstr = nullptr;
@@ -2955,7 +2955,7 @@ bool Inline::InlineApplyScriptTarget(IR::Instr *callInstr, const FunctionJITTime
29552955
});
29562956

29572957
// If the arguments object was passed in as the first argument to apply,
2958-
// 'arguments' access continues to exist even after apply target inlining
2958+
// 'arguments' access continues to exist even after apply target inlining
29592959
if (!HasArgumentsAccess(explicitThisArgOut))
29602960
{
29612961
callInstr->m_func->SetApplyTargetInliningRemovedArgumentsAccess();
@@ -3023,7 +3023,7 @@ Inline::InlineCallApplyTarget_Shared(IR::Instr *callInstr, bool originalCallTarg
30233023
if (isCallback)
30243024
{
30253025
char16 debugStringBuffer[MAX_FUNCTION_BODY_DEBUG_STRING_SIZE];
3026-
INLINE_CALLBACKS_TRACE(_u("INLINING CALLBACK : Inlining callback for call/apply target : \t%s (%s)\n"), inlineeData->GetBody()->GetDisplayName(),
3026+
INLINE_CALLBACKS_TRACE(_u("INLINING CALLBACK : Inlining callback for call/apply target : \t%s (%s)\n"), inlineeData->GetBody()->GetDisplayName(),
30273027
inlineeData->GetDebugNumberSet(debugStringBuffer));
30283028
}
30293029
#endif
@@ -4067,7 +4067,7 @@ void Inline::InlineDOMGetterSetterFunction(IR::Instr *ldFldInstr, const Function
40674067
// type-specific optimizations. Otherwise, this optimization to reduce calls into the host will also
40684068
// result in relatively more expensive calls in the runtime.
40694069
tmpDst->SetValueType(ldFldInstr->GetDst()->GetValueType());
4070-
4070+
40714071
IR::Opnd * callInstrDst = ldFldInstr->UnlinkDst();
40724072
ldFldInstr->SetDst(tmpDst);
40734073

@@ -4491,7 +4491,7 @@ Inline::InsertJsFunctionCheck(IR::Instr * callInstr, IR::Instr *insertBeforeInst
44914491
void
44924492
Inline::InsertFunctionInfoCheck(IR::RegOpnd * funcOpnd, IR::Instr *insertBeforeInstr, IR::Instr* bailoutInstr, const FunctionJITTimeInfo *funcInfo)
44934493
{
4494-
// if (JavascriptFunction::FromVar(r1)->functionInfo != funcInfo) goto noInlineLabel
4494+
// if (VarTo<JavascriptFunction>(r1)->functionInfo != funcInfo) goto noInlineLabel
44954495
// BrNeq_I4 noInlineLabel, r1->functionInfo, funcInfo
44964496
IR::IndirOpnd* opndFuncInfo = IR::IndirOpnd::New(funcOpnd, Js::JavascriptFunction::GetOffsetOfFunctionInfo(), TyMachPtr, insertBeforeInstr->m_func);
44974497
IR::AddrOpnd* inlinedFuncInfo = IR::AddrOpnd::New(funcInfo->GetFunctionInfoAddr(), IR::AddrOpndKindDynamicFunctionInfo, insertBeforeInstr->m_func);

lib/Backend/InlineeFrameInfo.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -211,9 +211,9 @@ void InlineeFrameRecord::Restore(Js::FunctionBody* functionBody, InlinedFrameLay
211211
BAILOUT_VERBOSE_TRACE(functionBody, _u("Restore function object: "));
212212
// No deepCopy needed for just the function
213213
Js::Var varFunction = this->Restore(this->functionOffset, /*isFloat64*/ false, /*isInt32*/ false, layout, functionBody, boxValues);
214-
Assert(Js::ScriptFunction::Is(varFunction));
214+
Assert(Js::VarIs<Js::ScriptFunction>(varFunction));
215215

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

219219
inlinedFrame->function = function;
@@ -230,7 +230,7 @@ void InlineeFrameRecord::Restore(Js::FunctionBody* functionBody, InlinedFrameLay
230230
#if DBG
231231
if (boxValues && !Js::TaggedNumber::Is(var))
232232
{
233-
Js::RecyclableObject *const recyclableObject = Js::RecyclableObject::FromVar(var);
233+
Js::RecyclableObject *const recyclableObject = Js::VarTo<Js::RecyclableObject>(var);
234234
Assert(!ThreadContext::IsOnStack(recyclableObject));
235235
}
236236
#endif

0 commit comments

Comments
 (0)