Bug 68301 – Macro assembler branch8 & 16 methods vary in treatment of upper bits

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

The martial register macro assembler functions vary in their handling of the "unused" bits.  Some of the functions generate code that ignores the unused bits, either via anding with a mask or shifting.  Other functions assume that the unused bits are zero.  Below are some examples of each.

There are two ways we can proceed, either fix functions that don't ignore to ignore the unused bits or change the convention so that a 32 bit compare will work in all cases.  For architectures that can perform zero filled (unsigned extend) loads of 8 and 16 bit quantities, the later solution would be fine.  

Function that generates code to ignore unused bits:

From MacroAssemblerARM.h:
    Jump branch16(RelationalCondition cond, RegisterID left, TrustedImm32 right)
    {
        ASSERT(!(right.m_value & 0xFFFF0000));
        right.m_value <<= 16;
        m_assembler.mov_r(ARMRegisters::S1, left);
        lshift32(TrustedImm32(16), ARMRegisters::S1);
        return branch32(cond, ARMRegisters::S1, right);
    }

From MacroAssemblerARMv7.h:
    Jump branch16(RelationalCondition cond, BaseIndex left, TrustedImm32 right)
    {
        // use addressTempRegister incase the branch32 we call uses dataTempRegister. :-/
        load16(left, addressTempRegister);
        m_assembler.lsl(addressTempRegister, addressTempRegister, 16);
        return branch32(cond, addressTempRegister, TrustedImm32(right.m_value << 16));
    }

Function that assumes unused bits are zero:

From MacroAssemblerArmv7.h:
    Jump branch8(RelationalCondition cond, RegisterID left, TrustedImm32 right)
    {
        compare32(left, right);
        return Jump(makeBranch(cond));
    }

From MacroAssemblerMIPS.h:
    Jump branch16(RelationalCondition cond, BaseIndex left, RegisterID right)
    {
        load16(left, dataTempRegister);
        return branch32(cond, dataTempRegister, right);
    }

[Gavin Barraclough]

In the case of branch8/16, the current intended behaviour is for any bits other than the low N to be ignored (where N is 8/16).  Any MacroAssembler implementation that does otherwise may function incorrectly.

This is easy to implement correctly on x86, but may be expensive on other platforms.  In many cases we may be able to change the JIT to ensure the full 32-bit value is known (e.g. zero extended), and call branch32 instead.

If we can change all client code to use branch32 we can fix this by simply removing the 8/16 bit compares.

[Gavin Barraclough]

Created attachment 108579 [details]
Fix for branch16

[Gavin Barraclough]

Fixed (removed) branch16 in r96169, but still need to check correctness of branch8.