nicer dynamic casts #5456

sethbrenith · 2018-07-13T00:39:37Z

I recently came across the following code:

// 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

This change is

Penguinwizzard · 2018-07-13T01:48:00Z

lib/Runtime/Types/RecyclableObject.h

@@ -466,4 +463,55 @@ namespace Js {
        int GetHeapEnumValidationCookie() { return m_heapEnumValidationCookie; }
 #endif
    };
+


Note: For reviewers, jump here to see VarIs/VarTo implementation

dilijev · 2018-07-13T02:11:18Z

I'll take a look soon

Penguinwizzard

Still looking, but wanted to get these comments out there for initial discussion.

Penguinwizzard · 2018-07-13T01:51:20Z

lib/Runtime/Types/RecyclableObject.h

+    template <typename T> T* VarTo(RecyclableObject* obj)
+    {
+        AssertOrFailFast(VarIs<T>(obj));
+        return reinterpret_cast<T*>(obj);


Shouldn't you be able to do this with a static_cast downcast? (I know that's not what the previous code did, but while you're changing it...) #Resolved

Yep, updated in a subsequent commit

In reply to: 202225543 [](ancestors = 202225543)

Penguinwizzard · 2018-07-13T02:29:32Z

lib/Runtime/Library/JavascriptArray.h

@@ -645,8 +645,7 @@ namespace Js
        template<typename T, typename P>
        static BOOL TryTemplatedGetItem(T *arr, P index, Var *element, ScriptContext *scriptContext, bool checkHasItem = true)
        {
-            AssertOrFailFastMsg(false, "FIXME");
-            return /*T::Is(arr) ? JavascriptArray::TemplatedGetItem(arr, index, element, scriptContext, checkHasItem) :*/
+            return LegacyVarIs<T>(arr) ? JavascriptArray::TemplatedGetItem(arr, index, element, scriptContext, checkHasItem) :


VarIs<T>(T* arg) should static_assert - that sort of call means we've already done the typecast that we're trying to check. This function should be reworked, imo, to something more like

template<typename TGT, typename T, typename P> static BOOL TryTemplatedGetItem(T *arr, P index, Var *element, ScriptContext *scriptContext, bool checkHasItem = true) { return VarIs<TGT>(arr) ? JavascriptArray::TemplatedGetItem(UnsafeVarTo<TGT>(arr), index, element, scriptContext, checkHasItem) : JavascriptOperators::GetItem(arr, index, element, scriptContext); }

#Resolved

Agreed, but I'm trying to keep this change a strict refactor without behavior modifications, to mitigate the risk of such a large change. Could this be deferred to a follow-up?

In reply to: 202229437 [](ancestors = 202229437)

Sure #Resolved

Penguinwizzard · 2018-07-13T02:33:53Z

lib/Runtime/Types/RecyclableObject.h

+
+    CompileAssertMsg(AtomTag_Object == 0, "Ensure GC objects do not need to be marked");
+
+    template <typename T> T* VarTo(RecyclableObject* obj)


It'd be nice to have this as

template <typename T, typename U> T* VarTo(U* obj) { static_assert(!is_same<T,U>::value, "Check should be unnecessary - did you prematurely cast?"); static_assert(is_base_of<U,T>::value, "VarTo should only downcast!"); return static_cast<T*>(obj); }

This way, we'd check for two more properties that could indicate logical issues in call sites. #Resolved

Do you have a recommendation for how to handle TemplatedForEachItemInRange? It has an input parameter of a templated type, and calls VarTo<JavascriptArray> on that input variable. For that call site, is_same<T,U> is sometimes true and sometimes false depending on the template parameter. Should we have a separate casting operation that allows is_same? VarToOrNotReallyIKnowWhatImDoing?

In reply to: 202229876 [](ancestors = 202229876)

Potentially, or the TemplatedForEachItemInRange could have different a different implementation if is_same<T,U>.

I looked back over TemplatedForEachItemInRange, and it's now apparent why it's an issue - we're using a pointer of the old type after it got mutated by the foreach'd function. We should likely re-write that function with something like this:

template<typename T, typename P> static BOOL TryTemplatedGetItem(Var arr, P index, Var *element, ScriptContext *scriptContext, bool checkHasItem = true) { return T::Is(arr) ? JavascriptArray::TemplatedGetItem(VarToUnsafe<T>(arr), index, element, scriptContext, checkHasItem) : JavascriptOperators::GetItem(arr, index, element, scriptContext); } template <bool hasSideEffect, typename T, typename Fn> static void TemplatedForEachItemInRange(T * arr, uint32 startIndex, uint32 limitIndex, Var missingItem, ScriptContext * scriptContext, Fn fn) { for (uint32 i = startIndex; i < limitIndex; i++) { Var element; Var curArr = arr; fn(i, TryTemplatedGetItem<T>(arr, i, &element, scriptContext) ? element : missingItem); if (hasSideEffect && MayChangeType<T>() && !T::Is(curArr)) { // The function has changed, go to another ForEachItemInRange. It is possible that the array might have changed to // an ES5Array, in such cases we don't need to call the JavascriptArray specific implementation. if (JavascriptArray::Is(curArr)) { VarTo<JavascriptArray>(curArr)->template ForEachItemInRange<true>(i + 1, limitIndex, missingItem, scriptContext, fn); return; } else { AssertOrFailFastMsg(VarIs<ES5Array>(curArr), "The array should have been converted to an ES5Array"); } } } } template <bool hasSideEffect, typename T, typename P, typename Fn> static void TemplatedForEachItemInRange(T * arr, P startIndex, P limitIndex, ScriptContext * scriptContext, Fn fn) { for (P i = startIndex; i < limitIndex; i++) { Var element; Var curArr = arr; if (TryTemplatedGetItem<T>(curArr, i, &element, scriptContext)) { fn(i, element); if (hasSideEffect && MayChangeType<T>() && !T::Is(curArr)) { // The function has changed, go to another ForEachItemInRange. It is possible that the array might have changed to // an ES5Array, in such cases we don't need to call the JavascriptArray specific implementation. if (JavascriptArray::Is(curArr)) { VarTo<JavascriptArray>(curArr)->template ForEachItemInRange<true>(i + 1, limitIndex, scriptContext, fn); return; } else { AssertOrFailFastMsg(VarIs<ES5Array>(curArr), "The array should have been converted to an ES5Array"); } } } } }

This patterning makes me more convinced that we should have those static asserts, since this is a (thankfully safe in this case) type confusion opportunity.

VarToOrNotReallyIKnowWhatImDoing -> UnsafeVarTo or ForceVarTo?

In theory, we shouldn't need the VartoOrNotreallyIKnowWhatImDoing; in this case, at least, we're doing it the wrong way anyway. In general, it'd be a separate implementation of VarTo without the static asserts.

Thanks @Penguinwizzard for the suggested fix; it works great! I used RecyclableObject* rather than Var to ensure we weren't slowing anything down due to tag checks. Does that seem reasonable?

In reply to: 203209057 [](ancestors = 203209057)

Yeah, that looks better, and I don't think there's a way for a tagged value to get into there regardless.

MSLaguana · 2018-07-13T15:38:00Z

Looks like you might have missed some T::Is calls:

17:52:11 �[1m/mnt/j/workspace/Microsoft_ChakraCore/master/static_ubuntu_linux_test_prtest/lib/Runtime/Library/IntlEngineInterfaceExtensionObject.cpp:363:46: �[0m�[0;1;31merror: �[0m�[1mno member named 'Is' in 'Js::JavascriptString'�[0m
17:52:11         AssertOrFailFast(propertyValue && T::Is(propertyValue));

#Resolved

MSLaguana · 2018-07-13T15:53:10Z

lib/Runtime/Library/ArgumentsObject.h

@@ -32,6 +30,11 @@ namespace Js
        virtual BOOL AdvanceWalkerToArgsFrame(JavascriptStackWalker *walker) = 0;
    };

+    template <> inline bool VarIs<ArgumentsObject>(RecyclableObject* obj)


Looks like you've changed the semantics a little here, from Var aValue to RecyclableObject* aValue; is that intentional? #Resolved

Ah, looks like the Var case is handled polymorphically by checking it isn't a tagged value, and then passing to the RecyclableObject case anyway. #Resolved

MSLaguana · 2018-07-13T15:56:30Z

lib/Runtime/Library/ArrayBuffer.h

        static int GetIsDetachedOffset() { return offsetof(ArrayBufferBase, isDetached); }

    protected:
        Field(bool) isDetached;
    };

+    template <> bool VarIs<ArrayBufferBase>(RecyclableObject* obj);


What's the reason for not providing the implementation of this one template method in the header? #Resolved

I tried in the header and it was a compiler error: the implementation tried to refer to something that wasn't defined yet. Same for any others that I moved to the cpp file.

In reply to: 202397007 [](ancestors = 202397007)

MSLaguana · 2018-07-13T16:00:14Z

lib/Runtime/Library/CustomExternalIterator.h

        static Var CreateNextFunction(JavascriptLibrary *library, JavascriptTypeId typeId);
        static Var EntryNext(RecyclableObject* function, CallInfo callInfo, ...);
    };

+    template <> bool VarIs<CustomExternalIterator>(RecyclableObject* obj);


Also curious why this template is only declared but not defined here. #Resolved

There are a couple of others too; If there's no particular reason, I'd prefer if the definitions were all in headers, both for simplicity and because I think it better enables inlining. #Resolved

MSLaguana · 2018-07-13T16:18:57Z

lib/Runtime/Types/DynamicObject.cpp

    {
-        RecyclableObject* obj = RecyclableObject::FromVar(aValue);
-        AssertMsg(obj->DbgIsDynamicObject(), "Ensure instance is actually a DynamicObject");
-        AssertOrFailFast(DynamicType::Is(obj->GetTypeId()));


Where has this failfast ended up, if anywhere? #Resolved

In VarTo implementation, it does AssertOrFailFast(VarIs<T>(obj))

In reply to: 202403174 [](ancestors = 202403174)

MSLaguana · 2018-07-13T16:25:37Z

lib/Runtime/Types/RecyclableObject.inl

-    {
-        AssertMsg(AtomTag_Object == 0, "Ensure GC objects do not need to be marked");
-        AssertMsg(Is(aValue), "Ensure instance is a RecyclableObject");
-        AssertOrFailFastMsg(!TaggedNumber::Is(aValue), "Tagged value being used as RecyclableObject");


Looks like this TaggedNumber::Is check is no longer present after the refactor, is that intentional? #Resolved

The code in VarIs(Var aValue) checks for any type of number tagging. If the tag bits are all zero, then we know it's recyclable. This was accomplishing the same thing via a slightly different call path.

In reply to: 202404837 [](ancestors = 202404837)

MSLaguana · 2018-07-13T16:29:27Z

lib/Runtime/Language/JavascriptOperators.h

@@ -238,12 +238,12 @@ namespace Js
        template <typename T>
        __forceinline static T* TryFromVar(_In_ RecyclableObject* value)
        {
-            return T::Is(value) ? T::UnsafeFromVar(value) : nullptr;
+            return LegacyVarIs<T>(value) ? UnsafeVarTo<T>(value) : nullptr;


Why does TryFromVar use the legacy behavior? Is that just because you don't know what the behavior is? #Resolved

I was trying very hard to not make any behavior changes. There are a bunch of TryFromVar<JavascriptArray> and a couple of TryFromVar<DynamicObject>, and I wanted them to call into the new methods rather than using VarIs (which now matches the assertions that used to be in FromVar).

In reply to: 202405808 [](ancestors = 202405808)

This has now been fixed and LegacyVarIs is gone. New non-template static methods have replaced TryFromVar<JavascriptArray> and TryFromVar<DynamicObject>. I used the linker to verify that I didn't miss anything, by building with the declarations of the LegacyVarIs specializations present but the definitions absent.

In reply to: 202433493 [](ancestors = 202433493,202405808)

MikeHolman · 2018-07-17T00:29:46Z

Change makes sense. It was pretty uncomfortable having FromVar not necessarily work the way you would expect.

Only comment is that I'd like to wait to merge this for a couple weeks when 1.10 has less churn. Otherwise I anticipate lots of merge conflicts on release/1.10->master.

Penguinwizzard · 2018-07-27T21:17:14Z

lib/Runtime/Language/JavascriptOperators.cpp

@@ -1049,7 +1049,8 @@ using namespace Js;

    JavascriptArray* JavascriptOperators::GetOwnPropertyNames(Var instance, ScriptContext *scriptContext)
    {
-        RecyclableObject *object = VarTo<RecyclableObject>(ToObject(instance, scriptContext));
+        RecyclableObject *object = ToObject(instance, scriptContext);
+        AssertOrFailFast(VarIs<RecyclableObject>(object));


nit: this doesn't really make sense - imo, it's better structured as

AssertOrFailFast(VarIs<RecyclableObject>(instance)); RecyclableObject *object = ToObject(instance, scriptContext);

In the current form, you're saying "Is this RecyclableObject a RecyclableObject", which feels a bit off. #Resolved

Well, ToObject can allocate Number objects for number primitives, so this wouldn't actually be valid to check before. As you mentioned elsewhere, we can add a separate method for these tautological checks.

In reply to: 205901862 [](ancestors = 205901862)

Penguinwizzard · 2018-07-27T21:20:24Z

lib/Runtime/Library/ArrayBuffer.cpp

@@ -282,6 +282,12 @@ namespace Js
        }
    }

+    ArrayBuffer * ArrayBuffer::GetAsArrayBuffer()
+    {
+        AssertOrFailFast(VarIs<ArrayBuffer>(this));


nit: This VarIs should not be possible to fail without already having significantly violated the type system. #Resolved

Yeah, I was trying to maintain the original semantics, but a bunch of these cases ended up kind of silly. I can remove them.

Rather than removing existing checks, let's change them to a version of the VarIs call that necessarily failfasts if the check doesn't pass. If we don't have information that this is affecting perf, then we might as well keep the checks in.

Penguinwizzard · 2018-07-27T21:25:06Z

lib/Runtime/Library/JavascriptLibrary.cpp

@@ -4197,7 +4197,8 @@ namespace Js
        }
        else
        {
-            objPrototype = Js::VarTo<Js::RecyclableObject>(prototype);
+            AssertOrFailFast(Js::VarIs<Js::RecyclableObject>(prototype));


There's a number of these instances in this commit, where you do VarIs in a manner that's implied to be tautological by the type system. Can you put the static_asserts to avoid this in VarIs as well, and use something else (e.g. FailFastIfNotVarIs) for these cases? #Resolved

Penguinwizzard · 2018-09-25T23:00:56Z

lib/Runtime/Types/RecyclableObject.h

+    // Validate that the object is actually the type that the type system thinks it is.
+    // This should only be used for extremely defensive assertions; if you find code
+    // relying on this behavior for correctness, then it's cause for concern.
+    template <typename T> bool VarIsCorrectType(T* obj)


This feels like there's a few situations in which a smart compiler would be able to make it always return true - e.g. with a static build and ltcg, I think it may be legal to optimize the "GetTypeId() == mytypeid" ::Is() checks for various object types in our type system to always return true when working on the same type as that passed in - since there's no avenue other than UB to change the typeid of valid objects of that pointer type to something else. Since we're already doing effectively that, it's not a regression with this change, but in a future change we'll most likely want to change this to being closer to VarIs in terms of signature, with the exception of making the last line AssertOrFailFast(VarIsImpl<T>(obj)).

Penguinwizzard

Reviewed 221 of 226 files at r1, 5 of 5 files at r2.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @sethbrenith and @dilijev)

lib/Backend/Lower.cpp, line 25033 at r2 (raw file):

    //  if (instance == nullptr) goto $Done;
    //
    //  if (!VarIs<RecyclableObject>(instance)) goto $Done

Kudos for updating the comments.

lib/Backend/SimpleJitProfilingHelpers.cpp, line 72 at r2 (raw file):

        JavascriptFunction *const calleeFunction =
            VarIs<JavascriptFunction>(callee) ? VarTo<JavascriptFunction>(callee) : nullptr;

Do we want to use TryVarTo for this pattern?

lib/Jsrt/JsrtSourceHolder.cpp, line 254 at r2 (raw file):

        END_LEAVE_SCRIPT(scriptContext);

        bool isExternalArray = Js::VarIs<Js::ArrayBuffer>(scriptVal),

Reviewers: This is one location where we were doing a different check than earlier code implied.

lib/Runtime/Language/AsmJsLink.cpp, line 18 at r2 (raw file):

        }

        if (!VarIs<ArrayBuffer>(bufferView))

Reviewers: This is one location where we were doing a different check than earlier code implied.

lib/Runtime/Language/AsmJsUtils.cpp, line 236 at r2 (raw file):

                        val = ConvertStringToInt64(*origArgs, scriptContext);
                    }
                    else if (DynamicObject::IsBaseDynamicObject(*origArgs))

Reviewers: This is one location where we were doing a different check than earlier code implied.

lib/Runtime/Language/JavascriptConversion.cpp, line 40 at r2 (raw file):

    bool JavascriptConversion::IsCallable(_In_ RecyclableObject* aValue)
    {
        Assert(VarIsCorrectType(aValue));

Belongs in the caller of this function, imo. Previous implementation of this function just did UnsafeFromVar from RecyclableObject* to RecyclableObject*, so this is new. Maybe put a //TODO: Move to callers on this assert?

lib/Runtime/Language/JavascriptOperators.cpp, line 1067 at r2 (raw file):

    {
        RecyclableObject *object = ToObject(instance, scriptContext);
        AssertOrFailFast(VarIsCorrectType(object));

Nit: maybe leave comment that this check should be moved to the inside of ToObject in the future?

lib/Runtime/Language/i386/AsmJsJitTemplate.cpp, line 554 at r2 (raw file):

        int arraySize = 0;
        BYTE* arrayPtr = nullptr;
        if (VarIsCorrectType<ArrayBuffer>(arrayBuffer))

nit: would like an else block here that nulls out the arrayBuffer local if this is false, since it's not of the type it claims to be, with a //TODO note for later fix-up.

lib/Runtime/Types/RecyclableObject.h, line 481 at r2 (raw file):

    {
        // ChakraFull can't include type_traits, but ChakraCore does include it for debug builds
#if DBG && !defined(NTBUILD)

Just a note that I believe that we can implement these particular type_traits properties in c++, and avoid the issue around platform-specific type_traits limitations. Not necessary for this pr though imo, we should be fine going forward with just our cc/xplat builds checking these properties, and we can add that functionality later if it is demonstrated that we'd improve our code with it.

sethbrenith

Opened #5737 to track

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @sethbrenith, @Penguinwizzard, and @dilijev)

lib/Backend/Lower.cpp, line 25033 at r2 (raw file):