Bug 179355 - AccessCase::generateImpl() should exclude the result register when restoring registers after a call.
Summary: AccessCase::generateImpl() should exclude the result register when restoring ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-06 17:30 PST by Mark Lam
Modified: 2017-11-07 11:34 PST (History)
8 users (show)

See Also:


Attachments
proposed patch. (12.31 KB, patch)
2017-11-06 17:45 PST, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (12.40 KB, patch)
2017-11-06 17:50 PST, Mark Lam
sbarati: review-
Details | Formatted Diff | Diff
proposed patch. (3.46 KB, patch)
2017-11-07 11:02 PST, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2017-11-06 17:30:01 PST
We should be using this exclude list to exclude result registers from being preserved, instead of needing to exclude them from being restored later.

<rdar://problem/35263053>
Comment 1 Mark Lam 2017-11-06 17:45:09 PST
Created attachment 326176 [details]
proposed patch.
Comment 2 Mark Lam 2017-11-06 17:50:54 PST
Created attachment 326177 [details]
proposed patch.
Comment 3 Michael Saboff 2017-11-06 18:06:58 PST
Comment on attachment 326177 [details]
proposed patch.

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

r=me

> Source/JavaScriptCore/ChangeLog:25
> +        1. Added support to specify an exclude RegisterSet for
> +           AccessGenerationState::preserveLiveRegistersToStackForCall().  This allows us
> +           to exclude the result registers from the set of registers to preserve.
> +
> +        2. Removed the exclude RegisterSet from restoreLiveRegistersFromStackForCall().
> +           Instead, restoreLiveRegistersFromStackForCall() should only restore registers
> +           that were preserved.  This simplifies the restoration code.
> +
> +        3. Changed preserveLiveRegistersToStackForCall() and restoreLiveRegistersFromStackForCall()
> +           to take RegisterSet arguments by value instead of by reference.  RegisterSet
> +           is designed to be passed by value.
> +
> +        4. Added an implicit default constructor to RegisterSet.  This allows us to use
> +           the { } notation for instantiation an empty set.  There is an explicit
> +           RegisterSet(Regs... regs) constructor which was made explicit so that we don't
> +           inadvertently convert register IDs into RegisterSet.  Having a default
> +           constructor does not break intent of the RegisterSet(Regs... regs) constructor.

Most of these are change descriptions for the affected functions.  Move that detail down under the appropriate changed file / function section below.  A summary of what the change does here would be great.
Comment 4 Saam Barati 2017-11-06 18:10:52 PST
Comment on attachment 326177 [details]
proposed patch.

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

> Source/JavaScriptCore/bytecode/AccessCase.cpp:677
> +        RegisterSet resultRegistersToExclude;
> +        if (isGetter())
> +            resultRegistersToExclude.set(valueRegs);

I believe this is wrong. Imagine we make a getter call, and the base/result are allocated the same register. Assume that the getter throws an exception. Assume OSR exit now needs to recover the original base. This code breaks that.
Comment 5 Mark Lam 2017-11-07 10:25:23 PST
Comment on attachment 326177 [details]
proposed patch.

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

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:677
>> +            resultRegistersToExclude.set(valueRegs);
> 
> I believe this is wrong. Imagine we make a getter call, and the base/result are allocated the same register. Assume that the getter throws an exception. Assume OSR exit now needs to recover the original base. This code breaks that.

Thanks for catching that.  You made a very good point.  This completely invalidates my approach of skipping the result on the preservation pass.  I will redo the entire patch.  I'll also leave out the refactoring parts for a subsequent patch.
Comment 6 Mark Lam 2017-11-07 11:02:18 PST
Created attachment 326222 [details]
proposed patch.
Comment 7 Mark Lam 2017-11-07 11:34:53 PST
Thanks for the review.  I noticed that the title in the JSTests/ChangeLog was from the old patch.  I've updated it before landing.

Landed in r224539: <http://trac.webkit.org/r224539>.