Bug 206361

Summary: Reland bytecode checkpoints since bugs have been fixed
Product: WebKit Reporter: Keith Miller <keith_miller>
Component: New BugsAssignee: Keith Miller <keith_miller>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, benjamin, cdumez, cmarcelo, commit-queue, dbates, ews-watchlist, gyuyoung.kim, mark.lam, msaboff, pmatos, ryuan.choi, saam, sergio, ticaiolima, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Keith Miller
Reported 2020-01-16 10:13:16 PST
Reland bytecode checkpoints since bugs have been fixed
Attachments
Patch (652.58 KB, patch)
2020-01-16 10:15 PST, Keith Miller 		no flags
DetailsFormatted DiffDiff
Patch (652.37 KB, patch)
2020-01-16 11:09 PST, Keith Miller 		no flags
DetailsFormatted DiffDiff
Patch (727.31 KB, patch)
2020-01-16 13:41 PST, Keith Miller 		no flags
DetailsFormatted DiffDiff
Patch (727.32 KB, patch)
2020-01-16 15:47 PST, Keith Miller 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Keith Miller
Comment 1 2020-01-16 10:15:37 PST
Created attachment 387929 [details] Patch
Caio Lima
Comment 2 2020-01-16 10:51:25 PST
Comment on attachment 387929 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387929&action=review > Source/JavaScriptCore/interpreter/CallFrame.h:55 > +#else Now we use PC base register on 32-bits, we don't need this constructor anymore. Could you remove this IFDEF?
Keith Miller
Comment 3 2020-01-16 11:01:25 PST
Comment on attachment 387929 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387929&action=review >> Source/JavaScriptCore/interpreter/CallFrame.h:55 >> +#else > > Now we use PC base register on 32-bits, we don't need this constructor anymore. Could you remove this IFDEF? Whoops, will fix.
Keith Miller
Comment 4 2020-01-16 11:09:08 PST
Created attachment 387932 [details] Patch
Caio Lima
Comment 5 2020-01-16 11:32:11 PST
Comment on attachment 387932 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387932&action=review r- because I've found a bug. > Source/JavaScriptCore/jit/SpecializedThunkJIT.h:58 > + VirtualRegister src = virtualRegisterForArgument(argument); Look that this can cause some troubles on existing code. `CallFrame::argumentOffset` returns the value based on `CallFrameSlot::firstArgument + argument`, however virtualRegisterForArgument uses `argument + CallFrame::thisArgumentOffset();`, making the results off by one. For example, this can cause issues on code generated by `stringCharLoad`, because the assembly generated by `jit.loadJSStringArgument(SpecializedThunkJIT::ThisArgument, SpecializedThunkJIT::regT0);` won't access `thisArgument` correctly (The reason is because `SpecializedThunkJIT::ThisArgument == -1`). We don't get crashes on 64-bits for `charCodeAt` because it always fallback to slow path (I suspect we can observe performance regressions for this issue).
Keith Miller
Comment 6 2020-01-16 13:18:51 PST
Comment on attachment 387932 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387932&action=review >> Source/JavaScriptCore/jit/SpecializedThunkJIT.h:58 >> + VirtualRegister src = virtualRegisterForArgument(argument); > > Look that this can cause some troubles on existing code. `CallFrame::argumentOffset` returns the value based on `CallFrameSlot::firstArgument + argument`, however virtualRegisterForArgument uses `argument + CallFrame::thisArgumentOffset();`, making the results off by one. For example, this can cause issues on code generated by `stringCharLoad`, because the assembly generated by `jit.loadJSStringArgument(SpecializedThunkJIT::ThisArgument, SpecializedThunkJIT::regT0);` won't access `thisArgument` correctly (The reason is because `SpecializedThunkJIT::ThisArgument == -1`). We don't get crashes on 64-bits for `charCodeAt` because it always fallback to slow path (I suspect we can observe performance regressions for this issue). Nice catch. I'm gonna rename virtualRegisterForArgument to virtualRegisterForArgumentIncludingThis so we don't make the same mistake again.
Keith Miller
Comment 7 2020-01-16 13:41:58 PST
Created attachment 387953 [details] Patch
Keith Miller
Comment 8 2020-01-16 15:47:53 PST
Created attachment 387972 [details] Patch
WebKit Commit Bot
Comment 9 2020-01-16 20:08:36 PST
The commit-queue encountered the following flaky tests while processing attachment 387972 [details]: editing/spelling/spellcheck-attribute.html bug 206178 (authors: g.czajkowski@samsung.com, mark.lam@apple.com, and rniwa@webkit.org) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 10 2020-01-16 20:09:44 PST
Comment on attachment 387972 [details] Patch Clearing flags on attachment: 387972 Committed r254735: <https://trac.webkit.org/changeset/254735>
WebKit Commit Bot
Comment 11 2020-01-16 20:09:46 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 12 2020-01-16 20:10:14 PST
<rdar://problem/58669583>
Paulo Matos
Comment 13 2020-01-17 00:43:36 PST
This badly breaks 32bit testing: https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/builds/10571 I will open a bug.
Note You need to log in before you can comment on or make changes to this bug.