122332 2013-10-04 11:18:30 -0700 FTL: Crash in OSRExit::convertToForward() using VirtualRegister.offset() as array index 2013-10-04 11:51:34 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 122336 1 msaboff msaboff fpizlo nrotem oldest_to_newest 936259 0 msaboff 2013-10-04 11:18:30 -0700 There are several instances in OSRExit::convertToForward() where we are using the offset() method on a virtual register as an array index. The should be toLocal() calls instead. 936275 1 nrotem 2013-10-04 11:32:49 -0700 Looks Good To Me (LGTM). 936277 2 213381 msaboff 2013-10-04 11:33:56 -0700 Created attachment 213381 Patch 936280 3 msaboff 2013-10-04 11:36:55 -0700 Committed r156900: <http://trac.webkit.org/changeset/156900> 936287 4 213381 fpizlo 2013-10-04 11:44:33 -0700 Comment on attachment 213381 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213381&action=review > Source/JavaScriptCore/ftl/FTLOSRExit.cpp:90 > - if (m_values[overriddenOperand.offset()].isArgument()) { > - ExitArgument exitArgument = m_values[overriddenOperand.offset()].exitArgument(); > + if (m_values[overriddenOperand.toLocal()].isArgument()) { > + ExitArgument exitArgument = m_values[overriddenOperand.toLocal()].exitArgument(); > arguments[exitArgument.argument()] = value.value(); > - m_values[overriddenOperand.offset()] = ExitValue::exitArgument( > + m_values[overriddenOperand.toLocal()] = ExitValue::exitArgument( I think that this is still wrong. m_values is an Operands<>, which can be indexed in two ways: Operands<>::operator[]: this is only meant for iterating over all of the entries, and the index is meaningless. it is neither the offset nor the local - it's nothing. Hence this code was totally wrong to begin with for using m_values[...], and only worked by accident. Operands<>::operand(): this takes a VirtualRegister. There are other indexing modes like Operands<>::local() and Operands<>::argument(), but those are less useful since usually in the DFG and FTL we don't know if something is a local or an argument and we don't really care. In this code, overriddenOperand could be an argument, maybe if you did something like: function foo(x) { x = x | 0 } I don't know if you'll actually get a MovHint into the argument here, but that would be valid IR for this byte code so we should be able to handle it. So, the correct thing to do here is to say: if (m_values.operand(overriddenOperand).isArgument()) { ... And: m_values.operand(overriddenOperand) = ExitValue::exitArgument( ... That way, it'll handle both locals and arguments and there will be no confusion over what to use as indices. 936288 5 msaboff 2013-10-04 11:46:27 -0700 I'll make the changes that Phil suggests and submit a new patch. 936290 6 fpizlo 2013-10-04 11:47:35 -0700 (In reply to comment #5) > I'll make the changes that Phil suggests and submit a new patch. Can you fix it in: https://bugs.webkit.org/show_bug.cgi?id=122336 936291 7 fpizlo 2013-10-04 11:47:50 -0700 (In reply to comment #6) > (In reply to comment #5) > > I'll make the changes that Phil suggests and submit a new patch. > > Can you fix it in: https://bugs.webkit.org/show_bug.cgi?id=122336 (or dup appropriately if you already created your own bug.) 936294 8 msaboff 2013-10-04 11:51:25 -0700 (In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > I'll make the changes that Phil suggests and submit a new patch. > > > > Can you fix it in: https://bugs.webkit.org/show_bug.cgi?id=122336 > > (or dup appropriately if you already created your own bug.) I'll fix it in 122336. Testing now. 213381 2013-10-04 11:33:56 -0700 2013-10-04 11:44:33 -0700 Patch 122332.patch text/plain 2172 msaboff SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTU2ODk2KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBA CisyMDEzLTEwLTA0ICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAg ICAgIEZUTDogQ3Jhc2ggaW4gT1NSRXhpdDo6Y29udmVydFRvRm9yd2FyZCgpIHVzaW5nIFZpcnR1 YWxSZWdpc3Rlci5vZmZzZXQoKSBhcyBhcnJheSBpbmRleAorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTIyMzMyCisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQ2hhbmdlZCB0aGUgdXNlcyBvZiAub2Zmc2V0KCks IHdoaWNoIHJldHVybnMgYSBuZWdhdGl2ZSBudW1iZXIgZm9yIGxvY2FscywgdG8gYmUKKyAgICAg ICAgdG9Mb2NhbCgpIHdoaWNoIHJldHVybnMgYSBsb2NhbCdzIG9yZGluYWwgbnVtYmVyLgorCisg ICAgICAgICogZnRsL0ZUTE9TUkV4aXQuY3BwOgorICAgICAgICAoSlNDOjpGVEw6Ok9TUkV4aXQ6 OmNvbnZlcnRUb0ZvcndhcmQpOgorCiAyMDEzLTEwLTA0ICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJv ZmZAYXBwbGUuY29tPgogCiAgICAgICAgIEFkZCBjYWxsT3BlcmF0aW9uIHRvIEJhc2VsaW5lIEpJ VApJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL2Z0bC9GVExPU1JFeGl0LmNwcAo9PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZnRsL0ZUTE9TUkV4aXQuY3BwCShyZXZpc2lv biAxNTY4OTYpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZnRsL0ZUTE9TUkV4aXQuY3BwCSh3 b3JraW5nIGNvcHkpCkBAIC04NCwxNyArODQsMTcgQEAgdm9pZCBPU1JFeGl0Ojpjb252ZXJ0VG9G b3J3YXJkKAogICAgIC8vIElzIHRoZSB2YWx1ZSBmb3IgdGhpcyBvcGVyYW5kIGJlaW5nIHBhc3Nl ZCBhcyBhbiBhcmd1bWVudCB0byB0aGUgZXhpdCwgb3IgaXMKICAgICAvLyBpdCBzb21ldGhpbmcg ZWxzZT8gSWYgaXQncyBhbiBhcmd1bWVudCBhbHJlYWR5LCB0aGVuIHJlcGxhY2UgdGhhdCBhcmd1 bWVudDsKICAgICAvLyBvdGhlcndpc2UgYWRkIGFub3RoZXIgYXJndW1lbnQuCi0gICAgaWYgKG1f dmFsdWVzW292ZXJyaWRkZW5PcGVyYW5kLm9mZnNldCgpXS5pc0FyZ3VtZW50KCkpIHsKLSAgICAg ICAgRXhpdEFyZ3VtZW50IGV4aXRBcmd1bWVudCA9IG1fdmFsdWVzW292ZXJyaWRkZW5PcGVyYW5k Lm9mZnNldCgpXS5leGl0QXJndW1lbnQoKTsKKyAgICBpZiAobV92YWx1ZXNbb3ZlcnJpZGRlbk9w ZXJhbmQudG9Mb2NhbCgpXS5pc0FyZ3VtZW50KCkpIHsKKyAgICAgICAgRXhpdEFyZ3VtZW50IGV4 aXRBcmd1bWVudCA9IG1fdmFsdWVzW292ZXJyaWRkZW5PcGVyYW5kLnRvTG9jYWwoKV0uZXhpdEFy Z3VtZW50KCk7CiAgICAgICAgIGFyZ3VtZW50c1tleGl0QXJndW1lbnQuYXJndW1lbnQoKV0gPSB2 YWx1ZS52YWx1ZSgpOwotICAgICAgICBtX3ZhbHVlc1tvdmVycmlkZGVuT3BlcmFuZC5vZmZzZXQo KV0gPSBFeGl0VmFsdWU6OmV4aXRBcmd1bWVudCgKKyAgICAgICAgbV92YWx1ZXNbb3ZlcnJpZGRl bk9wZXJhbmQudG9Mb2NhbCgpXSA9IEV4aXRWYWx1ZTo6ZXhpdEFyZ3VtZW50KAogICAgICAgICAg ICAgZXhpdEFyZ3VtZW50LndpdGhGb3JtYXQodmFsdWUuZm9ybWF0KCkpKTsKICAgICAgICAgcmV0 dXJuOwogICAgIH0KICAgICAKICAgICB1bnNpZ25lZCBhcmd1bWVudCA9IGFyZ3VtZW50cy5zaXpl KCk7CiAgICAgYXJndW1lbnRzLmFwcGVuZCh2YWx1ZS52YWx1ZSgpKTsKLSAgICBtX3ZhbHVlc1tt X2xhc3RTZXRPcGVyYW5kLm9mZnNldCgpXSA9IEV4aXRWYWx1ZTo6ZXhpdEFyZ3VtZW50KAorICAg IG1fdmFsdWVzW21fbGFzdFNldE9wZXJhbmQudG9Mb2NhbCgpXSA9IEV4aXRWYWx1ZTo6ZXhpdEFy Z3VtZW50KAogICAgICAgICBFeGl0QXJndW1lbnQodmFsdWUuZm9ybWF0KCksIGFyZ3VtZW50KSk7 CiB9CiAK