Reland bytecode checkpoints since bugs have been fixed

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

Reland bytecode checkpoints since bugs have been fixed

[Keith Miller]

Created attachment 387929 [details]
Patch

[Caio Lima]

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 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]

Created attachment 387932 [details]
Patch

[Caio Lima]

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 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]

Created attachment 387953 [details]
Patch

[Keith Miller]

Created attachment 387972 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 387972 [details]:

editing/spelling/spellcheck-attribute.html bug 206178 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 387972 [details]
Patch

Clearing flags on attachment: 387972

Committed r254735: <https://trac.webkit.org/changeset/254735>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58669583>

[Paulo Matos]

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

I will open a bug.