Bug 172098 – [JSC][DFG][DOMJIT] Extend CheckDOM to CheckSubClass

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

CheckDOM is not DOMJIT specific thing. It performs subclass-of with the given ClassInfo*.
We can expand it to accept arbitrary ClassInfo*.
And ClassInfo* itself should have a way to teach DFG the way to perform faster class info check instead of having checkDOM function in each DOMJIT annotation.

[Yusuke Suzuki]

Created attachment 310110 [details]
Patch

[Build Bot]

Attachment 310110 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSTypedArrays.cpp:37:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Total errors found: 1 in 302 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 310125 [details]
Patch

[Build Bot]

Attachment 310125 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSTypedArrays.cpp:37:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Total errors found: 1 in 303 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 310125 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7923
> +        m_jit.loadPtr(CCallHelpers::Address(specifiedGPR, ClassInfo::offsetOfParentClass()), specifiedGPR);

This looks backwards. Don't you want to check if otherGPR is a subclass of specifiedGPR, not the other way around?

[Yusuke Suzuki]

Created attachment 310345 [details]
Patch

[Build Bot]

Attachment 310345 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSTypedArrays.cpp:37:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Total errors found: 1 in 309 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 310345 [details]
Patch

Attachment 310345 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3757346

New failing tests:
http/tests/loading/resourceLoadStatistics/non-prevalent-resource-without-user-interaction.html

[Build Bot]

Created attachment 310353 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

Created attachment 310354 [details]
Patch

[Build Bot]

Attachment 310354 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSTypedArrays.cpp:37:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Total errors found: 1 in 309 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 310354 [details]
Patch

Attachment 310354 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3759002

New failing tests:
http/tests/loading/resourceLoadStatistics/non-prevalent-resource-without-user-interaction.html

[Build Bot]

Created attachment 310358 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 310354 [details]
Patch

Attachment 310354 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3760688

New failing tests:
http/tests/appcache/404-resource-with-slow-main-resource.php

[Build Bot]

Created attachment 310370 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Saam Barati]

Comment on attachment 310354 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:18
> +        One problem is that it enlarges the size of ClassInfo.
> +        But this is the best place to put this function pointer.

I don't think this will matter much in practice, since ClassInfo size is bounded per process.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7931
> +    RefPtr<DOMJIT::Patchpoint> patchpoint = classInfo->checkSubClassPatchpoint();

Style nit: It seems weird we're still calling this a DOMJIT::Patchpoint even though the function is called checkSubClassPatchpoint.

> Source/JavaScriptCore/runtime/ClassInfo.cpp:33
> +void ClassInfo::dump(PrintStream& out) const

nice!

> Source/JavaScriptCore/runtime/ClassInfo.h:214
> +    JS_EXPORT_PRIVATE void dump(PrintStream&) const;

Is this the WTF dataLog compatible way of printing?

[Yusuke Suzuki]

Comment on attachment 310354 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:18
>> +        But this is the best place to put this function pointer.
> 
> I don't think this will matter much in practice, since ClassInfo size is bounded per process.

OK.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7931
>> +    RefPtr<DOMJIT::Patchpoint> patchpoint = classInfo->checkSubClassPatchpoint();
> 
> Style nit: It seems weird we're still calling this a DOMJIT::Patchpoint even though the function is called checkSubClassPatchpoint.

Yeah, we can move this out from DOMJIT directory. Actually, I think it is useful.

>> Source/JavaScriptCore/runtime/ClassInfo.h:214
>> +    JS_EXPORT_PRIVATE void dump(PrintStream&) const;
> 
> Is this the WTF dataLog compatible way of printing?

Yes. dataLog will call stderr printstream's print(...). And it will invoke `value.dump(out)`.

[Yusuke Suzuki]

Comment on attachment 310354 [details]
Patch

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

>>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7931
>>> +    RefPtr<DOMJIT::Patchpoint> patchpoint = classInfo->checkSubClassPatchpoint();
>> 
>> Style nit: It seems weird we're still calling this a DOMJIT::Patchpoint even though the function is called checkSubClassPatchpoint.
> 
> Yeah, we can move this out from DOMJIT directory. Actually, I think it is useful.

To move this out of domjit directory, we need to move many DOMJIT files (DOMJITPatchpointParam etc.).
So, I would like to do this in a separate patch. https://bugs.webkit.org/show_bug.cgi?id=172260

[Yusuke Suzuki]

Committed r217031: <http://trac.webkit.org/changeset/217031>

[Yusuke Suzuki]

Committed r217032: <http://trac.webkit.org/changeset/217032>

[Yusuke Suzuki]

Committed r217037: <http://trac.webkit.org/changeset/217037>

[Alexey Proskuryakov]

Windows build remains broken. How did this get landed despite EWS detecting failure?

