| Summary: | [JSC] Attempt to fix BytecodeIndex handling in 32bit | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||
| Component: | New Bugs | Assignee: | 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
Yusuke Suzuki
2020-01-22 03:25:58 PST
Created attachment 388411 [details]
Patch
Created attachment 388413 [details]
Patch
Created attachment 388414 [details]
Patch
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. BTW, I just noticed that we missed adding entries `preserveCalleeSavesUsedByLLInt()` and `restoreCalleeSavesUsedByLLInt()` for the new callee-save PB. Could you add those there? Comment on attachment 388414 [details]
Patch
r=me.
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. (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. Committed r254934: <https://trac.webkit.org/changeset/254934> |