Bug 74341 – Start de-virtualizing destructors starting with JSCell and moving down

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

In order to preserve the correctness across patches, we'll start de-virtualizing destructors starting with JSCell and moving down the hierarchy using the destroy functions.  Any class in the next level down that we haven't de-virtualized yet will receive a static destroy function that calls the virtual destructor for that subtree of the hierarchy.  Any class in the current level or above that has a non-empty destructor (i.e. it has members that require calls to their destructors or any other pre-existing logic) will receive a destroy method that statically calls that class's destructor, whether that destructor is implicitly or explicitly defined.  Any class with what C++ calls a non-trivial destructor will receive a COMPILE_ASSERT stating as much.  In a future patch sequence, we will move toward making all classes in the JSCell hierarchy have non-trivial destructors.  We must keep a virtual vtableAnchor() function rooted in JSCell to prevent objects from losing the vptrs, since we're not quite ready for them to disappear yet.

We will use

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wnon-virtual-dtor"
// class declaration...
#pragma clang diagnostic pop

to avoid errors due to the fact that classes will still have a virtual method but no virtual destructor.

[Mark Hahnenberg]

Created attachment 118864 [details]
Start de-virtualizing destructors

No changelog (see bug 74331)

[Geoffrey Garen]

Comment on attachment 118864 [details]
Start de-virtualizing destructors

View in context: https://bugs.webkit.org/attachment.cgi?id=118864&action=review

Remember to remove the #pragmas in your final patch.

> Source/JavaScriptCore/heap/MarkedBlock.cpp:72
> +    if (cell->classInfo() != &JSFinalObject::s_info)

Use isJSFinalObject().

> Source/JavaScriptCore/runtime/Executable.cpp:47
> +    jsCast<ExecutableBase*>(cell)->~ExecutableBase();

Should be ExecutableBase::~ExecutableBase().

> Source/JavaScriptCore/runtime/JSArray.cpp:280
> +    ASSERT(classInfo()->isSubClassOf(&JSArray::s_info));

I think this would read better as ASSERT(jsCast<JSArray>(this)).

> Source/JavaScriptCore/runtime/JSByteArray.cpp:47
> +    ASSERT(classInfo()->isSubClassOf(&JSByteArray::s_info));

Ditto.

> Source/JavaScriptCore/runtime/JSObject.cpp:90
> +    jsCast<JSObject*>(cell)->~JSObject();

JSObject::~JSObject().

> Source/JavaScriptCore/runtime/JSString.cpp:55
> +    ASSERT(thisObject->classInfo() == &JSString::s_info);

I think this ASSERT would be more readable as ASSERT(jsCast<JSString>(cell)).