Meta-program setupArguments and callOperation
Created attachment 334853 [details] WIP
Created attachment 334929 [details] WIP needs 32-bit and Baseline
Comment on attachment 334929 [details] WIP needs 32-bit and Baseline looks pretty good so far
Comment on attachment 334929 [details] WIP needs 32-bit and Baseline So much red 👌🏽
Comment on attachment 334929 [details] WIP needs 32-bit and Baseline View in context: https://bugs.webkit.org/attachment.cgi?id=334929&action=review I like how much red is in the patch. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:937 > + callOperation(OperationType operation, ResultRegType result, Args... args) Args&&... args and below std::forward<Args>(args)... > Source/JavaScriptCore/jit/CCallHelpers.h:413 > + || std::is_convertible<Arg, TrustedImm>::value> // We have this since Speculative JIT has it's own implementation of TrustedImmPtr "its" > Source/JavaScriptCore/jit/FPRInfo.h:309 > +// We using this hack for getting the FPRInfo from the FPRReg type in templates because our code is bad and we should feel bad.. We using? > Source/WTF/wtf/FunctionTraits.h:36 > +struct FunctionTraits<Result(Args...)> { mlam and I were just checking out std::result_of versus std::invoke_result yesterday. Context turns out to be http://wg21.link/P0604
Created attachment 335218 [details] Patch
Created attachment 335220 [details] Patch
Comment on attachment 335220 [details] Patch rs=me
Created attachment 335225 [details] Patch
Created attachment 335227 [details] Patch
Just as a heads up this is definitely going to break the ARM32 JITs.
(In reply to Keith Miller from comment #11) > Just as a heads up this is definitely going to break the ARM32 JITs. Also, probably MIPS.
Comment on attachment 335227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335227&action=review LGTM too > Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:2248 > + // FIXME: This is kinda a hack since we don't use xmm7 as a temp. Bug #? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4317 > +// m_jit.probe([=] (Probe::Context& context) { Remove > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10145 > + operationSwitchString, string, size_t(data->switchTableIndex), string); Static cast > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10182 > + callOperation(operationSwitchString, string, size_t(data->switchTableIndex), string); Ditto > Source/JavaScriptCore/jit/FPRInfo.h:309 > +// We using this hack for getting the FPRInfo from the FPRReg type in templates because our code is bad and we should feel bad.. Is this actually true? Doesn’t seem like this does anything > Source/JavaScriptCore/jit/FPRInfo.h:310 > +constexpr FPRInfo toInfoFromReg(FPRReg) { return FPRInfo(); } Also, “we using” in above comment > Source/JavaScriptCore/jit/GPRInfo.h:811 > +// We using this hack for getting the GPRInfo from the GPRReg type in templates because our code is bad and we should feel bad.. Ditto
Thanks for the heads-up!
Created attachment 335236 [details] Patch
Comment on attachment 335227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335227&action=review uuse >> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:2248 >> + // FIXME: This is kinda a hack since we don't use xmm7 as a temp. > > Bug #? I'm gonna make this not a FIXME since I don't think there's an alternative... >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4317 >> +// m_jit.probe([=] (Probe::Context& context) { > > Remove Fixed. >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10145 >> + operationSwitchString, string, size_t(data->switchTableIndex), string); > > Static cast changed. >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10182 >> + callOperation(operationSwitchString, string, size_t(data->switchTableIndex), string); > > Ditto changed. >> Source/JavaScriptCore/jit/FPRInfo.h:309 >> +// We using this hack for getting the FPRInfo from the FPRReg type in templates because our code is bad and we should feel bad.. > > Is this actually true? Doesn’t seem like this does anything What do you mean? If you have a function: template<typename RegType> void foo(RegType reg) { using RegInfo = decltype(toInfoFromReg(reg)); { How do you get the RegType's Info class without something like this? >> Source/JavaScriptCore/jit/FPRInfo.h:310 >> +constexpr FPRInfo toInfoFromReg(FPRReg) { return FPRInfo(); } > > Also, “we using” in above comment Fixed. >> Source/JavaScriptCore/jit/GPRInfo.h:811 >> +// We using this hack for getting the GPRInfo from the GPRReg type in templates because our code is bad and we should feel bad.. > > Ditto Fixed.
Comment on attachment 335236 [details] Patch Attachment 335236 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/6847337 New failing tests: imported/w3c/web-platform-tests/dom/nodes/Document-characterSet-normalization.html js/dom/dfg-custom-getter-throw-inlined.html js/dom/dfg-inline-resolve.html fast/workers/worker-cloneport.html imported/w3c/web-platform-tests/WebCryptoAPI/generateKey/successes_AES-CBC.worker.html js/dom/dfg-custom-getter-throw.html
Created attachment 335247 [details] Archive of layout-test-results from ews116 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-sierra Platform: Mac OS X 10.12.6
Windows looks like it failed to build for unrelated reasons (built through JSC). Fixed an issue where I wasn't setting vm.topCallFrame before calling into DOM getters (I guess they don't expect to need to set topCallFrame).
Created attachment 335248 [details] Patch
Committed r229391: <https://trac.webkit.org/changeset/229391>
<rdar://problem/38246426>
Really awesome!
The Windows build break from this patch was fixed in https://bugs.webkit.org/show_bug.cgi?id=183450, but this only fixed compile-time errors and not runtime ones. Specifically, WinCairo (and perhaps AppleWin too) crashes when (e.g.) trying to launch Web Inspector as of this revision. The first issue is here, where the array is supposedly uninitialized: https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/jit/CCallHelpers.h#L262 A compiler warning for this was squelched in r229424/r229432 but I don't think that helps at all(?) because there's still a runtime error either way. If I add braces to the std::array declaration, the next error is here: https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/interpreter/Register.h#L203 With the following call stack: > JavaScriptCore.dll!JSC::Register::pointer() Line 203 > JavaScriptCore.dll!JSC::ExecState::callee() Line 108 > JavaScriptCore.dll!JSC::ExecState::vm() Line 129 > JavaScriptCore.dll!operationOptimize(JSC::ExecState * exec, unsigned int bytecodeIndex) Line 1302 > [External Code]
(In reply to Ross Kirsling from comment #24) > The Windows build break from this patch was fixed in > https://bugs.webkit.org/show_bug.cgi?id=183450, but this only fixed > compile-time errors and not runtime ones. > > Specifically, WinCairo (and perhaps AppleWin too) crashes when (e.g.) trying > to launch Web Inspector as of this revision. AFAICT, the AppleWin bots don't have any new failing tests from this patch. > > The first issue is here, where the array is supposedly uninitialized: > https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/jit/ > CCallHelpers.h#L262 > A compiler warning for this was squelched in r229424/r229432 but I don't > think that helps at all(?) because there's still a runtime error either way. Oh? I thought all of the following were equivalent in C++ (at least since C++11): T v; T v(); T v { }; If so, all that would happen is that the std::array would contain indeterminate values until they were initialized in the loop below. Maybe I'm wrong though? > > If I add braces to the std::array declaration, the next error is here: > https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/ > interpreter/Register.h#L203 > With the following call stack: > > JavaScriptCore.dll!JSC::Register::pointer() Line 203 > > JavaScriptCore.dll!JSC::ExecState::callee() Line 108 > > JavaScriptCore.dll!JSC::ExecState::vm() Line 129 > > JavaScriptCore.dll!operationOptimize(JSC::ExecState * exec, unsigned int bytecodeIndex) Line 1302 > > [External Code] Seems weird that there would be a crash there since that's just a cast. Is this from a release build?
(In reply to Keith Miller from comment #25) > (In reply to Ross Kirsling from comment #24) > > The Windows build break from this patch was fixed in > > https://bugs.webkit.org/show_bug.cgi?id=183450, but this only fixed > > compile-time errors and not runtime ones. > > > > Specifically, WinCairo (and perhaps AppleWin too) crashes when (e.g.) trying > > to launch Web Inspector as of this revision. > > AFAICT, the AppleWin bots don't have any new failing tests from this patch. Damn, I was hoping our destinies would align. :P > > > > The first issue is here, where the array is supposedly uninitialized: > > https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/jit/ > > CCallHelpers.h#L262 > > A compiler warning for this was squelched in r229424/r229432 but I don't > > think that helps at all(?) because there's still a runtime error either way. > > Oh? I thought all of the following were equivalent in C++ (at least since > C++11): > > T v; > T v(); > T v { }; > > If so, all that would happen is that the std::array would contain > indeterminate values until they were initialized in the loop below. Maybe > I'm wrong though? While `T v();` would be taken as a function declaration (the so-called "most vexing parse"), my understanding too is that adding braces shouldn't help, so I was surprised when they did. Perhaps this is disguising a different issue -- is it problematic that TargetSize is 0? > > > > If I add braces to the std::array declaration, the next error is here: > > https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/ > > interpreter/Register.h#L203 > > With the following call stack: > > > JavaScriptCore.dll!JSC::Register::pointer() Line 203 > > > JavaScriptCore.dll!JSC::ExecState::callee() Line 108 > > > JavaScriptCore.dll!JSC::ExecState::vm() Line 129 > > > JavaScriptCore.dll!operationOptimize(JSC::ExecState * exec, unsigned int bytecodeIndex) Line 1302 > > > [External Code] > > Seems weird that there would be a crash there since that's just a cast. Is > this from a release build? Whoops, I stupidly forgot to say what the error is -- that `this` is null. It's from a Debug build, as I was getting awfully confused trying to debug in Release mode, hehe.
(In reply to Ross Kirsling from comment #26) > (In reply to Keith Miller from comment #25) > > (In reply to Ross Kirsling from comment #24) > > > The Windows build break from this patch was fixed in > > > https://bugs.webkit.org/show_bug.cgi?id=183450, but this only fixed > > > compile-time errors and not runtime ones. > > > > > > Specifically, WinCairo (and perhaps AppleWin too) crashes when (e.g.) trying > > > to launch Web Inspector as of this revision. > > > > AFAICT, the AppleWin bots don't have any new failing tests from this patch. > > Damn, I was hoping our destinies would align. :P It looks like AppleWin is at least affected by the same uninitialized Run-time check failure. I downloaded the latest AppleWin artifacts (https://s3-us-west-2.amazonaws.com/archives.webkit.org/win-i386-debug/229557.zip) and tested a simple looped function call. function foo(a){ a += 1; }; for (i=0; i<1000; i++) foo(1); print('done'); 'done' is not getting printed unless I lower the maximum iterations to around 10. I attached a debugger to confirm that it was hitting that same uninitialized Run-time check failure. I'm not sure if I can easily check to see if AppleWin hitting that null crash too without getting a build going.
(In reply to Christopher Reid from comment #27) > (In reply to Ross Kirsling from comment #26) > > (In reply to Keith Miller from comment #25) > > > (In reply to Ross Kirsling from comment #24) > > > > The Windows build break from this patch was fixed in > > > > https://bugs.webkit.org/show_bug.cgi?id=183450, but this only fixed > > > > compile-time errors and not runtime ones. > > > > > > > > Specifically, WinCairo (and perhaps AppleWin too) crashes when (e.g.) trying > > > > to launch Web Inspector as of this revision. > > > > > > AFAICT, the AppleWin bots don't have any new failing tests from this patch. > > > > Damn, I was hoping our destinies would align. :P > > It looks like AppleWin is at least affected by the same uninitialized > Run-time check failure. I downloaded the latest AppleWin artifacts > (https://s3-us-west-2.amazonaws.com/archives.webkit.org/win-i386-debug/ > 229557.zip) and tested a simple looped function call. > > function foo(a){ > a += 1; > }; > for (i=0; i<1000; i++) > foo(1); > print('done'); > > 'done' is not getting printed unless I lower the maximum iterations to > around 10. I attached a debugger to confirm that it was hitting that same > uninitialized Run-time check failure. > > I'm not sure if I can easily check to see if AppleWin hitting that null > crash too without getting a build going. Interesting... It's weird that the bots are not showing it? https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29?numbuilds=200 doesn't show any new regressions after the patch landed. (In reply to Ross Kirsling from comment #26) > (In reply to Keith Miller from comment #25) > > (In reply to Ross Kirsling from comment #24) > > > The Windows build break from this patch was fixed in > > > https://bugs.webkit.org/show_bug.cgi?id=183450, but this only fixed > > > compile-time errors and not runtime ones. > > > > > > Specifically, WinCairo (and perhaps AppleWin too) crashes when (e.g.) trying > > > to launch Web Inspector as of this revision. > > > > AFAICT, the AppleWin bots don't have any new failing tests from this patch. > > Damn, I was hoping our destinies would align. :P > > > > > > > The first issue is here, where the array is supposedly uninitialized: > > > https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/jit/ > > > CCallHelpers.h#L262 > > > A compiler warning for this was squelched in r229424/r229432 but I don't > > > think that helps at all(?) because there's still a runtime error either way. > > > > Oh? I thought all of the following were equivalent in C++ (at least since > > C++11): > > > > T v; > > T v(); > > T v { }; > > > > If so, all that would happen is that the std::array would contain > > indeterminate values until they were initialized in the loop below. Maybe > > I'm wrong though? > > While `T v();` would be taken as a function declaration (the so-called "most > vexing parse"), my understanding too is that adding braces shouldn't help, > so I was surprised when they did. Perhaps this is disguising a different > issue -- is it problematic that TargetSize is 0? Right, C++ is such a beautiful language... I don't think it should be a problem if TargetSize is 0. We only ever loop on the iterator or indexed on the size of the array. If the size of the std::array is 0 then begin() == end() so the loops should never run. Maybe there's some weird MSVC bug? I don't have a windows machine right now but I might be able to find one and take a look. If you could run: function foo(a){ a += 1; }; for (i=0; i<1000; i++) foo(1); print('done'); again in jsc. With `--dumpDisassembly=1 --useDFGJIT=0` and send me the Baseline disassembly that might help me fix the issue too.
Created attachment 335721 [details] Baseline disassembly (WinCairo 64-bit Debug) Here's the disassembly with the options you requested. I noticed that (on my machine at least) the crash occurs when the loop has 485+ iterations, but not with less than that.
(In reply to Keith Miller from comment #28) > (In reply to Christopher Reid from comment #27) > > (In reply to Ross Kirsling from comment #26) > > > (In reply to Keith Miller from comment #25) > > > > (In reply to Ross Kirsling from comment #24) > > > > > The Windows build break from this patch was fixed in > > > > > https://bugs.webkit.org/show_bug.cgi?id=183450, but this only fixed > > > > > compile-time errors and not runtime ones. > > > > > > > > > > Specifically, WinCairo (and perhaps AppleWin too) crashes when (e.g.) trying > > > > > to launch Web Inspector as of this revision. > > > > > > > > AFAICT, the AppleWin bots don't have any new failing tests from this patch. > > > > > > Damn, I was hoping our destinies would align. :P > > > > It looks like AppleWin is at least affected by the same uninitialized > > Run-time check failure. I downloaded the latest AppleWin artifacts > > (https://s3-us-west-2.amazonaws.com/archives.webkit.org/win-i386-debug/ > > 229557.zip) and tested a simple looped function call. > > > > function foo(a){ > > a += 1; > > }; > > for (i=0; i<1000; i++) > > foo(1); > > print('done'); > > > > 'done' is not getting printed unless I lower the maximum iterations to > > around 10. I attached a debugger to confirm that it was hitting that same > > uninitialized Run-time check failure. > > > > I'm not sure if I can easily check to see if AppleWin hitting that null > > crash too without getting a build going. > > Interesting... It's weird that the bots are not showing it? > https://build.webkit.org/builders/ > Apple%20Win%207%20Release%20%28Tests%29?numbuilds=200 doesn't show any new > regressions after the patch landed. Ah, it looks like the RTC failure is only happening on debug builds and AppleWin tests are running on release. The crash looks Wincario specific as it crashes in Wincairo release but not on AppleWin release. I'm not sure what could be causing that, but it's starting to look like a separate issue. I took a look at the MSVC disassembly https://godbolt.org/g/FMCe9b. For whatever reason MSVC is initializing the std::array to null causing _RTC_UninitUse to get hit. With braces, the _RTC_UninitUse call doesn't get compiled in. Adding braces is looking like the best fix here.
(In reply to Christopher Reid from comment #30) > (In reply to Keith Miller from comment #28) > > (In reply to Christopher Reid from comment #27) > > > (In reply to Ross Kirsling from comment #26) > > > > (In reply to Keith Miller from comment #25) > > > > > (In reply to Ross Kirsling from comment #24) > > > > > > The Windows build break from this patch was fixed in > > > > > > https://bugs.webkit.org/show_bug.cgi?id=183450, but this only fixed > > > > > > compile-time errors and not runtime ones. > > > > > > > > > > > > Specifically, WinCairo (and perhaps AppleWin too) crashes when (e.g.) trying > > > > > > to launch Web Inspector as of this revision. > > > > > > > > > > AFAICT, the AppleWin bots don't have any new failing tests from this patch. > > > > > > > > Damn, I was hoping our destinies would align. :P > > > > > > It looks like AppleWin is at least affected by the same uninitialized > > > Run-time check failure. I downloaded the latest AppleWin artifacts > > > (https://s3-us-west-2.amazonaws.com/archives.webkit.org/win-i386-debug/ > > > 229557.zip) and tested a simple looped function call. > > > > > > function foo(a){ > > > a += 1; > > > }; > > > for (i=0; i<1000; i++) > > > foo(1); > > > print('done'); > > > > > > 'done' is not getting printed unless I lower the maximum iterations to > > > around 10. I attached a debugger to confirm that it was hitting that same > > > uninitialized Run-time check failure. > > > > > > I'm not sure if I can easily check to see if AppleWin hitting that null > > > crash too without getting a build going. > > > > Interesting... It's weird that the bots are not showing it? > > https://build.webkit.org/builders/ > > Apple%20Win%207%20Release%20%28Tests%29?numbuilds=200 doesn't show any new > > regressions after the patch landed. > > Ah, it looks like the RTC failure is only happening on debug builds and > AppleWin tests are running on release. The crash looks Wincario specific as > it crashes in Wincairo release but not on AppleWin release. I'm not sure > what could be causing that, but it's starting to look like a separate issue. > > I took a look at the MSVC disassembly https://godbolt.org/g/FMCe9b. For > whatever reason MSVC is initializing the std::array to null causing > _RTC_UninitUse to get hit. With braces, the _RTC_UninitUse call doesn't get > compiled in. Adding braces is looking like the best fix here. Seems like perhaps that's the only issue with the example script? But on Web Inspector launch, we still end up with no VM (?) here: https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/jit/JITOperations.cpp#L1302
(In reply to Ross Kirsling from comment #31) > ... on Web Inspector launch, we still end up with no VM (?) here: > https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/jit/ > JITOperations.cpp#L1302 I'm still looking into this, but it appears that `callOperation(operationOptimize, m_bytecodeOffset);` @ JIT::emitEnterOptimizationCheck / JIT::emitSlow_op_loop_hint used to take us into: JIT::appendCallWithExceptionCheckAndSlowPathReturnType JIT::appendCallWithSlowPathReturnType MacroAssemblerX86_64::callWithSlowPathReturnType These functions are all #if'ed for 64-bit Windows and were not removed in the patch, yet they no longer appear to be used anywhere in JSC -- after this patch, we enter the non-SPRT call chain instead. Perhaps this is the cause of the issue?
(In reply to Ross Kirsling from comment #32) > (In reply to Ross Kirsling from comment #31) > > ... on Web Inspector launch, we still end up with no VM (?) here: > > https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/jit/ > > JITOperations.cpp#L1302 > > I'm still looking into this, but it appears that > `callOperation(operationOptimize, m_bytecodeOffset);` @ > JIT::emitEnterOptimizationCheck / JIT::emitSlow_op_loop_hint used to take us > into: > JIT::appendCallWithExceptionCheckAndSlowPathReturnType > JIT::appendCallWithSlowPathReturnType > MacroAssemblerX86_64::callWithSlowPathReturnType > > These functions are all #if'ed for 64-bit Windows and were not removed in > the patch, yet they no longer appear to be used anywhere in JSC -- after > this patch, we enter the non-SPRT call chain instead. Perhaps this is the > cause of the issue? Yes, I think this is a likely cause of crashes on Windows. I just happened to discover this independently while analyzing the code. I filed https://bugs.webkit.org/show_bug.cgi?id=183655 to track this.
(In reply to Mark Lam from comment #33) > Yes, I think this is a likely cause of crashes on Windows. I just happened > to discover this independently while analyzing the code. I filed > https://bugs.webkit.org/show_bug.cgi?id=183655 to track this. Great! I've put up a patch for the braces issue in https://bugs.webkit.org/show_bug.cgi?id=183673.