Bug 179355 – AccessCase::generateImpl() should exclude the result register when restoring registers after a call.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

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>

[Mark Lam]

Created attachment 326176 [details]
proposed patch.

[Mark Lam]

Created attachment 326177 [details]
proposed patch.

[Michael Saboff]

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.

[Saam Barati]

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.

[Mark Lam]

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.

[Mark Lam]

Created attachment 326222 [details]
proposed patch.

[Mark Lam]

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