Changeset 291756 in webkit

Timestamp:

Mar 23, 2022, 11:47:18 AM (5 years ago)

Author:

ysuzuki@apple.com

Message:

[JSC][MSVC] custom getter creation needs to include classInfo since MSVC ICF is not "safe" variant
​https://bugs.webkit.org/show_bug.cgi?id=238030

Reviewed by Alexey Shvayka.

MSVC performs very aggressive ICF (identical code folding) and it even merges the identical two functions
into one even though a pointer to this function is used. This means MSVC's ICF is not "safe"[1], and custom
function weakmap is broken on MSVC since it is assuming function pointers are different for different functions.
Unfortunately, it seems that there is no attribute / annotation to prevent this behavior, so we need to workaround it.
Since JSCustomGetterFunction does separate thing based on attached DOMAttribute, we need to include const ClassInfo*
into a key of JSCustomGetterFunction weakmap to ensure that two identical functions with different const ClassInfo*
do not get the same JSCustomGetterFunction.

[1]: ​https://static.googleusercontent.com/media/research.google.com/en//pubs/archive/36912.pdf

• runtime/JSCustomGetterFunction.h:
• runtime/JSCustomSetterFunction.h:
• runtime/JSGlobalObject.h:
• runtime/JSGlobalObjectInlines.h:

(JSC::JSGlobalObject::WeakCustomGetterOrSetterHash<T>::hash):

• runtime/JSObject.cpp:

(JSC::WeakCustomGetterOrSetterHashTranslator::hash):
(JSC::WeakCustomGetterOrSetterHashTranslator::equal):
(JSC::createCustomGetterFunction):
(JSC::createCustomSetterFunction):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• runtime/JSCustomGetterFunction.h (modified) (1 diff)
• runtime/JSCustomSetterFunction.h (modified) (1 diff)
• runtime/JSGlobalObject.h (modified) (1 diff)
• runtime/JSGlobalObjectInlines.h (modified) (3 diffs)
• runtime/JSObject.cpp (modified) (4 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2022-03-23  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC][MSVC] custom getter creation needs to include classInfo since MSVC ICF is not "safe" variant
+        https://bugs.webkit.org/show_bug.cgi?id=238030
+
+        Reviewed by Alexey Shvayka.
+
+        MSVC performs very aggressive ICF (identical code folding) and it even merges the identical two functions
+        into one even though a pointer to this function is used. This means MSVC's ICF is not "safe"[1], and custom
+        function weakmap is broken on MSVC since it is assuming function pointers are different for different functions.
+        Unfortunately, it seems that there is no attribute / annotation to prevent this behavior, so we need to workaround it.
+        Since JSCustomGetterFunction does separate thing based on attached DOMAttribute, we need to include const ClassInfo*
+        into a key of JSCustomGetterFunction weakmap to ensure that two identical functions with different const ClassInfo*
+        do not get the same JSCustomGetterFunction.
+
+        [1]: https://static.googleusercontent.com/media/research.google.com/en//pubs/archive/36912.pdf
+
+        * runtime/JSCustomGetterFunction.h:
+        * runtime/JSCustomSetterFunction.h:
+        * runtime/JSGlobalObject.h:
+        * runtime/JSGlobalObjectInlines.h:
+        (JSC::JSGlobalObject::WeakCustomGetterOrSetterHash<T>::hash):
+        * runtime/JSObject.cpp:
+        (JSC::WeakCustomGetterOrSetterHashTranslator::hash):
+        (JSC::WeakCustomGetterOrSetterHashTranslator::equal):
+        (JSC::createCustomGetterFunction):
+        (JSC::createCustomSetterFunction):
+
 2022-03-23  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/JavaScriptCore/runtime/JSCustomGetterFunction.h

@@ -60 +60 @@
     CustomFunctionPointer customFunctionPointer() const { return m_getter; };
     std::optional<DOMAttributeAnnotation> domAttribute() const { return m_domAttribute; };
+    const ClassInfo* slotBaseClassInfoIfExists() const
+    {
+        if (m_domAttribute)
+            return m_domAttribute->classInfo;
+        return nullptr;
+    }
 
 private:

trunk/Source/JavaScriptCore/runtime/JSCustomSetterFunction.h

@@ -59 +59 @@
     CustomFunctionPointer setter() const { return m_setter; };
     CustomFunctionPointer customFunctionPointer() const { return m_setter; };
