117373 2013-06-08 13:20:44 -0700 REGRESSION: ASSERT failures in stack walking code on FTL branch 2013-06-08 13:40:15 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 mark.lam mark.lam fpizlo ggaren mhahnenberg msaboff oliver oldest_to_newest 898401 0 mark.lam 2013-06-08 13:20:44 -0700 The first assertion, in CallFrame::setLocationAsBytecodeOffset(), wrongly assumes that the incoming offset argument is in units of bytes. This is not true. It is in units of Instruction*. Hence, the assertion which checks for the low 2 bits to be clear can fail. The fix is to remove the assertion. 898402 1 204095 mark.lam 2013-06-08 13:23:42 -0700 Created attachment 204095 the patch. 898403 2 204095 oliver 2013-06-08 13:27:50 -0700 Comment on attachment 204095 the patch. Can we have an enum type so distinguish whether an offset should be bytes or instructions? e.g. enum InstructionOffset {}; enum ByteOffset {}; ByteOffset byteOffsetFromInstruction(unsigned i) { return i * sizeof Instruction } , etc 898404 3 mark.lam 2013-06-08 13:31:15 -0700 <rdar://problem/14100876> 898405 4 mark.lam 2013-06-08 13:37:10 -0700 (In reply to comment #2) > (From update of attachment 204095 [details]) > Can we have an enum type so distinguish whether an offset should be bytes or instructions? > > e.g. enum InstructionOffset {}; enum ByteOffset {}; ByteOffset byteOffsetFromInstruction(unsigned i) { return i * sizeof Instruction } , etc I'm not sure that will work. My gut feeling is that it will result in a lot of casting, which would defeat the type safety checks (which is why I think you want the enums, right?). Thanks for the review. Patch landed in r151347: <http://trac.webkit.org>. 898407 5 mark.lam 2013-06-08 13:40:15 -0700 (In reply to comment #4) > Patch landed in r151347: <http://trac.webkit.org>. Landed in r151347: <http://trac.webkit.org/changeset/151347>. 204095 2013-06-08 13:23:42 -0700 2013-06-08 13:27:50 -0700 the patch. bug-117373.patch text/plain 1478 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTUxMzQ2KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBA CisyMDEzLTA2LTA4ICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBS ZW1vdmVkIGJvZ3VzIGFzc2VydGlvbiBpbiBDYWxsRnJhbWU6OnNldExvY2F0aW9uQXNCeXRlY29k ZU9mZnNldCgpLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9MTE3MzczLgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIFRoZSBhc3NlcnRpb24gd3JvbmdseSBhc3N1bWVzIHRoYXQgdGhlIGluY29taW5nIG9mZnNl dCBhcmd1bWVudCBpcyBpbgorICAgICAgICB1bml0cyBvZiBieXRlcy4gVGhpcyBpcyBub3QgdHJ1 ZS4gSXQgaXMgaW4gdW5pdHMgb2YgSW5zdHJ1Y3Rpb24qLiBIZW5jZSwKKyAgICAgICAgdGhlIGFz c2VydGlvbiB3aGljaCBjaGVja3MgZm9yIHRoZSBsb3cgMiBiaXRzIHRvIGJlIGNsZWFyIGNhbiBm YWlsLgorCisgICAgICAgICogaW50ZXJwcmV0ZXIvQ2FsbEZyYW1lLmNwcDoKKyAgICAgICAgKEpT Qzo6Q2FsbEZyYW1lOjpzZXRMb2NhdGlvbkFzQnl0ZWNvZGVPZmZzZXQpOgorCiAyMDEzLTA2LTA3 ICBGaWxpcCBQaXpsbyAgPGZwaXpsb0BhcHBsZS5jb20+CiAKICAgICAgICAgVW5yZXZpZXdlZCwg cm9sbCBvdXQgaHR0cDovL3RyYWMud2Via2l0Lm9yZy9jaGFuZ2VzZXQvMTUxMzQyCkluZGV4OiBT b3VyY2UvSmF2YVNjcmlwdENvcmUvaW50ZXJwcmV0ZXIvQ2FsbEZyYW1lLmNwcAo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaW50ZXJwcmV0ZXIvQ2FsbEZyYW1lLmNwcAkocmV2 aXNpb24gMTUxMzQ2KQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL2ludGVycHJldGVyL0NhbGxG cmFtZS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTUyLDcgKzUyLDYgQEAgdW5zaWduZWQgQ2FsbEZy YW1lOjpsb2NhdGlvbkFzQnl0ZWNvZGVPZgogdm9pZCBDYWxsRnJhbWU6OnNldExvY2F0aW9uQXNC eXRlY29kZU9mZnNldCh1bnNpZ25lZCBvZmZzZXQpCiB7CiAgICAgQVNTRVJUKGNvZGVCbG9jaygp KTsKLSAgICBBU1NFUlQoTG9jYXRpb246OmlzQnl0ZWNvZGVMb2NhdGlvbihvZmZzZXQpKTsKICAg ICBzZXRDdXJyZW50VlBDKGNvZGVCbG9jaygpLT5pbnN0cnVjdGlvbnMoKS5iZWdpbigpICsgb2Zm c2V0KTsKICAgICBBU1NFUlQoaGFzTG9jYXRpb25Bc0J5dGVjb2RlT2Zmc2V0KCkpOwogfQo=