Created attachment 387929 [details] Patch

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?

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.

Created attachment 387932 [details] Patch

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

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.

Created attachment 387953 [details] Patch

Created attachment 387972 [details] Patch

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.

Comment on attachment 387972 [details] Patch Clearing flags on attachment: 387972 Committed r254735: <https://trac.webkit.org/changeset/254735>

All reviewed patches have been landed. Closing bug.

<rdar://problem/58669583>

This badly breaks 32bit testing: https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/builds/10571 I will open a bug.