Bug 227670 - ActiveScratchBufferScope should take the buffer as argument
Summary: ActiveScratchBufferScope should take the buffer as argument
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Robin Morisset
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-04 00:15 PDT by Robin Morisset
Modified: 2021-07-04 16:55 PDT (History)
7 users (show)

See Also:


Attachments
Patch (11.52 KB, patch)
2021-07-04 00:23 PDT, Robin Morisset
mark.lam: review+
Details | Formatted Diff | Diff
Patch for landing (11.04 KB, patch)
2021-07-04 07:47 PDT, Robin Morisset
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (11.13 KB, patch)
2021-07-04 11:46 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Morisset 2021-07-04 00:15:52 PDT
https://bugs.webkit.org/show_bug.cgi?id=227013 created ActiveScratchBufferScope.
It is used by operations that can cause the GC to run, to mark as roots the contents of the scratch buffer that is live during that time (if any).
The bug is that it simply asks the VM for a scratch buffer of the right size, but this will always return the last scratch buffer, and not necessarily the one that the operations is actually using.

A fairly simple fix is to pass it directly the scratch buffer, since the operation normally can get it easily enough.
In most cases the operation has access to the m_buffer field of the ScratchBuffer, but getting a pointer to the entire structure from that is fairly simple.
Comment 1 Robin Morisset 2021-07-04 00:16:42 PDT
rdar://80011612
Comment 2 Robin Morisset 2021-07-04 00:23:09 PDT
Created attachment 432859 [details]
Patch
Comment 3 Mark Lam 2021-07-04 00:44:27 PDT
Comment on attachment 432859 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432859&action=review

r=me with suggested improvement, and if EWS bots are happy.

> Source/JavaScriptCore/dfg/DFGThunks.cpp:94
> -    // Set up one argument.
> +    // Set up two arguments.
>      jit.move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
> +    jit.move(bufferGPR, GPRInfo::argumentGPR1);

Let's just replace this with:
    jit.setupArguments<decltype(operationCompileOSRExit)>(GPRInfo::callFrameRegister, bufferGPR);

This has the benefit of ensuring that if bufferGPR happens to be GPRInfo::argumentGPR0, that it will do the right thing to shuffle the value out before overwriting it.  And you can also remove the comment now.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1916
> -    
> +

Please undo since there are no other relevant changes in this file.
Comment 4 Robin Morisset 2021-07-04 07:47:37 PDT
Created attachment 432863 [details]
Patch for landing
Comment 5 Robin Morisset 2021-07-04 11:46:22 PDT
Created attachment 432865 [details]
Patch for landing
Comment 6 EWS 2021-07-04 16:55:55 PDT
Committed r279560 (239391@main): <https://commits.webkit.org/239391@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 432865 [details].