WebCore.lib(JSNodeDOMJIT.obj) : error LNK2005: "public: static class WTF::RefPtr<class JSC::DOMJIT::Patchpoint> __cdecl WebCore::JSNode::checkSubClassPatchpoint(void)" (?checkSubClassPatchpoint@JSNode@WebCore@@SA?AV?$RefPtr@VPatchpoint@DOMJIT@JSC@@@WTF@@XZ) already defined in WebKit.lib(WebKit.dll) [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
WebCore.lib(JSDocumentDOMJIT.obj) : error LNK2005: "public: static class WTF::RefPtr<class JSC::DOMJIT::Patchpoint> __cdecl WebCore::JSDocument::checkSubClassPatchpoint(void)" (?checkSubClassPatchpoint@JSDocument@WebCore@@SA?AV?$RefPtr@VPatchpoint@DOMJIT@JSC@@@WTF@@XZ) already defined in WebKit.lib(WebKit.dll) [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
     Creating library C:/cygwin/home/buildbot/slave/win-release/build/WebKitBuild/Release/lib32/TestWebCoreLib.lib and object C:/cygwin/home/buildbot/slave/win-release/build/WebKitBuild/Release/lib32/TestWebCoreLib.exp
C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\bin32\TestWebCoreLib.dll : fatal error LNK1169: one or more multiply defined symbols found [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
WebCore.lib(JSNodeDOMJIT.obj) : error LNK2005: "public: static class WTF::RefPtr<class JSC::DOMJIT::Patchpoint> __cdecl WebCore::JSNode::checkSubClassPatchpoint(void)" (?checkSubClassPatchpoint@JSNode@WebCore@@SA?AV?$RefPtr@VPatchpoint@DOMJIT@JSC@@@WTF@@XZ) already defined in WebKit.lib(WebKit.dll) [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebKitLib.vcxproj]
WebCore.lib(JSDocumentDOMJIT.obj) : error LNK2005: "public: static class WTF::RefPtr<class JSC::DOMJIT::Patchpoint> __cdecl WebCore::JSDocument::checkSubClassPatchpoint(void)" (?checkSubClassPatchpoint@JSDocument@WebCore@@SA?AV?$RefPtr@VPatchpoint@DOMJIT@JSC@@@WTF@@XZ) already defined in WebKit.lib(WebKit.dll) [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebKitLib.vcxproj]
     Creating library C:/cygwin/home/buildbot/slave/win-release/build/WebKitBuild/Release/lib32/TestWebKitLib.lib and object C:/cygwin/home/buildbot/slave/win-release/build/WebKitBuild/Release/lib32/TestWebKitLib.exp
C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\bin32\TestWebKitLib.dll : fatal error LNK1169: one or more multiply defined symbols found [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebKitLib.vcxproj]

[Yusuke Suzuki]

(In reply to Alexey Proskuryakov from comment #22)
> Windows build remains broken. How did this get landed despite EWS detecting
> failure?
> 
> WebCore.lib(JSNodeDOMJIT.obj) : error LNK2005: "public: static class
> WTF::RefPtr<class JSC::DOMJIT::Patchpoint> __cdecl
> WebCore::JSNode::checkSubClassPatchpoint(void)"
> (?checkSubClassPatchpoint@JSNode@WebCore@@SA?AV?$RefPtr@VPatchpoint@DOMJIT@JS
> C@@@WTF@@XZ) already defined in WebKit.lib(WebKit.dll)
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
> WebCore.lib(JSDocumentDOMJIT.obj) : error LNK2005: "public: static class
> WTF::RefPtr<class JSC::DOMJIT::Patchpoint> __cdecl
> WebCore::JSDocument::checkSubClassPatchpoint(void)"
> (?checkSubClassPatchpoint@JSDocument@WebCore@@SA?AV?$RefPtr@VPatchpoint@DOMJI
> T@JSC@@@WTF@@XZ) already defined in WebKit.lib(WebKit.dll)
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
>      Creating library
> C:/cygwin/home/buildbot/slave/win-release/build/WebKitBuild/Release/lib32/
> TestWebCoreLib.lib and object
> C:/cygwin/home/buildbot/slave/win-release/build/WebKitBuild/Release/lib32/
> TestWebCoreLib.exp
> C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\bin32\TestWebCoreLib.dll : fatal error
> LNK1169: one or more multiply defined symbols found
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
> WebCore.lib(JSNodeDOMJIT.obj) : error LNK2005: "public: static class
> WTF::RefPtr<class JSC::DOMJIT::Patchpoint> __cdecl
> WebCore::JSNode::checkSubClassPatchpoint(void)"
> (?checkSubClassPatchpoint@JSNode@WebCore@@SA?AV?$RefPtr@VPatchpoint@DOMJIT@JS
> C@@@WTF@@XZ) already defined in WebKit.lib(WebKit.dll)
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebKitLib.vcxproj]
> WebCore.lib(JSDocumentDOMJIT.obj) : error LNK2005: "public: static class
> WTF::RefPtr<class JSC::DOMJIT::Patchpoint> __cdecl
> WebCore::JSDocument::checkSubClassPatchpoint(void)"
> (?checkSubClassPatchpoint@JSDocument@WebCore@@SA?AV?$RefPtr@VPatchpoint@DOMJI
> T@JSC@@@WTF@@XZ) already defined in WebKit.lib(WebKit.dll)
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebKitLib.vcxproj]
>      Creating library
> C:/cygwin/home/buildbot/slave/win-release/build/WebKitBuild/Release/lib32/
> TestWebKitLib.lib and object
> C:/cygwin/home/buildbot/slave/win-release/build/WebKitBuild/Release/lib32/
> TestWebKitLib.exp
> C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\bin32\TestWebKitLib.dll : fatal error
> LNK1169: one or more multiply defined symbols found
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebKitLib.vcxproj]

We are now tracking it in bug 172267. The problem is that this is highly Windows specific...
Does any Windows folks know about this super strange behavior?

[Brent Fulgham]

(In reply to Yusuke Suzuki from comment #23)
> (In reply to Alexey Proskuryakov from comment #22)
> > Windows build remains broken. How did this get landed despite EWS detecting
> > failure?
> > 
> > WebCore.lib(JSNodeDOMJIT.obj) : error LNK2005: "public: static class
> > WTF::RefPtr<class JSC::DOMJIT::Patchpoint> __cdecl
> > WebCore::JSNode::checkSubClassPatchpoint(void)"
> > (?checkSubClassPatchpoint@JSNode@WebCore@@SA?AV?$RefPtr@VPatchpoint@DOMJIT@JS
> > C@@@WTF@@XZ) already defined in WebKit.lib(WebKit.dll)
> > [C:\cygwin\home\buildbot\slave\win-
> > release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
> > WebCore.lib(JSDocumentDOMJIT.obj) : error LNK2005: "public: static class
> > WTF::RefPtr<class JSC::DOMJIT::Patchpoint> __cdecl
> > WebCore::JSDocument::checkSubClassPatchpoint(void)"
> > (?checkSubClassPatchpoint@JSDocument@WebCore@@SA?AV?$RefPtr@VPatchpoint@DOMJI
> > T@JSC@@@WTF@@XZ) already defined in WebKit.lib(WebKit.dll)
> > [C:\cygwin\home\buildbot\slave\win-
> > release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
> >      Creating library
> > C:/cygwin/home/buildbot/slave/win-release/build/WebKitBuild/Release/lib32/
> > TestWebCoreLib.lib and object
> > C:/cygwin/home/buildbot/slave/win-release/build/WebKitBuild/Release/lib32/
> > TestWebCoreLib.exp
> > C:\cygwin\home\buildbot\slave\win-
> > release\build\WebKitBuild\Release\bin32\TestWebCoreLib.dll : fatal error
> > LNK1169: one or more multiply defined symbols found
> > [C:\cygwin\home\buildbot\slave\win-
> > release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
> > WebCore.lib(JSNodeDOMJIT.obj) : error LNK2005: "public: static class
> > WTF::RefPtr<class JSC::DOMJIT::Patchpoint> __cdecl
> > WebCore::JSNode::checkSubClassPatchpoint(void)"
> > (?checkSubClassPatchpoint@JSNode@WebCore@@SA?AV?$RefPtr@VPatchpoint@DOMJIT@JS
> > C@@@WTF@@XZ) already defined in WebKit.lib(WebKit.dll)
> > [C:\cygwin\home\buildbot\slave\win-
> > release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebKitLib.vcxproj]
> > WebCore.lib(JSDocumentDOMJIT.obj) : error LNK2005: "public: static class
> > WTF::RefPtr<class JSC::DOMJIT::Patchpoint> __cdecl
> > WebCore::JSDocument::checkSubClassPatchpoint(void)"
> > (?checkSubClassPatchpoint@JSDocument@WebCore@@SA?AV?$RefPtr@VPatchpoint@DOMJI
> > T@JSC@@@WTF@@XZ) already defined in WebKit.lib(WebKit.dll)
> > [C:\cygwin\home\buildbot\slave\win-
> > release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebKitLib.vcxproj]
> >      Creating library
> > C:/cygwin/home/buildbot/slave/win-release/build/WebKitBuild/Release/lib32/
> > TestWebKitLib.lib and object
> > C:/cygwin/home/buildbot/slave/win-release/build/WebKitBuild/Release/lib32/
> > TestWebKitLib.exp
> > C:\cygwin\home\buildbot\slave\win-
> > release\build\WebKitBuild\Release\bin32\TestWebKitLib.dll : fatal error
> > LNK1169: one or more multiply defined symbols found
> > [C:\cygwin\home\buildbot\slave\win-
> > release\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebKitLib.vcxproj]
> 
> We are now tracking it in bug 172267. The problem is that this is highly
> Windows specific...
> Does any Windows folks know about this super strange behavior?

I think we should roll this out until a fix is available. We are losing Windows testing support at present.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 172293

[Yusuke Suzuki]

Created attachment 310637 [details]
Patch

[Yusuke Suzuki]

Committed r217108: <http://trac.webkit.org/changeset/217108>