638926 - Possible incorrect register allocation in conditional branches on MIPS

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect)

Description

[William Maddox]

When a register is allocated vis findRegFor(), it may be necessary to evict a register in order to satisfy the allocation.  If eviction is required, code is emitted to reload the evicted register with its previous contents.  The expectation is that, since the code is emitted in reverse order, than the reload will then be executed following the code that uses the newly-allocated register.  This is a problem if the code that uses the register branches, and control never falls through to the reload.  The register state assumed when merging with the state from the branch target may then fail to agree with the actual register contents.

On most platforms, we deal with this issue by separating the generation of a conditional branch into a comparison, e.g., asm_cmp(), in which the registers are allocated, followed by the generation of a conditional branch on the condition codes.  Any register reloads due to the allocations in asm_cmp() are then executed before the actual branch.

While the MIPS architecture does not use condition codes in this manner, the unallocated assembler temporary (AT) register can be used to safely communicate the comparison result to the branch.

For reasons I have not investigated, the MIPS code generator does not follow this approach, but combines both the emission of the comparison and the branch in a helper routine asm_bxx():


    NIns* Assembler::asm_branch(bool branchOnFalse, LIns *cond, NIns * const targ)
    {
        NanoAssert(cond->isCmp());
        LOpcode condop = cond->opcode();
        RegisterMask allow = isCmpDOpcode(condop) ? FpRegs : GpRegs;
        LIns *a = cond->oprnd1();
        LIns *b = cond->oprnd2();
        Register ra = findRegFor(a, allow);
        Register rb = (b==a) ? ra : findRegFor(b, allow & ~rmask(ra));

        return asm_bxx(branchOnFalse, condop, ra, rb, targ);
    }

This should be refactored to look something like this:

    NIns* Assembler::asm_branch(bool branchOnFalse, LIns *cond, NIns * const targ)
    {
        NanoAssert(cond->isCmp());
        LOpcode condop = cond->opcode();

        NIns* patch = asm_bxx(branchOnFalse, condop, targ);  // emit branch

        RegisterMask allow = isCmpDOpcode(condop) ? FpRegs : GpRegs;
        LIns *a = cond->oprnd1();
        LIns *b = cond->oprnd2();
        Register ra = findRegFor(a, allow);
        Register rb = (b==a) ? ra : findRegFor(b, allow & ~rmask(ra));

        asm_cxx(condop, ra, rb, targ);  // emit comparison

        return patch;
    }

This issue was discovered by inspection, and has not been observed in the field.  Most likely, the generous size of of the MIPS register set has masked the issue, but my analysis could be mistaken, e.g., if provision has been made elsewhere to assure that sufficient free registers are available to avoid spills.