Bug 148345 – jsc-tailcall: Specialized thunks need to save / restore callee save "tag" registers

• Component: JavaScriptCore
• Status: RESOLVED DUPLICATE

[Michael Saboff]

The specialized thunks, like charAt use the tagTypeNumberRegister and/or tagMaskRegister.  These are both callee saves, but the thunks assume that they have the special values.  If the thunk is called by an FTL generated function, those registers do not have the right value.  At the beginning of these thunks, the current values need to be saved and the tag values materialized.  The values need to be restores before returning from the thunk.

[Michael Saboff]

Created attachment 259731 [details]
Patch

[Basile Clement]

Comment on attachment 259731 [details]
Patch

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

LGTM

> Source/JavaScriptCore/jit/SpecializedThunkJIT.h:196
> +        void emitSaveTagCalleeSavesAndMaterializeTagRegisters()

Nit: Maybe it's just because English is not my native language, but this name was confusing to me as I read it as "emit save, tag callee saves, and materialize tag registers". Maybe something like "emitSaveThenMaterializeTagRegisters" would be clearer, I don't know.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:227
> +    else if (entryType == EnterViaJump) {

Just curious, would it be hard to do the register restoration before the jump instead? It would probably be a less contrived dependency (and wouldn't prevent jumping from places that didn't save the tag registers).

[Michael Saboff]

(In reply to comment #2)
> Comment on attachment 259731 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259731&action=review
> 
> LGTM
> 
> > Source/JavaScriptCore/jit/SpecializedThunkJIT.h:196
> > +        void emitSaveTagCalleeSavesAndMaterializeTagRegisters()
> 
> Nit: Maybe it's just because English is not my native language, but this
> name was confusing to me as I read it as "emit save, tag callee saves, and
> materialize tag registers". Maybe something like
> "emitSaveThenMaterializeTagRegisters" would be clearer, I don't know.

I'll rename this and the other function to emitSaveThenMaterializeTagRegisters() and emitRestoreSavedTagRegisters().

> > Source/JavaScriptCore/jit/ThunkGenerators.cpp:227
> > +    else if (entryType == EnterViaJump) {
> 
> Just curious, would it be hard to do the register restoration before the
> jump instead? It would probably be a less contrived dependency (and wouldn't
> prevent jumping from places that didn't save the tag registers).

The problem is that most of the places where we tailCall (EnterViaJump) to this thunk are from conditional jumps.  We could create some other common landing place to jump to, restore the callee saves and then jump here, but the specialized thunks are the only place that uses the EnterViaJump path.

[Michael Saboff]

Committed r188889: <http://trac.webkit.org/changeset/188889>

[Basile Clement]

*** This bug has been marked as a duplicate of bug 148666 ***

[Csaba Osztrogonác]

Comment on attachment 259731 [details]
Patch

Cleared review? from attachment 259731 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).