148345 2015-08-21 17:09:49 -0700 jsc-tailcall: Specialized thunks need to save / restore callee save "tag" registers 2015-09-14 11:00:56 -0700 1 1 1 Unclassified WebKit JavaScriptCore Other All All RESOLVED DUPLICATE 148666 P2 Normal --- 148099 1 msaboff msaboff basile_clement oldest_to_newest 1120008 0 msaboff 2015-08-21 17:09:49 -0700 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. 1120154 1 259731 msaboff 2015-08-22 20:43:07 -0700 Created attachment 259731 Patch 1120301 2 259731 basile_clement 2015-08-24 10:51:36 -0700 Comment on attachment 259731 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). 1120380 3 msaboff 2015-08-24 14:33:06 -0700 (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. 1120426 4 msaboff 2015-08-24 15:40:57 -0700 Committed r188889: <http://trac.webkit.org/changeset/188889> 1122418 5 basile_clement 2015-08-31 18:10:46 -0700 *** This bug has been marked as a duplicate of bug 148666 *** 1125652 6 259731 ossy 2015-09-14 11:00:56 -0700 Comment on attachment 259731 Patch Cleared review? from attachment 259731 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). 259731 2015-08-22 20:43:07 -0700 2015-09-14 11:00:56 -0700 Patch 148345.patch text/plain 5925 msaboff SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTg4ODMyKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDMzIEBA CisyMDE1LTA4LTIyICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAg ICAgIGpzYy10YWlsY2FsbDogU3BlY2lhbGl6ZWQgdGh1bmtzIG5lZWQgdG8gc2F2ZSAvIHJlc3Rv cmUgY2FsbGVlIHNhdmUgInRhZyIgcmVnaXN0ZXJzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNDgzNDUKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBTaW5jZSB0aGVzZSB0aHVua3MgY2FuIGJlIGNhbGxlZCBi eSBGVEwgY29tcGlsZWQgZnVuY3Rpb25zLCB3ZSBjYW4ndCBjb3VudCBvbiB0aGUgCisgICAgICAg IHRhZ1R5cGVOdW1iZXJSZWdpc3RlciBhbmQgdGFnTWFza1JlZ2lzdGVyLiAgSW4gZmFjdCwgdGhl IHNhbWUgcmVnaXN0ZXJzIGNvdWxkIGJlIHVzZWQKKyAgICAgICAgZm9yIG90aGVyIHB1cnBvc2Vz LiAgU2luY2UgdGhleSBhcmUgY2FsbGVlIHNhdmVzLCBzYXZlIHRoZSBjdXJyZW50IHZhbHVlcyBv biBlbnRyeSB0bworICAgICAgICB0aGVzZSB0aHVua3MgYW5kIG1hdGVyaWFsaXplIHRoZSB0YWcg Y29uc3RhbnQgdmFsdWVzLiAgT24gZnVuY3Rpb24gZXhpdCB3ZSByZXN0b3JlCisgICAgICAgIHRo ZSBjYWxsZXIncyB2YWx1ZXMuCisKKyAgICAgICAgKiBqaXQvU3BlY2lhbGl6ZWRUaHVua0pJVC5o OgorICAgICAgICAoSlNDOjpTcGVjaWFsaXplZFRodW5rSklUOjpTcGVjaWFsaXplZFRodW5rSklU KToKKyAgICAgICAgKEpTQzo6U3BlY2lhbGl6ZWRUaHVua0pJVDo6bG9hZERvdWJsZUFyZ3VtZW50 KToKKyAgICAgICAgKEpTQzo6U3BlY2lhbGl6ZWRUaHVua0pJVDo6cmV0dXJuSlNWYWx1ZSk6Cisg ICAgICAgIChKU0M6OlNwZWNpYWxpemVkVGh1bmtKSVQ6OnJldHVybkRvdWJsZSk6CisgICAgICAg IChKU0M6OlNwZWNpYWxpemVkVGh1bmtKSVQ6OnJldHVybkludDMyKToKKyAgICAgICAgKEpTQzo6 U3BlY2lhbGl6ZWRUaHVua0pJVDo6cmV0dXJuSlNDZWxsKToKKyAgICAgICAgKEpTQzo6U3BlY2lh bGl6ZWRUaHVua0pJVDo6Y2FsbERvdWJsZVRvRG91YmxlUHJlc2VydmluZ1JldHVybik6CisgICAg ICAgIChKU0M6OlNwZWNpYWxpemVkVGh1bmtKSVQ6OmVtaXRTYXZlVGFnQ2FsbGVlU2F2ZXNBbmRN YXRlcmlhbGl6ZVRhZ1JlZ2lzdGVycyk6CisgICAgICAgIChKU0M6OlNwZWNpYWxpemVkVGh1bmtK SVQ6OmVtaXRSZXN0b3JlUHJpb3JUYWdDYWxsZWVTYXZlcyk6CisgICAgICAgIChKU0M6OlNwZWNp YWxpemVkVGh1bmtKSVQ6OnRhZ1JldHVybkFzSW50MzIpOgorICAgICAgICAqIGppdC9UaHVua0dl bmVyYXRvcnMuY3BwOgorICAgICAgICAoSlNDOjpuYXRpdmVGb3JHZW5lcmF0b3IpOiAgU2luY2Ug dGhlIG5hdGl2ZUZvckdlbmVyYXRvciBjYW4gYmUgY2FsbGVkIHZpYSBhIHRhaWwgY2FsbAorICAg ICAgICBieSB2YXJpb3VzIHNwZWNpYWxpemVkIHRodW5rcywgd2UgbmVlZCB0byByZXN0b3JlIHRo ZSB2YWx1ZXMgb2YgdGhlIHRhZyBjb25zdGFudCBjYWxsZWUKKyAgICAgICAgc2F2ZSByZWdpc3Rl cnMuICBUaGUgc3BlY2lhbGl6ZWQgdGh1bmtzIGlzIHRoZSBvbmx5IGNvZGUgdGhhdCBtYWtlIHN1 Y2ggdGFpbCBjYWxscworICAgICAgICB0byBuYXRpdmVGb3JHZW5lcmF0b3IgZ2VuZXJhdGVkIGNv ZGUuCisKIDIwMTUtMDgtMjEgIEJhc2lsZSBDbGVtZW50ICA8YmFzaWxlX2NsZW1lbnRAYXBwbGUu Y29tPgogCiAgICAgICAgIGpzYy10YWlsY2FsbDogV2Ugc2hvdWxkIG5vdCBhc3N1bWUgdGhhdCBv bmx5IHRoZSBiYXNlbGluZSBKSVQgY2FuIGhhdmUgQnlJZCBhY2Nlc3NlcwpJbmRleDogU291cmNl L0phdmFTY3JpcHRDb3JlL2ppdC9TcGVjaWFsaXplZFRodW5rSklULmgKPT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0g U291cmNlL0phdmFTY3JpcHRDb3JlL2ppdC9TcGVjaWFsaXplZFRodW5rSklULmgJKHJldmlzaW9u IDE4ODgwNikKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9qaXQvU3BlY2lhbGl6ZWRUaHVua0pJ VC5oCSh3b3JraW5nIGNvcHkpCkBAIC00NCw2ICs0NCw3IEBAIG5hbWVzcGFjZSBKU0MgewogICAg ICAgICAgICAgOiBKU0ludGVyZmFjZUpJVCh2bSkKICAgICAgICAgewogICAgICAgICAgICAgZW1p dEZ1bmN0aW9uUHJvbG9ndWUoKTsKKyAgICAgICAgICAgIGVtaXRTYXZlVGFnQ2FsbGVlU2F2ZXNB bmRNYXRlcmlhbGl6ZVRhZ1JlZ2lzdGVycygpOwogICAgICAgICAgICAgLy8gQ2hlY2sgdGhhdCB3 ZSBoYXZlIHRoZSBleHBlY3RlZCBudW1iZXIgb2YgYXJndW1lbnRzCiAgICAgICAgICAgICBtX2Zh aWx1cmVzLmFwcGVuZChicmFuY2gzMihOb3RFcXVhbCwgcGF5bG9hZEZvcihKU1N0YWNrOjpBcmd1 bWVudENvdW50KSwgVHJ1c3RlZEltbTMyKGV4cGVjdGVkQXJnQ291bnQgKyAxKSkpOwogICAgICAg ICB9CkBAIC01Miw2ICs1Myw3IEBAIG5hbWVzcGFjZSBKU0MgewogICAgICAgICAgICAgOiBKU0lu dGVyZmFjZUpJVCh2bSkKICAgICAgICAgewogICAgICAgICAgICAgZW1pdEZ1bmN0aW9uUHJvbG9n dWUoKTsKKyAgICAgICAgICAgIGVtaXRTYXZlVGFnQ2FsbGVlU2F2ZXNBbmRNYXRlcmlhbGl6ZVRh Z1JlZ2lzdGVycygpOwogICAgICAgICB9CiAgICAgICAgIAogICAgICAgICB2b2lkIGxvYWREb3Vi bGVBcmd1bWVudChpbnQgYXJndW1lbnQsIEZQUmVnaXN0ZXJJRCBkc3QsIFJlZ2lzdGVySUQgc2Ny YXRjaCkKQEAgLTEwNSw2ICsxMDcsOCBAQCBuYW1lc3BhY2UgSlNDIHsKICAgICAgICAgewogICAg ICAgICAgICAgaWYgKHNyYyAhPSByZWdUMCkKICAgICAgICAgICAgICAgICBtb3ZlKHNyYywgcmVn VDApOworICAgICAgICAgICAgCisgICAgICAgICAgICBlbWl0UmVzdG9yZVByaW9yVGFnQ2FsbGVl U2F2ZXMoKTsKICAgICAgICAgICAgIGVtaXRGdW5jdGlvbkVwaWxvZ3VlKCk7CiAgICAgICAgICAg ICByZXQoKTsKICAgICAgICAgfQpAQCAtMTEzLDYgKzExNyw3IEBAIG5hbWVzcGFjZSBKU0Mgewog ICAgICAgICB7CiAgICAgICAgICAgICBBU1NFUlRfVU5VU0VEKHBheWxvYWQsIHBheWxvYWQgPT0g cmVnVDApOwogICAgICAgICAgICAgQVNTRVJUX1VOVVNFRCh0YWcsIHRhZyA9PSByZWdUMSk7Cisg ICAgICAgICAgICBlbWl0UmVzdG9yZVByaW9yVGFnQ2FsbGVlU2F2ZXMoKTsKICAgICAgICAgICAg IGVtaXRGdW5jdGlvbkVwaWxvZ3VlKCk7CiAgICAgICAgICAgICByZXQoKTsKICAgICAgICAgfQpA QCAtMTM3LDYgKzE0Miw3IEBAIG5hbWVzcGFjZSBKU0MgewogICAgICAgICAgICAgbG93Tm9uWmVy by5saW5rKHRoaXMpOwogICAgICAgICAgICAgaGlnaE5vblplcm8ubGluayh0aGlzKTsKICNlbmRp ZgorICAgICAgICAgICAgZW1pdFJlc3RvcmVQcmlvclRhZ0NhbGxlZVNhdmVzKCk7CiAgICAgICAg ICAgICBlbWl0RnVuY3Rpb25FcGlsb2d1ZSgpOwogICAgICAgICAgICAgcmV0KCk7CiAgICAgICAg IH0KQEAgLTE0Niw2ICsxNTIsNyBAQCBuYW1lc3BhY2UgSlNDIHsKICAgICAgICAgICAgIGlmIChz cmMgIT0gcmVnVDApCiAgICAgICAgICAgICAgICAgbW92ZShzcmMsIHJlZ1QwKTsKICAgICAgICAg ICAgIHRhZ1JldHVybkFzSW50MzIoKTsKKyAgICAgICAgICAgIGVtaXRSZXN0b3JlUHJpb3JUYWdD YWxsZWVTYXZlcygpOwogICAgICAgICAgICAgZW1pdEZ1bmN0aW9uRXBpbG9ndWUoKTsKICAgICAg ICAgICAgIHJldCgpOwogICAgICAgICB9CkBAIC0xNTUsNiArMTYyLDcgQEAgbmFtZXNwYWNlIEpT QyB7CiAgICAgICAgICAgICBpZiAoc3JjICE9IHJlZ1QwKQogICAgICAgICAgICAgICAgIG1vdmUo c3JjLCByZWdUMCk7CiAgICAgICAgICAgICB0YWdSZXR1cm5Bc0pTQ2VsbCgpOworICAgICAgICAg ICAgZW1pdFJlc3RvcmVQcmlvclRhZ0NhbGxlZVNhdmVzKCk7CiAgICAgICAgICAgICBlbWl0RnVu Y3Rpb25FcGlsb2d1ZSgpOwogICAgICAgICAgICAgcmV0KCk7CiAgICAgICAgIH0KQEAgLTE4NSw3 ICsxOTMsMzEgQEAgbmFtZXNwYWNlIEpTQyB7CiAgICAgICAgIH0KIAogICAgIHByaXZhdGU6Cisg ICAgICAgIHZvaWQgZW1pdFNhdmVUYWdDYWxsZWVTYXZlc0FuZE1hdGVyaWFsaXplVGFnUmVnaXN0 ZXJzKCkKKyAgICAgICAgeworI2lmIFVTRShKU1ZBTFVFNjQpCisjaWYgQ1BVKEFSTTY0KQorICAg ICAgICAgICAgcHVzaFBhaXIodGFnVHlwZU51bWJlclJlZ2lzdGVyLCB0YWdNYXNrUmVnaXN0ZXIp OworI2Vsc2UKKyAgICAgICAgICAgIHB1c2godGFnVHlwZU51bWJlclJlZ2lzdGVyKTsKKyAgICAg ICAgICAgIHB1c2godGFnTWFza1JlZ2lzdGVyKTsKKyNlbmRpZgorICAgICAgICAgICAgZW1pdE1h dGVyaWFsaXplVGFnQ2hlY2tSZWdpc3RlcnMoKTsKKyNlbmRpZgorICAgICAgICB9CiAKKyAgICAg ICAgdm9pZCBlbWl0UmVzdG9yZVByaW9yVGFnQ2FsbGVlU2F2ZXMoKQorICAgICAgICB7CisjaWYg VVNFKEpTVkFMVUU2NCkKKyNpZiBDUFUoQVJNNjQpCisgICAgICAgICAgICBwb3BQYWlyKHRhZ1R5 cGVOdW1iZXJSZWdpc3RlciwgdGFnTWFza1JlZ2lzdGVyKTsKKyNlbHNlCisgICAgICAgICAgICBw b3AodGFnTWFza1JlZ2lzdGVyKTsKKyAgICAgICAgICAgIHBvcCh0YWdUeXBlTnVtYmVyUmVnaXN0 ZXIpOworI2VuZGlmCisjZW5kaWYKKyAgICAgICAgfQorICAgICAgICAKICAgICAgICAgdm9pZCB0 YWdSZXR1cm5Bc0ludDMyKCkKICAgICAgICAgewogI2lmIFVTRShKU1ZBTFVFNjQpCkluZGV4OiBT b3VyY2UvSmF2YVNjcmlwdENvcmUvaml0L1RodW5rR2VuZXJhdG9ycy5jcHAKPT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQot LS0gU291cmNlL0phdmFTY3JpcHRDb3JlL2ppdC9UaHVua0dlbmVyYXRvcnMuY3BwCShyZXZpc2lv biAxODg4MDYpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaml0L1RodW5rR2VuZXJhdG9ycy5j cHAJKHdvcmtpbmcgY29weSkKQEAgLTIyMyw2ICsyMjMsMTggQEAgc3RhdGljIE1hY3JvQXNzZW1i bGVyQ29kZVJlZiBuYXRpdmVGb3JHZQogCiAgICAgaWYgKGVudHJ5VHlwZSA9PSBFbnRlclZpYUNh bGwpCiAgICAgICAgIGppdC5lbWl0RnVuY3Rpb25Qcm9sb2d1ZSgpOworI2lmIFVTRShKU1ZBTFVF NjQpCisgICAgZWxzZSBpZiAoZW50cnlUeXBlID09IEVudGVyVmlhSnVtcCkgeworICAgICAgICAv LyBXZSdyZSBjb21pbmcgZnJvbSBhIHNwZWNpYWxpemVkIHRodW5rIHRoYXQgaGFzIHNhdmVkIHRo ZSBwcmlvciB0YWcgcmVnaXN0ZXJzJyBjb250ZW50cy4KKyAgICAgICAgLy8gUmVzdG9yZSB0aGVt IG5vdy4KKyNpZiBDUFUoQVJNNjQpCisgICAgICAgIGppdC5wb3BQYWlyKEpTSW50ZXJmYWNlSklU Ojp0YWdUeXBlTnVtYmVyUmVnaXN0ZXIsIEpTSW50ZXJmYWNlSklUOjp0YWdNYXNrUmVnaXN0ZXIp OworI2Vsc2UKKyAgICAgICAgaml0LnBvcChKU0ludGVyZmFjZUpJVDo6dGFnTWFza1JlZ2lzdGVy KTsKKyAgICAgICAgaml0LnBvcChKU0ludGVyZmFjZUpJVDo6dGFnVHlwZU51bWJlclJlZ2lzdGVy KTsKKyNlbmRpZgorICAgIH0KKyNlbmRpZgogCiAgICAgaml0LmVtaXRQdXRJbW1lZGlhdGVUb0Nh bGxGcmFtZUhlYWRlcigwLCBKU1N0YWNrOjpDb2RlQmxvY2spOwogICAgIGppdC5zdG9yZVB0cihK U0ludGVyZmFjZUpJVDo6Y2FsbEZyYW1lUmVnaXN0ZXIsICZ2bS0+dG9wQ2FsbEZyYW1lKTsK