146846 2015-07-10 12:23:26 -0700 jsc-tailcall: JavaScript functions should restore the stack pointer after a call 2015-07-14 14:20:20 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 146484 146847 1 basile_clement basile_clement fpizlo ggaren mark.lam msaboff oldest_to_newest 1108710 0 basile_clement 2015-07-10 12:23:26 -0700 The LLint and baseline JIT are already doing this, we statically know the stack size in the DFG, and we can get it from the LLVM stackmap in FTL code - so this should be an easy change. Once https://bugs.webkit.org/show_bug.cgi?id=146845 is taken care of as well, this will allow us to: 1) Get rid of the arity fixup return thunk (since the caller now knows where the stack pointer should be as an offset of its base pointer) 2) Actually implement tail calls since they can shrink or extend the stack in unpredictable ways. 1109156 1 256709 basile_clement 2015-07-13 11:53:48 -0700 Created attachment 256709 Patch using StackMap size This uses the stackmap size from LLVM to restore the stack pointer to where it should be in the patchpoint. I am interested in hearing opinions on the following two points: - We may want to adapt the default patchpoint size to accommodate the additional lea opcode. - LLVM provides a stackmap/stackrestore intrinsic that we should be able to use. I am unsure what guarantees we would have with this; in particular, I fear LLVM spilling values onto the stack before the call but after saving the stack pointer, performing the call, then trying to restore those values using the stack pointer as it is supposed to be callee-save. I did not see this happen even when testing that approach with high register pressure (LLVM ends up using bp-based offsets for the spilling), but again, I am unsure how much we can rely on this. I believe messing up with the stack pointer in the patchpoint is better because it makes it completely transparent to LLVM. 1109182 2 basile_clement 2015-07-13 12:52:58 -0700 (In reply to comment #1) > Created attachment 256709 [details] > Patch using StackMap size > > This uses the stackmap size from LLVM to restore the stack pointer to where > it should be in the patchpoint. I am interested in hearing opinions on the > following two points: > > - We may want to adapt the default patchpoint size to accommodate the > additional lea opcode. > > - LLVM provides a stackmap/stackrestore intrinsic that we should be able to > use. I am unsure what guarantees we would have with this; in particular, I > fear LLVM spilling values onto the stack before the call but after saving > the stack pointer, performing the call, then trying to restore those values > using the stack pointer as it is supposed to be callee-save. I did not see > this happen even when testing that approach with high register pressure > (LLVM ends up using bp-based offsets for the spilling), but again, I am > unsure how much we can rely on this. I believe messing up with the stack > pointer in the patchpoint is better because it makes it completely > transparent to LLVM. After talking offline with fpizlo, the stackmap size approach looks like the best one. 1109448 3 256709 msaboff 2015-07-14 14:03:51 -0700 Comment on attachment 256709 Patch using StackMap size View in context: https://bugs.webkit.org/attachment.cgi?id=256709&action=review r=me with suggested name change to a function. > Source/JavaScriptCore/ftl/FTLJSCall.cpp:54 > +void JSCall::emit(CCallHelpers& jit, unsigned stackSize) Instead of calling this "emit", how about "emitRestoreStackPointer"? 1109451 4 basile_clement 2015-07-14 14:09:17 -0700 (In reply to comment #3) > Comment on attachment 256709 [details] > Patch using StackMap size > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256709&action=review > > r=me with suggested name change to a function. > > > Source/JavaScriptCore/ftl/FTLJSCall.cpp:54 > > +void JSCall::emit(CCallHelpers& jit, unsigned stackSize) > > Instead of calling this "emit", how about "emitRestoreStackPointer"? FTR, after discussion offline, we'll keep "emit" since this is doing more than just restoring the stack pointer. 1109458 5 basile_clement 2015-07-14 14:20:20 -0700 (In reply to comment #4) > (In reply to comment #3) > > Comment on attachment 256709 [details] > > Patch using StackMap size > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=256709&action=review > > > > r=me with suggested name change to a function. > > > > > Source/JavaScriptCore/ftl/FTLJSCall.cpp:54 > > > +void JSCall::emit(CCallHelpers& jit, unsigned stackSize) > > > > Instead of calling this "emit", how about "emitRestoreStackPointer"? > > FTR, after discussion offline, we'll keep "emit" since this is doing more > than just restoring the stack pointer. Committed r186815 <http://trac.webkit.org/changeset/186815>. 256709 2015-07-13 11:53:48 -0700 2015-07-14 14:03:51 -0700 Patch using StackMap size restorestack.patch text/plain 5256 basile_clement ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IDg2NWZmZTIuLjU2MzkwMmQgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIzIEBACisyMDE1LTA3LTEzICBCYXNpbGUgQ2xlbWVu dCAgPGJhc2lsZV9jbGVtZW50QGFwcGxlLmNvbT4KKworICAgICAgICBKYXZhU2NyaXB0IGZ1bmN0 aW9ucyBzaG91bGQgcmVzdG9yZSB0aGUgc3RhY2sgcG9pbnRlciBhZnRlciBhIGNhbGwKKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE0Njg0NgorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoaXMgcGF0Y2ggbWFr ZXMgaXQgc28gdGhhdCB0aGUgREZHIGFuZCBGVEwgSklUIGFyZSByZXN0b3JpbmcgdGhlIHN0YWNr CisgICAgICAgIHBvaW50ZXIgYWZ0ZXIgYSBjYWxsIGFuZCBubyBsb25nZXIgcmVseWluZyBvbiBp dCBzdGlsbCBiZWluZyB2YWxpZC4KKworICAgICAgICAqIGRmZy9ERkdTcGVjdWxhdGl2ZUpJVDMy XzY0LmNwcDoKKyAgICAgICAgKEpTQzo6REZHOjpTcGVjdWxhdGl2ZUpJVDo6ZW1pdENhbGwpOgor ICAgICAgICAqIGRmZy9ERkdTcGVjdWxhdGl2ZUpJVDY0LmNwcDoKKyAgICAgICAgKEpTQzo6REZH OjpTcGVjdWxhdGl2ZUpJVDo6ZW1pdENhbGwpOgorICAgICAgICAqIGZ0bC9GVExDb21waWxlLmNw cDoKKyAgICAgICAgKEpTQzo6RlRMOjptbUFsbG9jYXRlRGF0YVNlY3Rpb24pOgorICAgICAgICAq IGZ0bC9GVExKU0NhbGwuY3BwOgorICAgICAgICAoSlNDOjpGVEw6OkpTQ2FsbDo6ZW1pdCk6Cisg ICAgICAgICogZnRsL0ZUTEpTQ2FsbC5oOgorCiAyMDE1LTA3LTA5ICBNaWNoYWVsIFNhYm9mZiAg PG1zYWJvZmZAYXBwbGUuY29tPgogCiAgICAgICAgIFJFR1JFU1NJT04gKHIxODAyNDgpOiBSZXBy byBDcmFzaDogY29tLmFwcGxlLldlYktpdC5XZWJDb250ZW50IGF0IGNvbS5hcHBsZS5KYXZhU2Ny aXB0Q29yZTogSlNDOjpjcmVhdGVSYW5nZUVycm9yICsgMjAKZGlmZiAtLWdpdCBhL1NvdXJjZS9K YXZhU2NyaXB0Q29yZS9kZmcvREZHU3BlY3VsYXRpdmVKSVQzMl82NC5jcHAgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvZGZnL0RGR1NwZWN1bGF0aXZlSklUMzJfNjQuY3BwCmluZGV4IDdlMDhkOWIu LjYwNDU4ZDUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHU3BlY3Vs YXRpdmVKSVQzMl82NC5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdTcGVj dWxhdGl2ZUpJVDMyXzY0LmNwcApAQCAtODMyLDkgKzgzMiwxMCBAQCB2b2lkIFNwZWN1bGF0aXZl SklUOjplbWl0Q2FsbChOb2RlKiBub2RlKQogICAgIGluZm8tPnNldFVwQ2FsbChjYWxsVHlwZSwg bm9kZS0+b3JpZ2luLnNlbWFudGljLCBjYWxsZWVQYXlsb2FkR1BSKTsKICAgICBtX2ppdC5hZGRK U0NhbGwoZmFzdENhbGwsIHNsb3dDYWxsLCB0YXJnZXRUb0NoZWNrLCBpbmZvKTsKICAgICAKLSAg ICAvLyBJZiB3ZSB3ZXJlIHZhcmFyZ3MsIHRoZW4gYWZ0ZXIgdGhlIGNhbGxzIGFyZSBkb25lLCB3 ZSBuZWVkIHRvIHJlZXN0YWJsaXNoIG91ciBzdGFjayBwb2ludGVyLgotICAgIGlmIChpc1ZhcmFy Z3MgfHwgaXNGb3J3YXJkVmFyYXJncykKLSAgICAgICAgbV9qaXQuYWRkUHRyKFRydXN0ZWRJbW0z MihtX2ppdC5ncmFwaCgpLnN0YWNrUG9pbnRlck9mZnNldCgpICogc2l6ZW9mKFJlZ2lzdGVyKSks IEdQUkluZm86OmNhbGxGcmFtZVJlZ2lzdGVyLCBKSVRDb21waWxlcjo6c3RhY2tQb2ludGVyUmVn aXN0ZXIpOworICAgIC8vIEFmdGVyIHRoZSBjYWxscyBhcmUgZG9uZSwgd2UgbmVlZCB0byByZWVz dGFibGlzaCBvdXIgc3RhY2sKKyAgICAvLyBwb2ludGVyLiBXZSByZWx5IG9uIHRoaXMgZm9yIHZh cmFyZ3MgY2FsbHMsIGNhbGxzIHdpdGggYXJpdHkKKyAgICAvLyBtaXNtYXRjaCAodGhlIGNhbGxm cmFtZSBpcyBzbGlkZWQpIGFuZCB0YWlsIGNhbGxzLgorICAgIG1faml0LmFkZFB0cihUcnVzdGVk SW1tMzIobV9qaXQuZ3JhcGgoKS5zdGFja1BvaW50ZXJPZmZzZXQoKSAqIHNpemVvZihSZWdpc3Rl cikpLCBHUFJJbmZvOjpjYWxsRnJhbWVSZWdpc3RlciwgSklUQ29tcGlsZXI6OnN0YWNrUG9pbnRl clJlZ2lzdGVyKTsKIH0KIAogdGVtcGxhdGU8Ym9vbCBzdHJpY3Q+CmRpZmYgLS1naXQgYS9Tb3Vy Y2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR1NwZWN1bGF0aXZlSklUNjQuY3BwIGIvU291cmNlL0ph dmFTY3JpcHRDb3JlL2RmZy9ERkdTcGVjdWxhdGl2ZUpJVDY0LmNwcAppbmRleCAyYzc4ZjlhLi43 ZWQ5NTY3IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR1NwZWN1bGF0 aXZlSklUNjQuY3BwCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHU3BlY3VsYXRp dmVKSVQ2NC5jcHAKQEAgLTc5NCwxMCArNzk0LDExIEBAIHZvaWQgU3BlY3VsYXRpdmVKSVQ6OmVt aXRDYWxsKE5vZGUqIG5vZGUpCiAgICAgCiAgICAgY2FsbExpbmtJbmZvLT5zZXRVcENhbGwoY2Fs bFR5cGUsIG1fY3VycmVudE5vZGUtPm9yaWdpbi5zZW1hbnRpYywgIGNhbGxlZUdQUik7ICAgIAog ICAgIG1faml0LmFkZEpTQ2FsbChmYXN0Q2FsbCwgc2xvd0NhbGwsIHRhcmdldFRvQ2hlY2ssIGNh bGxMaW5rSW5mbyk7Ci0gICAgCi0gICAgLy8gSWYgd2Ugd2VyZSB2YXJhcmdzLCB0aGVuIGFmdGVy IHRoZSBjYWxscyBhcmUgZG9uZSwgd2UgbmVlZCB0byByZWVzdGFibGlzaCBvdXIgc3RhY2sgcG9p bnRlci4KLSAgICBpZiAoaXNWYXJhcmdzIHx8IGlzRm9yd2FyZFZhcmFyZ3MpCi0gICAgICAgIG1f aml0LmFkZFB0cihUcnVzdGVkSW1tMzIobV9qaXQuZ3JhcGgoKS5zdGFja1BvaW50ZXJPZmZzZXQo KSAqIHNpemVvZihSZWdpc3RlcikpLCBHUFJJbmZvOjpjYWxsRnJhbWVSZWdpc3RlciwgSklUQ29t cGlsZXI6OnN0YWNrUG9pbnRlclJlZ2lzdGVyKTsKKworICAgIC8vIEFmdGVyIHRoZSBjYWxscyBh cmUgZG9uZSwgd2UgbmVlZCB0byByZWVzdGFibGlzaCBvdXIgc3RhY2sKKyAgICAvLyBwb2ludGVy LiBXZSByZWx5IG9uIHRoaXMgZm9yIHZhcmFyZ3MgY2FsbHMsIGNhbGxzIHdpdGggYXJpdHkKKyAg ICAvLyBtaXNtYXRjaCAodGhlIGNhbGxmcmFtZSBpcyBzbGlkZWQpIGFuZCB0YWlsIGNhbGxzLgor ICAgIG1faml0LmFkZFB0cihUcnVzdGVkSW1tMzIobV9qaXQuZ3JhcGgoKS5zdGFja1BvaW50ZXJP ZmZzZXQoKSAqIHNpemVvZihSZWdpc3RlcikpLCBHUFJJbmZvOjpjYWxsRnJhbWVSZWdpc3Rlciwg SklUQ29tcGlsZXI6OnN0YWNrUG9pbnRlclJlZ2lzdGVyKTsKIH0KIAogLy8gQ2xhbmcgc2hvdWxk IGFsbG93IHVucmVhY2hhYmxlIFtbY2xhbmc6OmZhbGx0aHJvdWdoXV0gaW4gdGVtcGxhdGUgZnVu Y3Rpb25zIGlmIGFueSB0ZW1wbGF0ZSBleHBhbnNpb24gdXNlcyBpdApkaWZmIC0tZ2l0IGEvU291 cmNlL0phdmFTY3JpcHRDb3JlL2Z0bC9GVExDb21waWxlLmNwcCBiL1NvdXJjZS9KYXZhU2NyaXB0 Q29yZS9mdGwvRlRMQ29tcGlsZS5jcHAKaW5kZXggYzZmY2NhMC4uNGU5YTRkOCAxMDA2NDQKLS0t IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2Z0bC9GVExDb21waWxlLmNwcAorKysgYi9Tb3VyY2Uv SmF2YVNjcmlwdENvcmUvZnRsL0ZUTENvbXBpbGUuY3BwCkBAIC01OTAsNyArNTkwLDcgQEAgc3Rh dGljIHZvaWQgZml4RnVuY3Rpb25CYXNlZE9uU3RhY2tNYXBzKAogICAgICAgICBKU0NhbGwmIGNh bGwgPSBzdGF0ZS5qc0NhbGxzW2ldOwogCiAgICAgICAgIENDYWxsSGVscGVycyBmYXN0UGF0aEpJ VCgmdm0sIGNvZGVCbG9jayk7Ci0gICAgICAgIGNhbGwuZW1pdChmYXN0UGF0aEpJVCk7CisgICAg ICAgIGNhbGwuZW1pdChmYXN0UGF0aEpJVCwgc3RhdGUuaml0Q29kZS0+c3RhY2ttYXBzLnN0YWNr U2l6ZSgpKTsKIAogICAgICAgICBjaGFyKiBzdGFydE9mSUMgPSBiaXR3aXNlX2Nhc3Q8Y2hhcio+ KGdlbmVyYXRlZEZ1bmN0aW9uKSArIGNhbGwubV9pbnN0cnVjdGlvbk9mZnNldDsKIApkaWZmIC0t Z2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2Z0bC9GVExKU0NhbGwuY3BwIGIvU291cmNlL0ph dmFTY3JpcHRDb3JlL2Z0bC9GVExKU0NhbGwuY3BwCmluZGV4IGU2ZjZiZGEuLmJkZjQ5OGYgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9mdGwvRlRMSlNDYWxsLmNwcAorKysgYi9T b3VyY2UvSmF2YVNjcmlwdENvcmUvZnRsL0ZUTEpTQ2FsbC5jcHAKQEAgLTUxLDYgKzUxLDE0IEBA IEpTQ2FsbDo6SlNDYWxsKHVuc2lnbmVkIHN0YWNrbWFwSUQsIE5vZGUqIG5vZGUpCiAgICAgQVNT RVJUKG5vZGUtPm9wKCkgPT0gQ2FsbCB8fCBub2RlLT5vcCgpID09IENvbnN0cnVjdCk7CiB9CiAK K3ZvaWQgSlNDYWxsOjplbWl0KENDYWxsSGVscGVycyYgaml0LCB1bnNpZ25lZCBzdGFja1NpemUp Cit7CisgICAgSlNDYWxsQmFzZTo6ZW1pdChqaXQpOworCisgICAgLy8gUmVzdG9yZSB0aGUgc3Rh Y2sgcG9pbnRlcgorICAgIGppdC5hZGRQdHIoQ0NhbGxIZWxwZXJzOjpUcnVzdGVkSW1tMzIoc2l6 ZW9mKFJlZ2lzdGVyKSAtIHN0YXRpY19jYXN0PGludDY0X3Q+KHN0YWNrU2l6ZSkpLCBDQ2FsbEhl bHBlcnM6OmZyYW1lUG9pbnRlclJlZ2lzdGVyLCBDQ2FsbEhlbHBlcnM6OnN0YWNrUG9pbnRlclJl Z2lzdGVyKTsKK30KKwogfSB9IC8vIG5hbWVzcGFjZSBKU0M6OkZUTAogCiAjZW5kaWYgLy8gRU5B QkxFKEZUTF9KSVQpCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvZnRsL0ZUTEpT Q2FsbC5oIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2Z0bC9GVExKU0NhbGwuaAppbmRleCAyZjVl NjRhLi4yMmQzMDk1IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvZnRsL0ZUTEpT Q2FsbC5oCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9mdGwvRlRMSlNDYWxsLmgKQEAgLTQy LDYgKzQyLDggQEAgY2xhc3MgSlNDYWxsIDogcHVibGljIEpTQ2FsbEJhc2UgewogcHVibGljOgog ICAgIEpTQ2FsbCgpOwogICAgIEpTQ2FsbCh1bnNpZ25lZCBzdGFja21hcElELCBERkc6Ok5vZGUq KTsKKworICAgIHZvaWQgZW1pdChDQ2FsbEhlbHBlcnMmLCB1bnNpZ25lZCBzdGFja1NpemUpOwog ICAgIAogICAgIHVuc2lnbmVkIHN0YWNrbWFwSUQoKSBjb25zdCB7IHJldHVybiBtX3N0YWNrbWFw SUQ7IH0KICAgICAK