[aarch64][Linux] m_allowScratchRegister assert hit in MacroAssemblerARM64 under B3::Air::CCallSpecial::generate()

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[aarch64][Linux] m_allowScratchRegister assert hit in MacroAssembler64 under B3::Air::CCallSpecial::generate()

[Zan Dobersek]

Created attachment 306686 [details]
Patch

[Yusuke Suzuki]

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?

[Yusuke Suzuki]

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.

[Zan Dobersek]

(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).

[Saam Barati]

(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.

[Saam Barati]

Any luck writing a test, Zan?

[Zan Dobersek]

(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.

[Zan Dobersek]

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.

[Zan Dobersek]

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.

[Zan Dobersek]

Created attachment 308715 [details]
Patch

[Build Bot]

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.

[Zan Dobersek]

Created attachment 308716 [details]
Patch

[Zan Dobersek]

(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.

[Saam Barati]

(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?

[Saam Barati]

Fil, Michael, any thoughts?

[Filip Pizlo]

(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.

[Build Bot]

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

[Build Bot]

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

[Zan Dobersek]

Comment on attachment 308716 [details]
Patch

Clearing flags on attachment: 308716

Committed r216058: <http://trac.webkit.org/changeset/216058>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.