| Summary: | [JSC][DFG][DOMJIT] Extend CheckDOM to CheckSubClass | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||
| Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | bfulgham, buildbot, commit-queue, fpizlo, pvollan, rniwa, saam, sam | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| Bug Depends on: | 172293 | ||||||||||||||||||||
| Bug Blocks: | 171637, 172267 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Yusuke Suzuki
2017-05-14 20:06:03 PDT
Created attachment 310110 [details]
Patch
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.
Created attachment 310125 [details]
Patch
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.
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? Created attachment 310345 [details]
Patch
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.
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 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
Created attachment 310354 [details]
Patch
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.
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 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
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 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
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? 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)`. 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 Committed r217031: <http://trac.webkit.org/changeset/217031> Committed r217032: <http://trac.webkit.org/changeset/217032> Committed r217037: <http://trac.webkit.org/changeset/217037> 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]
(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? (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. Re-opened since this is blocked by bug 172293 Created attachment 310637 [details]
Patch
Committed r217108: <http://trac.webkit.org/changeset/217108> |