Bug 160792 – OverridesHasInstance should not branch across register allocations.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

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.

[Mark Lam]

<rdar://problem/27361778>

[Mark Lam]

Created attachment 285874 [details]
proposed patch.

[Mark Lam]

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*)
...

[Benjamin Poulain]

Comment on attachment 285874 [details]
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?

[Saam Barati]

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?

[Saam Barati]

(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?

[Saam Barati]

(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.

[Mark Lam]

Comment on attachment 285874 [details]
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. =)

[Mark Lam]

(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.

[Mark Lam]

Landed in r204403: <http://trac.webkit.org/r204403>.