160792 2016-08-11 17:39:59 -0700 OverridesHasInstance should not branch across register allocations. 2016-08-11 20:41:43 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Local Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 mark.lam mark.lam benjamin fpizlo ggaren keith_miller msaboff saam webkit-bug-importer oldest_to_newest 1219478 0 mark.lam 2016-08-11 17:39:59 -0700 The OverrideHasInstance node has a branch test that is emitted conditionally. It also has a bug where it allocated a register after this branch. Because the branch isn't always emitted, this bug has gone unnoticed until now. Will fix. 1219479 1 mark.lam 2016-08-11 17:40:44 -0700 <rdar://problem/27361778> 1219487 2 285874 mark.lam 2016-08-11 17:52:14 -0700 Created attachment 285874 proposed patch. 1219488 3 mark.lam 2016-08-11 17:55:25 -0700 For the record, this bug was causing this assertion failure: ASSERTION FAILED: Unsafe branch over register allocation at instruction offset 1651 in jump offset range 1651..1678 !(low <= m_offset && m_offset <= high) /var/root/_REPOS/OpenSource/Source/JavaScriptCore/assembler/AbstractMacroAssembler.h(820) : void JSC::AbstractMacroAssembler<JSC::X86Assembler, JSC::MacroAssemblerX86Common>::RegisterAllocationOffset::checkOffsets(unsigned int, unsigned int) [AssemblerType = JSC::X86Assembler, MacroAssemblerType = JSC::MacroAssemblerX86Common] 1 0x10530382d WTFCrash 2 0x1042c8c3d JSC::AbstractMacroAssembler<JSC::X86Assembler, JSC::MacroAssemblerX86Common>::RegisterAllocationOffset::checkOffsets(unsigned int, unsigned int) 3 0x1042c8a3f JSC::AbstractMacroAssembler<JSC::X86Assembler, JSC::MacroAssemblerX86Common>::checkRegisterAllocationAgainstBranchRange(unsigned int, unsigned int) 4 0x1042cd75c JSC::AbstractMacroAssembler<JSC::X86Assembler, JSC::MacroAssemblerX86Common>::Jump::link(JSC::AbstractMacroAssembler<JSC::X86Assembler, JSC::MacroAssemblerX86Common>*) const 5 0x104a167bb JSC::DFG::SpeculativeJIT::compile(JSC::DFG::Node*) 6 0x1049a0c6f JSC::DFG::SpeculativeJIT::compileCurrentBlock() 7 0x1049a17b3 JSC::DFG::SpeculativeJIT::compile() 8 0x10486fe17 JSC::DFG::JITCompiler::compileBody() 9 0x104874d2f JSC::DFG::JITCompiler::compileFunction() 10 0x10495d82a JSC::DFG::Plan::compileInThreadImpl(JSC::DFG::LongLivedState&) 11 0x10495c360 JSC::DFG::Plan::compileInThread(JSC::DFG::LongLivedState&, JSC::DFG::ThreadData*) 12 0x104a95577 JSC::DFG::Worklist::runThread(JSC::DFG::ThreadData*) 13 0x104a92714 JSC::DFG::Worklist::threadFunction(void*) ... 1219497 4 285874 benjamin 2016-08-11 18:23:08 -0700 Comment on attachment 285874 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=285874&action=review > JSTests/stress/OverrideHasInstance-should-not-branch-across-register-allocations.js:1 > +//@ run("--useConcurrentJIT=false") needed? 1219499 5 285874 saam 2016-08-11 18:24:27 -0700 Comment on attachment 285874 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=285874&action=review > Source/JavaScriptCore/ChangeLog:12 > + The OverrideHasInstance node has a branch test that is emitted conditionally. > + It also has a bug where it allocated a register after this branch which is not > + allowed. Because the branch isn't always emitted, this bug has gone unnoticed > + until now. I guess I don't understand what the bug actually is. Can you elaborate what happens when this bad condition happens? 1219500 6 saam 2016-08-11 18:25:24 -0700 (In reply to comment #5) > Comment on attachment 285874 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=285874&action=review > > > Source/JavaScriptCore/ChangeLog:12 > > + The OverrideHasInstance node has a branch test that is emitted conditionally. > > + It also has a bug where it allocated a register after this branch which is not > > + allowed. Because the branch isn't always emitted, this bug has gone unnoticed > > + until now. > > I guess I don't understand what the bug actually is. Can you elaborate what > happens when this bad condition happens? Is the assertion there to just help us not make mistakes? 1219511 7 saam 2016-08-11 19:29:13 -0700 (In reply to comment #6) > (In reply to comment #5) > > Comment on attachment 285874 [details] > > proposed patch. > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=285874&action=review > > > > > Source/JavaScriptCore/ChangeLog:12 > > > + The OverrideHasInstance node has a branch test that is emitted conditionally. > > > + It also has a bug where it allocated a register after this branch which is not > > > + allowed. Because the branch isn't always emitted, this bug has gone unnoticed > > > + until now. > > > > I guess I don't understand what the bug actually is. Can you elaborate what > > happens when this bad condition happens? > > Is the assertion there to just help us not make mistakes? Ben explained this to me. Basically, calling gpr() can emit code that populates that gpr. That's really wrong to branch around that code that fills the baseGPR in the bug fix. 1219513 8 285874 mark.lam 2016-08-11 20:01:49 -0700 Comment on attachment 285874 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=285874&action=review Thanks for the review. >> JSTests/stress/OverrideHasInstance-should-not-branch-across-register-allocations.js:1 >> +//@ run("--useConcurrentJIT=false") > > needed? Not strictly needed. I'll change this to runDefault because we don't benefit from running this test against all flavors of configurations. So, just choosing one that will exercise the issue. >>>> Source/JavaScriptCore/ChangeLog:12 >>>> + until now. >>> >>> I guess I don't understand what the bug actually is. Can you elaborate what happens when this bad condition happens? >> >> Is the assertion there to just help us not make mistakes? > > Ben explained this to me. > Basically, calling gpr() can emit code that populates that gpr. > That's really wrong to branch around that code that fills the > baseGPR in the bug fix. Yep. That assertion is there to help catch these issues, and it did its job. =) 1219514 9 mark.lam 2016-08-11 20:10:16 -0700 (In reply to comment #8) > > Ben explained this to me. > > Basically, calling gpr() can emit code that populates that gpr. > > That's really wrong to branch around that code that fills the > > baseGPR in the bug fix. I'll also add this to the ChangeLog to make it easier to follow the reasoning. 1219523 10 mark.lam 2016-08-11 20:41:43 -0700 Landed in r204403: <http://trac.webkit.org/r204403>. 285874 2016-08-11 17:52:14 -0700 2016-08-11 19:26:08 -0700 proposed patch. bug-160792.patch text/plain 4485 mark.lam SW5kZXg6IEpTVGVzdHMvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIEpTVGVzdHMvQ2hhbmdlTG9n CShyZXZpc2lvbiAyMDQzOTcpCisrKyBKU1Rlc3RzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpA QCAtMSwzICsxLDEzIEBACisyMDE2LTA4LTExICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNv bT4KKworICAgICAgICBPdmVycmlkZXNIYXNJbnN0YW5jZSBzaG91bGQgbm90IGJyYW5jaCBhY3Jv c3MgcmVnaXN0ZXIgYWxsb2NhdGlvbnMuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xNjA3OTIKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzI3MzYxNzc4 PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogc3Ry ZXNzL092ZXJyaWRlSGFzSW5zdGFuY2Utc2hvdWxkLW5vdC1icmFuY2gtYWNyb3NzLXJlZ2lzdGVy LWFsbG9jYXRpb25zLmpzOiBBZGRlZC4KKwogMjAxNi0wOC0xMSAgTWFyayBMYW0gIDxtYXJrLmxh bUBhcHBsZS5jb20+CiAKICAgICAgICAgVGhlIGpzYyBzaGVsbCdzIEVsZW1lbnQgaG9zdCBjb25z dHJ1Y3RvciBzaG91bGQgdGhyb3cgaWYgaXQgZmFpbHMgdG8gY29uc3RydWN0IGFuIG9iamVjdC4K SW5kZXg6IEpTVGVzdHMvc3RyZXNzL092ZXJyaWRlSGFzSW5zdGFuY2Utc2hvdWxkLW5vdC1icmFu Y2gtYWNyb3NzLXJlZ2lzdGVyLWFsbG9jYXRpb25zLmpzCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIEpTVGVzdHMv c3RyZXNzL092ZXJyaWRlSGFzSW5zdGFuY2Utc2hvdWxkLW5vdC1icmFuY2gtYWNyb3NzLXJlZ2lz dGVyLWFsbG9jYXRpb25zLmpzCShub25leGlzdGVudCkKKysrIEpTVGVzdHMvc3RyZXNzL092ZXJy aWRlSGFzSW5zdGFuY2Utc2hvdWxkLW5vdC1icmFuY2gtYWNyb3NzLXJlZ2lzdGVyLWFsbG9jYXRp b25zLmpzCSh3b3JraW5nIGNvcHkpCkBAIC0wLDAgKzEsMTYgQEAKKy8vQCBydW4oIi0tdXNlQ29u Y3VycmVudEpJVD1mYWxzZSIpCisKKy8vIFRoaXMgdGVzdCBzaG91bGQgbm90IGNyYXNoLgorCitk ZWxldGUgdGhpcy5GdW5jdGlvbjsKKwordmFyIHRlc3QgPSBmdW5jdGlvbigpIHsgCisgICAgTWF0 aC5jb3MoIjAiIGluc3RhbmNlb2YgYXJndW1lbnRzKQorfQorCitmb3IgKHZhciBrID0gMDsgayA8 IDEwMDAwOyArK2spIHsKKyAgICB0cnkgeworICAgICAgICB0ZXN0KCk7CisgICAgfSBjYXRjaCAo ZSkgeworICAgIH0KK30KSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjA0 Mzg4KQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpA QCAtMSwzICsxLDI1IEBACisyMDE2LTA4LTExICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNv bT4KKworICAgICAgICBPdmVycmlkZXNIYXNJbnN0YW5jZSBzaG91bGQgbm90IGJyYW5jaCBhY3Jv c3MgcmVnaXN0ZXIgYWxsb2NhdGlvbnMuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xNjA3OTIKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzI3MzYxNzc4 PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZSBP dmVycmlkZUhhc0luc3RhbmNlIG5vZGUgaGFzIGEgYnJhbmNoIHRlc3QgdGhhdCBpcyBlbWl0dGVk IGNvbmRpdGlvbmFsbHkuCisgICAgICAgIEl0IGFsc28gaGFzIGEgYnVnIHdoZXJlIGl0IGFsbG9j YXRlZCBhIHJlZ2lzdGVyIGFmdGVyIHRoaXMgYnJhbmNoIHdoaWNoIGlzIG5vdAorICAgICAgICBh bGxvd2VkLiAgQmVjYXVzZSB0aGUgYnJhbmNoIGlzbid0IGFsd2F5cyBlbWl0dGVkLCB0aGlzIGJ1 ZyBoYXMgZ29uZSB1bm5vdGljZWQKKyAgICAgICAgdW50aWwgbm93LgorCisgICAgICAgIFRoaXMg cGF0Y2ggZml4ZXMgdGhpcyBpc3N1ZSBieSBwcmUtYWxsb2NhdGluZyB0aGUgcmVnaXN0ZXJzIGJl Zm9yZSBlbWl0dGluZyB0aGUKKyAgICAgICAgYnJhbmNoIGluIE92ZXJyaWRlSGFzSW5zdGFuY2Uu CisKKyAgICAgICAgTm90ZTogdGhpcyBpc3N1ZSBpcyBvbmx5IHByZXNlbnQgaW4gREZHU3BlY3Vs YXRpdmVKSVQ2NC5jcHAuICBUaGUgMzItYml0IHZlcnNpb24KKyAgICAgICAgaXMgZG9pbmcgaXQg cmlnaHQuCisKKyAgICAgICAgKiBkZmcvREZHU3BlY3VsYXRpdmVKSVQ2NC5jcHA6CisgICAgICAg IChKU0M6OkRGRzo6U3BlY3VsYXRpdmVKSVQ6OmNvbXBpbGUpOgorCiAyMDE2LTA4LTExICBNYXJr IExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KIAogICAgICAgICBUaGUganNjIHNoZWxsJ3MgRWxl bWVudCBob3N0IGNvbnN0cnVjdG9yIHNob3VsZCB0aHJvdyBpZiBpdCBmYWlscyB0byBjb25zdHJ1 Y3QgYW4gb2JqZWN0LgpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdTcGVjdWxh dGl2ZUpJVDY0LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RG R1NwZWN1bGF0aXZlSklUNjQuY3BwCShyZXZpc2lvbiAyMDQzODcpCisrKyBTb3VyY2UvSmF2YVNj cmlwdENvcmUvZGZnL0RGR1NwZWN1bGF0aXZlSklUNjQuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC00 NTc1LDE1ICs0NTc1LDE4IEBAIHZvaWQgU3BlY3VsYXRpdmVKSVQ6OmNvbXBpbGUoTm9kZSogbm9k ZSkKICAgICAgICAgR1BSVGVtcG9yYXJ5IHJlc3VsdCh0aGlzKTsKIAogICAgICAgICBHUFJSZWcg cmVzdWx0R1BSID0gcmVzdWx0LmdwcigpOworICAgICAgICBHUFJSZWcgYmFzZUdQUiA9IGJhc2Uu Z3ByKCk7CiAKICAgICAgICAgLy8gSXQgd291bGQgYmUgZ3JlYXQgaWYgY29uc3RhbnQgZm9sZGlu ZyBoYW5kbGVkIGF1dG9tYXRpY2FsbHkgdGhlIGNhc2Ugd2hlcmUgd2Uga25ldyB0aGUgaGFzSW5z dGFuY2UgZnVuY3Rpb24KICAgICAgICAgLy8gd2FzIGEgY29uc3RhbnQuIFVuZm9ydHVuYXRlbHks IHRoZSBmb2xkaW5nIHJ1bGUgZm9yIE92ZXJyaWRlc0hhc0luc3RhbmNlIGlzIGluIHRoZSBzdHJl bmd0aCByZWR1Y3Rpb24gcGhhc2UKICAgICAgICAgLy8gc2luY2UgaXQgcmVsaWVzIG9uIE9TUiBp bmZvcm1hdGlvbi4gaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1NDgz MgotICAgICAgICBpZiAoIWhhc0luc3RhbmNlVmFsdWVOb2RlLT5pc0NlbGxDb25zdGFudCgpIHx8 IGRlZmF1bHRIYXNJbnN0YW5jZUZ1bmN0aW9uICE9IGhhc0luc3RhbmNlVmFsdWVOb2RlLT5hc0Nl bGwoKSkKLSAgICAgICAgICAgIG5vdERlZmF1bHQgPSBtX2ppdC5icmFuY2hQdHIoTWFjcm9Bc3Nl bWJsZXI6Ok5vdEVxdWFsLCBoYXNJbnN0YW5jZVZhbHVlLmdwcigpLCBUcnVzdGVkSW1tUHRyKGRl ZmF1bHRIYXNJbnN0YW5jZUZ1bmN0aW9uKSk7CisgICAgICAgIGlmICghaGFzSW5zdGFuY2VWYWx1 ZU5vZGUtPmlzQ2VsbENvbnN0YW50KCkgfHwgZGVmYXVsdEhhc0luc3RhbmNlRnVuY3Rpb24gIT0g aGFzSW5zdGFuY2VWYWx1ZU5vZGUtPmFzQ2VsbCgpKSB7CisgICAgICAgICAgICBHUFJSZWcgaGFz SW5zdGFuY2VWYWx1ZUdQUiA9IGhhc0luc3RhbmNlVmFsdWUuZ3ByKCk7CisgICAgICAgICAgICBu b3REZWZhdWx0ID0gbV9qaXQuYnJhbmNoUHRyKE1hY3JvQXNzZW1ibGVyOjpOb3RFcXVhbCwgaGFz SW5zdGFuY2VWYWx1ZUdQUiwgVHJ1c3RlZEltbVB0cihkZWZhdWx0SGFzSW5zdGFuY2VGdW5jdGlv bikpOworICAgICAgICB9CiAKICAgICAgICAgLy8gQ2hlY2sgdGhhdCBiYXNlICdJbXBsZW1lbnRz RGVmYXVsdEhhc0luc3RhbmNlJy4KLSAgICAgICAgbV9qaXQudGVzdDgoTWFjcm9Bc3NlbWJsZXI6 Olplcm8sIE1hY3JvQXNzZW1ibGVyOjpBZGRyZXNzKGJhc2UuZ3ByKCksIEpTQ2VsbDo6dHlwZUlu Zm9GbGFnc09mZnNldCgpKSwgTWFjcm9Bc3NlbWJsZXI6OlRydXN0ZWRJbW0zMihJbXBsZW1lbnRz RGVmYXVsdEhhc0luc3RhbmNlKSwgcmVzdWx0R1BSKTsKKyAgICAgICAgbV9qaXQudGVzdDgoTWFj cm9Bc3NlbWJsZXI6Olplcm8sIE1hY3JvQXNzZW1ibGVyOjpBZGRyZXNzKGJhc2VHUFIsIEpTQ2Vs bDo6dHlwZUluZm9GbGFnc09mZnNldCgpKSwgTWFjcm9Bc3NlbWJsZXI6OlRydXN0ZWRJbW0zMihJ bXBsZW1lbnRzRGVmYXVsdEhhc0luc3RhbmNlKSwgcmVzdWx0R1BSKTsKICAgICAgICAgbV9qaXQu b3IzMihUcnVzdGVkSW1tMzIoVmFsdWVGYWxzZSksIHJlc3VsdEdQUik7CiAgICAgICAgIE1hY3Jv QXNzZW1ibGVyOjpKdW1wIGRvbmUgPSBtX2ppdC5qdW1wKCk7CiAK