Bug 206577

Summary: [JSC] Attempt to fix BytecodeIndex handling in 32bit
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: angelos, cgarcia, ews-watchlist, guijemont, keith_miller, mark.lam, msaboff, pmatos, saam, ticaiolima, tzagallo, webkit-bug-importer, zan
Priority: P2 Keywords: DoNotImportToRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 206602    
Attachments:
Description Flags
Patch
none
Patch
none
Patch keith_miller: review+

Yusuke Suzuki
Reported 2020-01-22 03:25:58 PST
[JSC] Attempt to fix BytecodeIndex handling in 32bit
Attachments
Patch (18.81 KB, patch)
2020-01-22 03:29 PST, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (19.25 KB, patch)
2020-01-22 03:37 PST, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (19.37 KB, patch)
2020-01-22 03:48 PST, Yusuke Suzuki 		keith_miller: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2020-01-22 03:29:36 PST
Created attachment 388411 [details] Patch
Yusuke Suzuki
Comment 2 2020-01-22 03:37:20 PST
Created attachment 388413 [details] Patch
Yusuke Suzuki
Comment 3 2020-01-22 03:48:11 PST
Created attachment 388414 [details] Patch
Caio Lima
Comment 4 2020-01-22 04:45:50 PST
Comment on attachment 388414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388414&action=review LGTM besides an issue I've found. Regarding 32-bits changes, EWS bots won't test them, but we have a WIP patch to re-enable JIT on ARMv7 and MIPS and we've applies those changes as well. > Source/JavaScriptCore/llint/LLIntData.cpp:132 > + ASSERT(CodeBlock::llintBaselineCalleeSaveSpaceAsVirtualRegisters() == 2); The way we calculate `llintBaselineCalleeSaveSpaceAsVirtualRegisters` will make this fail. The problem is that on `roundCalleeSaveSpaceAsVirtualRegisters` we are using `WTF::roundUpToMultipleOf(sizeof(Register), calleeSaveRegisters * sizeof(CPURegister)) / sizeof(Register)`. Having 2 callee-save registers on 32-bits still returns 1.
Caio Lima
Comment 5 2020-01-22 06:20:48 PST
BTW, I just noticed that we missed adding entries `preserveCalleeSavesUsedByLLInt()` and `restoreCalleeSavesUsedByLLInt()` for the new callee-save PB. Could you add those there?
Keith Miller
Comment 6 2020-01-22 10:02:58 PST
Comment on attachment 388414 [details] Patch r=me.
Yusuke Suzuki
Comment 7 2020-01-22 12:28:20 PST
Comment on attachment 388414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388414&action=review Fixed preserveCalleeSavesUsedByLLInt etc. I'm thinking this is also wrong in CLoop. We should fix it in a separate patch. >> Source/JavaScriptCore/llint/LLIntData.cpp:132 >> + ASSERT(CodeBlock::llintBaselineCalleeSaveSpaceAsVirtualRegisters() == 2); > > The way we calculate `llintBaselineCalleeSaveSpaceAsVirtualRegisters` will make this fail. The problem is that on `roundCalleeSaveSpaceAsVirtualRegisters` we are using `WTF::roundUpToMultipleOf(sizeof(Register), calleeSaveRegisters * sizeof(CPURegister)) / sizeof(Register)`. Having 2 callee-save registers on 32-bits still returns 1. Right. Fixed.
Yusuke Suzuki
Comment 8 2020-01-22 12:28:43 PST
(In reply to Yusuke Suzuki from comment #7) > Comment on attachment 388414 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388414&action=review > > Fixed preserveCalleeSavesUsedByLLInt etc. I'm thinking this is also wrong in > CLoop. We should fix it in a separate patch. Maybe no. Need to check.
Yusuke Suzuki
Comment 9 2020-01-22 12:31:37 PST
Committed r254934: <https://trac.webkit.org/changeset/254934>
Radar WebKit Bug Importer
Comment 10 2020-01-22 15:57:50 PST
<rdar://problem/58814703>
Note You need to log in before you can comment on or make changes to this bug.