Bug 203358 – [JSC] Get 32-bit ports back into building order

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[JSC] Get 32-bit ports back into building order

[Zan Dobersek]

Created attachment 381787 [details]
Patch

[Zan Dobersek]

Created attachment 381788 [details]
Patch for landing

[Zan Dobersek]

Comment on attachment 381788 [details]
Patch for landing

Clearing flags on attachment: 381788

Committed r251534: <https://trac.webkit.org/changeset/251534>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56572554>

[Pablo Saavedra]

Comment on attachment 381788 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=381788&action=review

LGTM

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1891
> +#if USE(JSVALUE64)

Good catch. I didn't see this yesterday.

[Keith Miller]

Comment on attachment 381788 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=381788&action=review

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:828
> +        uint32_t locationBits = CallSiteIndex(BytecodeIndex(bitwise_cast<uint32_t>(instruction))).bits();

This seems wrong... my change made CallSiteIndex always use the bytecode index rather than the instruction pointer on 32-bit.

I think you want: 
uint32_t locationBits = CallSiteIndex(BytecodeIndex(codeOrigin->bytecodeIndex())).bits();

> Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:332
> +        uint32_t locationBits = CallSiteIndex(BytecodeIndex(bitwise_cast<uint32_t>(instruction))).bits();

ditto.

> Source/JavaScriptCore/interpreter/CallFrame.cpp:119
> +    CallSiteIndex callSite(BytecodeIndex(bitwise_cast<uint32_t>(vpc)));

ditto.

> Source/JavaScriptCore/jit/JITCall32_64.cpp:291
> +    uint32_t locationBits = CallSiteIndex(BytecodeIndex(bitwise_cast<uint32_t>(instruction))).bits();

ditto.

> Source/JavaScriptCore/jit/JITInlines.h:131
> +#if USE(JSVALUE32_64)
> +    const Instruction* instruction = m_codeBlock->instructions().at(m_bytecodeIndex.offset()).ptr();
> +    uint32_t locationBits = CallSiteIndex(BytecodeIndex(bitwise_cast<uint32_t>(instruction))).bits();
> +#else

I think this is wrong.

> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:1389
> +    logShadowChickenTailPacket(shadowPacketReg, thisRegs, regT3, m_codeBlock, CallSiteIndex(BytecodeIndex(bitwise_cast<uint32_t>(currentInstruction))));

ditto.

> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:210
> +        m_codeBlock, CodeOrigin(m_bytecodeIndex), CallSiteIndex(BytecodeIndex(bitwise_cast<uint32_t>(currentInstruction))), RegisterSet::stubUnavailableRegisters(),

ditto.

> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:440
> +        m_codeBlock, CodeOrigin(m_bytecodeIndex), CallSiteIndex(BytecodeIndex(bitwise_cast<uint32_t>(currentInstruction))), RegisterSet::stubUnavailableRegisters(),

ditto.

> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:504
> +        m_codeBlock, CodeOrigin(m_bytecodeIndex), CallSiteIndex(BytecodeIndex(bitwise_cast<uint32_t>(currentInstruction))), RegisterSet::stubUnavailableRegisters(),

ditto.

> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:543
> +        m_codeBlock, CodeOrigin(m_bytecodeIndex), CallSiteIndex(BytecodeIndex(bitwise_cast<uint32_t>(currentInstruction))), RegisterSet::stubUnavailableRegisters(),

ditto.

[Zan Dobersek]

(In reply to Keith Miller from comment #7)
> Comment on attachment 381788 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381788&action=review
> 
> > Source/JavaScriptCore/dfg/DFGOSRExit.cpp:828
> > +        uint32_t locationBits = CallSiteIndex(BytecodeIndex(bitwise_cast<uint32_t>(instruction))).bits();
> 
> This seems wrong... my change made CallSiteIndex always use the bytecode
> index rather than the instruction pointer on 32-bit.
> 
> I think you want: 
> uint32_t locationBits =
> CallSiteIndex(BytecodeIndex(codeOrigin->bytecodeIndex())).bits();
> 

Besides the building errors, these changes were additionally causing plenty of test errors.

Problem is that on 32-bit platforms we don't yet have a PC base register that would hold the base address of the instructions data. Ideally we would fish out a register we could use for this, and eliminate the different approach here.

I opened bug #203563 to cover this.

[Saam Barati]

Comment on attachment 381788 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=381788&action=review

>>> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:828
>>> +        uint32_t locationBits = CallSiteIndex(BytecodeIndex(bitwise_cast<uint32_t>(instruction))).bits();
>> 
>> This seems wrong... my change made CallSiteIndex always use the bytecode index rather than the instruction pointer on 32-bit.
>> 
>> I think you want: 
>> uint32_t locationBits = CallSiteIndex(BytecodeIndex(codeOrigin->bytecodeIndex())).bits();
> 
> Besides the building errors, these changes were additionally causing plenty of test errors.
> 
> Problem is that on 32-bit platforms we don't yet have a PC base register that would hold the base address of the instructions data. Ideally we would fish out a register we could use for this, and eliminate the different approach here.
> 
> I opened bug #203563 to cover this.

Keith, are you sure you made that change? ToT says:

const Instruction* CallFrame::currentVPC() const
{
    return bitwise_cast<Instruction*>(callSiteIndex().bits());
}