Bug 170672

Created attachment 306686 [details] Patch

Comment on attachment 306686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306686&action=review Nice, r=me with comment. > Source/JavaScriptCore/b3/air/AirCCallSpecial.cpp:137 > + jit.call(scratchRegister); Could you add a test to testair to ensure that the bug is fixed?

Comment on attachment 306686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306686&action=review >> Source/JavaScriptCore/b3/air/AirCCallSpecial.cpp:137 >> + jit.call(scratchRegister); > > Could you add a test to testair to ensure that the bug is fixed? And I think it is not necessary for x86. Please add `isX86()` guard.

(In reply to Yusuke Suzuki from comment #2) > Comment on attachment 306686 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=306686&action=review > > Nice, r=me with comment. > > > Source/JavaScriptCore/b3/air/AirCCallSpecial.cpp:137 > > + jit.call(scratchRegister); > > Could you add a test to testair to ensure that the bug is fixed? I'm having some issues constructing the test in testair. As far as I understand a valid B3::CCallValue object is necessary when generating the Patch instruction that uses the problematic CCallSpecial. That requires including a bunch of B3 classes into testair.cpp, which I'm not certain is acceptable. Testing this in testb3 might also be difficult because the problem manifests when using an optimization level lower than the default (2).

(In reply to Zan Dobersek from comment #4) > (In reply to Yusuke Suzuki from comment #2) > > Comment on attachment 306686 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=306686&action=review > > > > Nice, r=me with comment. > > > > > Source/JavaScriptCore/b3/air/AirCCallSpecial.cpp:137 > > > + jit.call(scratchRegister); > > > > Could you add a test to testair to ensure that the bug is fixed? > > I'm having some issues constructing the test in testair. As far as I > understand a valid B3::CCallValue object is necessary when generating the > Patch instruction that uses the problematic CCallSpecial. That requires > including a bunch of B3 classes into testair.cpp, which I'm not certain is > acceptable. > > Testing this in testb3 might also be difficult because the problem manifests > when using an optimization level lower than the default (2). I think you should test it in testb3 and then just manually set the optimization level on the procedure you create.

Any luck writing a test, Zan?

(In reply to Saam Barati from comment #6) > Any luck writing a test, Zan? No. I'm able to write out a minimized B3 procedure that ends up generating a CCall instruction in Air, but I can't determine what gets the callee argument generated in the address form in the stress tests so that I could reproduce that in the B3 test case.

Created attachment 308092 [details] Verbose compilation output of a failing stress test Here's the verbose compilation output for one failing stress test. Right before the Air generation, there are two problematic patchpoints that use the callee in address form, in BB#10 and BB#22. If anyone wants to take this over, feel free to do so.

The problem turned out to be a combination of linear scan usage (enabled when optimization level is lowered below 2) and the callee argument of a Air::CCallSpecial being spilt on the stack.

Created attachment 308715 [details] Patch

Attachment 308715 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/testb3.cpp:10456: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 308716 [details] Patch

(In reply to Zan Dobersek from comment #12) > Created attachment 308716 [details] > Patch This takes a different approach, disallowing spilling the callee argument on the stack for ARM64. I'm not sure this is preferable to special-casing for the Arg::Addr callee in generate() (as proposed in comment #3) -- input welcome.

(In reply to Zan Dobersek from comment #13) > (In reply to Zan Dobersek from comment #12) > > Created attachment 308716 [details] > > Patch > > This takes a different approach, disallowing spilling the callee argument on > the stack for ARM64. I'm not sure this is preferable to special-casing for > the Arg::Addr callee in generate() (as proposed in comment #3) -- input > welcome. Why doesn't this effect x86? Can it encode calls to arbitrary addresses?

Fil, Michael, any thoughts?

(In reply to Saam Barati from comment #14) > (In reply to Zan Dobersek from comment #13) > > (In reply to Zan Dobersek from comment #12) > > > Created attachment 308716 [details] > > > Patch > > > > This takes a different approach, disallowing spilling the callee argument on > > the stack for ARM64. I'm not sure this is preferable to special-casing for > > the Arg::Addr callee in generate() (as proposed in comment #3) -- input > > welcome. > > Why doesn't this effect x86? Can it encode calls to arbitrary addresses? Yes.

Comment on attachment 308716 [details] Patch Attachment 308716 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3653003 New failing tests: fast/dom/Window/slow-unload-handler-overwritten-date.html

Created attachment 308759 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 308716 [details] Patch Clearing flags on attachment: 308716 Committed r216058: <http://trac.webkit.org/changeset/216058>

All reviewed patches have been landed. Closing bug.