129314 2014-02-25 09:36:05 -0800 JIT Engines use the wrong stack limit for stack checks 2014-02-25 10:50:40 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 msaboff msaboff ggaren mark.lam oldest_to_newest 984354 0 msaboff 2014-02-25 09:36:05 -0800 The VM has a few stack limit values, m_stackLimit for assembly LLInt, baseline and DFG limit checks, m_jsStackLimit for C Loop LLInt checks and m_ftlStackLimit for FTL limit checks. When we compile using the assembly LLInt, m_stackLimit and m_jsStackLimit are declared in a union and therefore are aliases. When compiling with the C Loop LLInt, they are separate values. The LLInt, baseline and DFG all refer to m_jsStackLimit. This is fine for the LLInt as it's behavior will change depending on how it is compiled while the baseline and DFG engines should always use m_stackLimit. 984358 1 225158 msaboff 2014-02-25 09:38:12 -0800 Created attachment 225158 Patch 984370 2 msaboff 2014-02-25 09:48:15 -0800 Committed r164653: <http://trac.webkit.org/changeset/164653> 984380 3 225158 mark.lam 2014-02-25 10:12:45 -0800 Comment on attachment 225158 Patch Why is this an issue at all? m_jsStackLimit and m_stackLimit should be the same field for JIT builds because they are in a union. Are the compilers actually storing them in different fields? My concern is that your fix is not actually doing what you think it is. 984390 4 ggaren 2014-02-25 10:22:27 -0800 Yeah, this looks wrong. What bug does this fix? 984391 5 msaboff 2014-02-25 10:28:29 -0800 (In reply to comment #4) > Yeah, this looks wrong. What bug does this fix? The doesn't change correctness, in fact it shouldn't change anything at the machine code level. This is primarily a fix for clarity / readability and eliminating future problems if we change it so that m_stackLimit and m_jsStackLimit are not aliases. We set m_stackLimit for ASM LLInt + JIT + DFG builds, but we check using m_jsStackLimit. Not the setting and use is consistent. 984392 6 msaboff 2014-02-25 10:33:20 -0800 (In reply to comment #5) > (In reply to comment #4) > .... Not the setting and use is consistent. Now the setting and use are consistent. 984393 7 ggaren 2014-02-25 10:35:03 -0800 I think this patch reduced future flexibility instead of increasing it because it introduced a hard dependency that JIT code must run on the machine stack. The point of m_jsStackLimit was to abstract out where the JS stack lived. All JS code checks m_jsStackLimit, and it's the VM that decides where m_jsStackLimit points. In other words, if we change it so that m_stackLimit and m_jsStackLimit are not aliases, it will be a bug that the JIT checks m_stackLimit instead of m_jsStackLimit. 984394 8 mark.lam 2014-02-25 10:37:40 -0800 (In reply to comment #7) > I think this patch reduced future flexibility instead of increasing it because it introduced a hard dependency that JIT code must run on the machine stack. The point of m_jsStackLimit was to abstract out where the JS stack lived. All JS code checks m_jsStackLimit, and it's the VM that decides where m_jsStackLimit points. > > In other words, if we change it so that m_stackLimit and m_jsStackLimit are not aliases, it will be a bug that the JIT checks m_stackLimit instead of m_jsStackLimit. I agree. Note: the LLINT still checks m_jsStackLimit by necessity (for C loop LLINT compatibility). Hence, changing only the JIT side to use m_stackLimit is no clearer than it was before. 984397 9 msaboff 2014-02-25 10:50:40 -0800 (In reply to comment #7) > I think this patch reduced future flexibility instead of increasing it because it introduced a hard dependency that JIT code must run on the machine stack. The point of m_jsStackLimit was to abstract out where the JS stack lived. All JS code checks m_jsStackLimit, and it's the VM that decides where m_jsStackLimit points. We only set m_jsStackLimit when compiling for the C Loop. The rest of the code sets m_stackLimit. If we roll out this patch then we should also change all the occurrences of setting m_stackLimit to set m_sjStackLimit. I believe there are other places the code would need to change in order to use a stack besides the machine stack. > In other words, if we change it so that m_stackLimit and m_jsStackLimit are not aliases, it will be a bug that the JIT checks m_stackLimit instead of m_jsStackLimit. Concerning the LLInt using m_jsStackLimit, that was covered in the description. It needs to switch based on how it is built. 225158 2014-02-25 09:38:12 -0800 2014-02-25 10:12:45 -0800 Patch 129314.patch text/plain 4896 msaboff SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTY0NjUwKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIzIEBA CisyMDE0LTAyLTI1ICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAg ICAgIEpJVCBFbmdpbmVzIHVzZSB0aGUgd3Jvbmcgc3RhY2sgbGltaXQgZm9yIHN0YWNrIGNoZWNr cworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTI5MzE0 CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQ2hhbmdl IHRoZSBCYXNlbGluZSBhbmQgREZHIGNvZGUgdG8gdXNlIFZNOjptX3N0YWNrTGltaXQgZm9yIHN0 YWNrIGxpbWl0IGNoZWNrcy4KKworICAgICAgICAqIGRmZy9ERkdKSVRDb21waWxlci5jcHA6Cisg ICAgICAgIChKU0M6OkRGRzo6SklUQ29tcGlsZXI6OmNvbXBpbGVGdW5jdGlvbik6CisgICAgICAg ICogaml0L0pJVC5jcHA6CisgICAgICAgIChKU0M6OkpJVDo6cHJpdmF0ZUNvbXBpbGUpOgorICAg ICAgICAqIGppdC9KSVRDYWxsLmNwcDoKKyAgICAgICAgKEpTQzo6SklUOjpjb21waWxlTG9hZFZh cmFyZ3MpOgorICAgICAgICAqIGppdC9KSVRDYWxsMzJfNjQuY3BwOgorICAgICAgICAoSlNDOjpK SVQ6OmNvbXBpbGVMb2FkVmFyYXJncyk6CisgICAgICAgICogcnVudGltZS9WTS5oOgorICAgICAg ICAoSlNDOjpWTTo6YWRkcmVzc09mU3RhY2tMaW1pdCk6CisKIDIwMTQtMDItMjQgIE9saXZlciBI dW50ICA8b2xpdmVyQGFwcGxlLmNvbT4KIAogICAgICAgICBGaXggYnVpbGQuCkluZGV4OiBTb3Vy Y2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0pJVENvbXBpbGVyLmNwcAo9PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBT b3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0pJVENvbXBpbGVyLmNwcAkocmV2aXNpb24gMTY0 NjUwKQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdKSVRDb21waWxlci5jcHAJKHdv cmtpbmcgY29weSkKQEAgLTMzNiw3ICszMzYsNyBAQCB2b2lkIEpJVENvbXBpbGVyOjpjb21waWxl RnVuY3Rpb24oKQogICAgIExhYmVsIGZyb21Bcml0eUNoZWNrKHRoaXMpOwogICAgIC8vIFBsYW50 IGEgY2hlY2sgdGhhdCBzdWZmaWNpZW50IHNwYWNlIGlzIGF2YWlsYWJsZSBpbiB0aGUgSlNTdGFj ay4KICAgICBhZGRQdHIoVHJ1c3RlZEltbTMyKHZpcnR1YWxSZWdpc3RlckZvckxvY2FsKG1fZ3Jh cGgucmVxdWlyZWRSZWdpc3RlckNvdW50Rm9yRXhlY3V0aW9uQW5kRXhpdCgpIC0gMSkub2Zmc2V0 KCkgKiBzaXplb2YoUmVnaXN0ZXIpKSwgR1BSSW5mbzo6Y2FsbEZyYW1lUmVnaXN0ZXIsIEdQUklu Zm86OnJlZ1QxKTsKLSAgICBKdW1wIHN0YWNrT3ZlcmZsb3cgPSBicmFuY2hQdHIoQWJvdmUsIEFi c29sdXRlQWRkcmVzcyhtX3ZtLT5hZGRyZXNzT2ZKU1N0YWNrTGltaXQoKSksIEdQUkluZm86OnJl Z1QxKTsKKyAgICBKdW1wIHN0YWNrT3ZlcmZsb3cgPSBicmFuY2hQdHIoQWJvdmUsIEFic29sdXRl QWRkcmVzcyhtX3ZtLT5hZGRyZXNzT2ZTdGFja0xpbWl0KCkpLCBHUFJJbmZvOjpyZWdUMSk7CiAK ICAgICAvLyBNb3ZlIHRoZSBzdGFjayBwb2ludGVyIGRvd24gdG8gYWNjb21tb2RhdGUgbG9jYWxz CiAgICAgYWRkUHRyKFRydXN0ZWRJbW0zMihtX2dyYXBoLnN0YWNrUG9pbnRlck9mZnNldCgpICog c2l6ZW9mKFJlZ2lzdGVyKSksIEdQUkluZm86OmNhbGxGcmFtZVJlZ2lzdGVyLCBzdGFja1BvaW50 ZXJSZWdpc3Rlcik7CkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaml0L0pJVC5jcHAKPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL2ppdC9KSVQuY3BwCShyZXZpc2lvbiAx NjQ2NTApCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaml0L0pJVC5jcHAJKHdvcmtpbmcgY29w eSkKQEAgLTUxOSw3ICs1MTksNyBAQCBDb21waWxhdGlvblJlc3VsdCBKSVQ6OnByaXZhdGVDb21w aWxlKEpJCiAgICAgICAgIH0KIAogICAgICAgICBhZGRQdHIoVHJ1c3RlZEltbTMyKHN0YWNrUG9p bnRlck9mZnNldEZvcihtX2NvZGVCbG9jaykgKiBzaXplb2YoUmVnaXN0ZXIpKSwgY2FsbEZyYW1l UmVnaXN0ZXIsIHJlZ1QxKTsKLSAgICAgICAgc3RhY2tPdmVyZmxvdyA9IGJyYW5jaFB0cihBYm92 ZSwgQWJzb2x1dGVBZGRyZXNzKG1fdm0tPmFkZHJlc3NPZkpTU3RhY2tMaW1pdCgpKSwgcmVnVDEp OworICAgICAgICBzdGFja092ZXJmbG93ID0gYnJhbmNoUHRyKEFib3ZlLCBBYnNvbHV0ZUFkZHJl c3MobV92bS0+YWRkcmVzc09mU3RhY2tMaW1pdCgpKSwgcmVnVDEpOwogICAgIH0KIAogICAgIGFk ZFB0cihUcnVzdGVkSW1tMzIoc3RhY2tQb2ludGVyT2Zmc2V0Rm9yKG1fY29kZUJsb2NrKSAqIHNp emVvZihSZWdpc3RlcikpLCBjYWxsRnJhbWVSZWdpc3Rlciwgc3RhY2tQb2ludGVyUmVnaXN0ZXIp OwpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL2ppdC9KSVRDYWxsMzJfNjQuY3BwCj09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9qaXQvSklUQ2FsbDMyXzY0LmNwcAkocmV2 aXNpb24gMTY0NjUwKQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL2ppdC9KSVRDYWxsMzJfNjQu Y3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xNjAsNyArMTYwLDcgQEAgdm9pZCBKSVQ6OmNvbXBpbGVM b2FkVmFyYXJncyhJbnN0cnVjdGlvbgogICAgICAgICBhZGRQdHIoY2FsbEZyYW1lUmVnaXN0ZXIs IHJlZ1QzKTsKICAgICAgICAgLy8gcmVnVDM6IG5ld0NhbGxGcmFtZQogCi0gICAgICAgIHNsb3dD YXNlLmFwcGVuZChicmFuY2hQdHIoQWJvdmUsIEFic29sdXRlQWRkcmVzcyhtX3ZtLT5hZGRyZXNz T2ZKU1N0YWNrTGltaXQoKSksIHJlZ1QzKSk7CisgICAgICAgIHNsb3dDYXNlLmFwcGVuZChicmFu Y2hQdHIoQWJvdmUsIEFic29sdXRlQWRkcmVzcyhtX3ZtLT5hZGRyZXNzT2ZTdGFja0xpbWl0KCkp LCByZWdUMykpOwogCiAgICAgICAgIC8vIEluaXRpYWxpemUgQXJndW1lbnRDb3VudC4KICAgICAg ICAgc3RvcmUzMihyZWdUMiwgcGF5bG9hZEZvcihKU1N0YWNrOjpBcmd1bWVudENvdW50LCByZWdU MykpOwpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL2ppdC9KSVRDYWxsLmNwcAo9PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaml0L0pJVENhbGwuY3BwCShyZXZpc2lvbiAx NjQ2NTApCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaml0L0pJVENhbGwuY3BwCSh3b3JraW5n IGNvcHkpCkBAIC04Nyw3ICs4Nyw3IEBAIHZvaWQgSklUOjpjb21waWxlTG9hZFZhcmFyZ3MoSW5z dHJ1Y3Rpb24KICAgICAgICAgYWRkUHRyKGNhbGxGcmFtZVJlZ2lzdGVyLCByZWdUMSk7CiAgICAg ICAgIC8vIHJlZ1QxOiBuZXdDYWxsRnJhbWUKIAotICAgICAgICBzbG93Q2FzZS5hcHBlbmQoYnJh bmNoUHRyKEFib3ZlLCBBYnNvbHV0ZUFkZHJlc3MobV92bS0+YWRkcmVzc09mSlNTdGFja0xpbWl0 KCkpLCByZWdUMSkpOworICAgICAgICBzbG93Q2FzZS5hcHBlbmQoYnJhbmNoUHRyKEFib3ZlLCBB YnNvbHV0ZUFkZHJlc3MobV92bS0+YWRkcmVzc09mU3RhY2tMaW1pdCgpKSwgcmVnVDEpKTsKIAog ICAgICAgICAvLyBJbml0aWFsaXplIEFyZ3VtZW50Q291bnQuCiAgICAgICAgIHN0b3JlMzIocmVn VDAsIEFkZHJlc3MocmVnVDEsIEpTU3RhY2s6OkFyZ3VtZW50Q291bnQgKiBzdGF0aWNfY2FzdDxp bnQ+KHNpemVvZihSZWdpc3RlcikpICsgT0JKRUNUX09GRlNFVE9GKEVuY29kZWRWYWx1ZURlc2Ny aXB0b3IsIGFzQml0cy5wYXlsb2FkKSkpOwpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL3J1 bnRpbWUvVk0uaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9W TS5oCShyZXZpc2lvbiAxNjQ2NTApCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9W TS5oCSh3b3JraW5nIGNvcHkpCkBAIC0zODksMTIgKzM4OSwxMiBAQCBuYW1lc3BhY2UgSlNDIHsK ICAgICAgICAgdm9pZCoqIGFkZHJlc3NPZkZUTFN0YWNrTGltaXQoKSB7IHJldHVybiAmbV9mdGxT dGFja0xpbWl0OyB9CiAjZW5kaWYKIAotICAgICAgICB2b2lkKiogYWRkcmVzc09mSlNTdGFja0xp bWl0KCkgeyByZXR1cm4gJm1fanNTdGFja0xpbWl0OyB9CiAjaWYgRU5BQkxFKExMSU5UX0NfTE9P UCkKICAgICAgICAgdm9pZCoganNTdGFja0xpbWl0KCkgeyByZXR1cm4gbV9qc1N0YWNrTGltaXQ7 IH0KICAgICAgICAgdm9pZCBzZXRKU1N0YWNrTGltaXQodm9pZCogbGltaXQpIHsgbV9qc1N0YWNr TGltaXQgPSBsaW1pdDsgfQogI2VuZGlmCiAgICAgICAgIHZvaWQqIHN0YWNrTGltaXQoKSB7IHJl dHVybiBtX3N0YWNrTGltaXQ7IH0KKyAgICAgICAgdm9pZCoqIGFkZHJlc3NPZlN0YWNrTGltaXQo KSB7IHJldHVybiAmbV9zdGFja0xpbWl0OyB9CiAKICAgICAgICAgYm9vbCBpc1NhZmVUb1JlY3Vy c2Uoc2l6ZV90IG5lZWRlZFN0YWNrSW5CeXRlcyA9IDApIGNvbnN0CiAgICAgICAgIHsK