Bug 204404 – Regression (r252680): JSCOnly build broken: no matching function for call to JSC::DFG::SpeculativeJIT::jsValueResult

• Component: New Bugs
• Status: RESOLVED FIXED

[Aakash Jain]

JSCOnly build seems to be broken currently (for ARMv7 and MIPS32el).

From https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/builds/10050:


FAILED: Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/dfg/DFGSpeculativeJIT.cpp.o 
/home/buildbot/buildroot/buildroot/output.rpi3/host/usr/ccache/arm-buildroot-linux-gnueabihf-g++ ... -c ../../Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp: In member function ‘void JSC::DFG::SpeculativeJIT::compileIncOrDec(JSC::DFG::Node*)’:

../../Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4625:37: error: no matching function for call to ‘JSC::DFG::SpeculativeJIT::jsValueResult(JSC::GPRReg, JSC::DFG::Node*&)’
     jsValueResult(result.gpr(), node);

[Aakash Jain]

Seems to be broken between r252678 and r252682.


JSCOnly Linux MIPS32el Release:
failed on: r252685 https://build.webkit.org/builders/JSCOnly%20Linux%20MIPS32el%20Release/builds/4021 
passed on: r252678  https://build.webkit.org/builders/JSCOnly%20Linux%20MIPS32el%20Release/builds/4020 


JSCOnly Linux ARMv7 Thumb2 Release
failed on: r252682 https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/builds/10050
passed on: r252670 https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/builds/10049

[Aakash Jain]

Almost certainly https://trac.webkit.org/changeset/252680/webkit 

That commit modified DFGSpeculativeJIT.cpp. More specifically it added jsValueResult() call inside SpeculativeJIT().

The landed patch wasn't run through EWS, and EWS had few complaints about previous patches in that bug (e.g.: JSC EWS complained about failing JSC tests). Unfortunately JSCOnly EWS from 'old EWS' wasn't run on this patch as this queue was behind.

[Caio Lima]

There are 2 Patches that broke builds https://trac.webkit.org/changeset/252684 and https://trac.webkit.org/changeset/252680

The fix to https://trac.webkit.org/changeset/252680 is trivial, but I'm working on a proper fix to https://trac.webkit.org/changeset/252684 in https://bugs.webkit.org/show_bug.cgi?id=204082. I'll try to fix both issues there. If it takes longer than expected, I'll create a patch to fix it ASAP.

[Caio Lima]

Created attachment 383955 [details]
Patch

It fixes the build issues, but it won't have desired behavior until we fix https://bugs.webkit.org/show_bug.cgi?id=204082

[Caio Lima]

(In reply to Caio Lima from comment #4)
> Created attachment 383955 [details]
> Patch
> 
> It fixes the build issues, but it won't have desired behavior until we fix
> https://bugs.webkit.org/show_bug.cgi?id=204082

I uploaded the wrong diff.

[Caio Lima]

Created attachment 383956 [details]
Patch

[Aakash Jain]

rs=me

[Caio Lima]

Created attachment 383963 [details]
Patch

[Saam Barati]

Comment on attachment 383963 [details]
Patch

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

r=me. I’ll let you decide if you want to wait on having the baseline use the IC.

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1154
> +                jit.loadValue(CCallHelpers::BaseIndex(scratchGPR, scratch2GPR, CCallHelpers::TimesEight), JSValueRegs::payloadOnly(scratchGPR));

This won’t work for 32-bit. Is the goal just to get it to compile?

> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:-149
> -    callOperation(operationGetByVal, dst, m_codeBlock->globalObject(), JSValueRegs(regT1, regT0), JSValueRegs(regT3, regT2));

Why not wait to do this until it all works?

[Caio Lima]

Comment on attachment 383963 [details]
Patch

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

Thank you very much for the review!

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:1154
>> +                jit.loadValue(CCallHelpers::BaseIndex(scratchGPR, scratch2GPR, CCallHelpers::TimesEight), JSValueRegs::payloadOnly(scratchGPR));
> 
> This won’t work for 32-bit. Is the goal just to get it to compile?

Yes. I already fixed it to 32-bit locally on GetByVal IC changes.

>> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:-149
>> -    callOperation(operationGetByVal, dst, m_codeBlock->globalObject(), JSValueRegs(regT1, regT0), JSValueRegs(regT3, regT2));
> 
> Why not wait to do this until it all works?

This also caused a compilation error, since it requires we move `operationGetByVal` out of "DFGOperations.h" or include this header in this file.  I think that falling back to slowpath always is a better solution to make it compile.

[WebKit Commit Bot]

Comment on attachment 383963 [details]
Patch

Clearing flags on attachment: 383963

Committed r252690: <https://trac.webkit.org/changeset/252690>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57362597>