Bug 148345

Summary: jsc-tailcall: Specialized thunks need to save / restore callee save "tag" registers
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: JavaScriptCoreAssignee: Michael Saboff <msaboff>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: basile_clement
Priority: P2    
Version: Other   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 148099    
Attachments:
Description Flags
Patch none

Michael Saboff
Reported 2015-08-21 17:09:49 PDT
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.
Attachments
Patch (5.79 KB, patch)
2015-08-22 20:43 PDT, Michael Saboff 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Michael Saboff
Comment 1 2015-08-22 20:43:07 PDT
Created attachment 259731 [details] Patch
Basile Clement
Comment 2 2015-08-24 10:51:36 PDT
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
Comment 3 2015-08-24 14:33:06 PDT
(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
Comment 4 2015-08-24 15:40:57 PDT
Committed r188889: <http://trac.webkit.org/changeset/188889>
Basile Clement
Comment 5 2015-08-31 18:10:46 PDT
*** This bug has been marked as a duplicate of bug 148666 ***
Csaba Osztrogonác
Comment 6 2015-09-14 11:00:56 PDT
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).
Note You need to log in before you can comment on or make changes to this bug.