Introducing ScratchRegisterAllocator::PreservedState.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

restoreReusedRegistersByPopping() should always be called with 2 values that matches the expectation of preserveReusedRegistersByPushing().  Those 2 values are the number of bytes preserved and the ExtraStackSpace requirement.  By encapsulating them in a ScratchRegisterAllocator::PreservedState, we can make this less error prone by only passing restoreReusedRegistersByPopping() the appropriate PreservedState that its matching preserveReusedRegistersByPushing() returned.

[Mark Lam]

Created attachment 267394 [details]
proposed patch.

[Geoffrey Garen]

Patch looks good.

I think we usually say "Saved", which is slightly shorter than "Preserved."

EWS is angry, so r-:

/Volumes/Data/EWS/WebKit/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/PolymorphicAccess.h:36:10: fatal error: 'ScratchRegisterAllocator.h' file not found
#include "ScratchRegisterAllocator.h"

[Mark Lam]

(In reply to comment #2)
> I think we usually say "Saved", which is slightly shorter than "Preserved."

Talked with Geoff offline.  The function that generates thus artifact is named preserveReusedRegistersByPushing().  It would be a mismatch to name the artifact SavedState while keeping the function as is.  There are also several other preserve...() functions in ScratchRegisterAllocator unrelated to the patch here.

In light of that, for now, we'll stick with calling it PreservedState.

> EWS is angry, so r-:

Will fix.

[Mark Lam]

Created attachment 267404 [details]
Patch with build fix.

[Geoffrey Garen]

Comment on attachment 267404 [details]
Patch with build fix.

r=me

EWS looks angry for unrelated reasons, so you'll need to land manually.

[Mark Lam]

Thanks for the review.  Landed in r194126: <http://trac.webkit.org/r194126>.

[Saam Barati]

Comment on attachment 267404 [details]
Patch with build fix.

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

> Source/JavaScriptCore/jit/ScratchRegisterAllocator.h:74
> +        PreservedState()
> +            : PreservedState(0)
> +        { }

Since we have a default empty constructor I think it's worth having an operator bool indicating that a PreservedState is valid or not.
We could just make the invalid state have UINT_MAX for preserved bytes.
We can assert inside restore / preserve reusedRegisters that we have a valid state.

[Saam Barati]

(In reply to comment #7)
> Comment on attachment 267404 [details]
> Patch with build fix.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267404&action=review
> 
> > Source/JavaScriptCore/jit/ScratchRegisterAllocator.h:74
> > +        PreservedState()
> > +            : PreservedState(0)
> > +        { }
> 
> Since we have a default empty constructor I think it's worth having an
> operator bool indicating that a PreservedState is valid or not.
> We could just make the invalid state have UINT_MAX for preserved bytes.
> We can assert inside restore / preserve reusedRegisters that we have a valid
> state.

I'll write a patch.