+    const ClassInfo* slotBaseClassInfoIfExists() const { return nullptr; }
 
 private:

trunk/Source/JavaScriptCore/runtime/JSGlobalObject.h

@@ -600 +600 @@
         static unsigned hash(const Weak<T>&);
         static bool equal(const Weak<T>&, const Weak<T>&);
-        static unsigned hash(const PropertyName&, typename T::CustomFunctionPointer);
+        static unsigned hash(const PropertyName&, typename T::CustomFunctionPointer, const ClassInfo*);
 
         static constexpr bool safeToCompareToEmptyOrDeleted = false;

trunk/Source/JavaScriptCore/runtime/JSGlobalObjectInlines.h

@@ -35 +35 @@
 #include "LinkTimeConstant.h"
 #include "ObjectPrototype.h"
+#include <wtf/Hasher.h>
 
 namespace JSC {
@@ -154 +155 @@
     if (!value)
         return 0;
-    return hash(value->propertyName(), value->customFunctionPointer());
+    return hash(value->propertyName(), value->customFunctionPointer(), value->slotBaseClassInfoIfExists());
 }
 
@@ -166 +167 @@
 
 template<typename T>
-inline unsigned JSGlobalObject::WeakCustomGetterOrSetterHash<T>::hash(const PropertyName& propertyName, typename T::CustomFunctionPointer functionPointer)
+inline unsigned JSGlobalObject::WeakCustomGetterOrSetterHash<T>::hash(const PropertyName& propertyName, typename T::CustomFunctionPointer functionPointer, const ClassInfo* classInfo)
 {
-    unsigned hash = DefaultHash<typename T::CustomFunctionPointer>::hash(functionPointer);
     if (!propertyName.isNull())
-        hash = WTF::pairIntHash(hash, propertyName.uid()->existingSymbolAwareHash());
-    return hash;
+        return WTF::computeHash(functionPointer, propertyName.uid()->existingSymbolAwareHash(), classInfo);
+    return WTF::computeHash(functionPointer, classInfo);
 }
 

trunk/Source/JavaScriptCore/runtime/JSObject.cpp

@@ -3622 +3622 @@
     using BaseHash = JSGlobalObject::WeakCustomGetterOrSetterHash<T>;
 
-    using Key = std::pair<PropertyName, typename T::CustomFunctionPointer>;
+    using Key = std::tuple<PropertyName, typename T::CustomFunctionPointer, const ClassInfo*>;
 
     static unsigned hash(const Key& key)
     {
-        return BaseHash::hash(std::get<0>(key), std::get<1>(key));
+        return BaseHash::hash(std::get<0>(key), std::get<1>(key), std::get<2>(key));
     }
 
@@ -3633 +3633 @@
         if (!a)
             return false;
-        return a->propertyName() == std::get<0>(b) && a->customFunctionPointer() == std::get<1>(b);
+        return a->propertyName() == std::get<0>(b) && a->customFunctionPointer() == std::get<1>(b) && a->slotBaseClassInfoIfExists() == std::get<2>(b);
     }
 };
@@ -3644 +3644 @@
     // We use DeferGC here (1) not to invoke GC when executing WeakGCSet::ensureValue and (2) to avoid looking up HashSet twice.
     DeferGC deferGC(vm);
-    return globalObject->customGetterFunctionSet().ensureValue<Translator>(std::make_pair(propertyName, getValueFunc), [&] {
+    const ClassInfo* classInfo = nullptr;
+    if (domAttribute)
+        classInfo = domAttribute->classInfo;
+    return globalObject->customGetterFunctionSet().ensureValue<Translator>(std::tuple { propertyName, getValueFunc, classInfo }, [&] {
         return JSCustomGetterFunction::create(vm, globalObject, propertyName, getValueFunc, domAttribute);
     });
@@ -3656 +3659 @@
     // We use DeferGC here (1) not to invoke GC when executing WeakGCSet::ensureValue and (2) to avoid looking up HashSet twice.
     DeferGC deferGC(vm);
-    return globalObject->customSetterFunctionSet().ensureValue<Translator>(std::make_pair(propertyName, putValueFunc), [&] {
+    return globalObject->customSetterFunctionSet().ensureValue<Translator>(std::tuple { propertyName, putValueFunc, nullptr }, [&] {
         return JSCustomSetterFunction::create(vm, globalObject, propertyName, putValueFunc);
     });