Bug 203358 - [JSC] Get 32-bit ports back into building order
Summary: [JSC] Get 32-bit ports back into building order
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords: InRadar
Depends on:
Blocks: 203290
  Show dependency treegraph
 
Reported: 2019-10-24 01:38 PDT by Zan Dobersek
Modified: 2019-10-30 18:41 PDT (History)
10 users (show)

See Also:


Attachments
Patch (17.12 KB, patch)
2019-10-24 01:47 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch for landing (16.94 KB, patch)
2019-10-24 01:58 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2019-10-24 01:38:07 PDT
[JSC] Get 32-bit ports back into building order
Comment 1 Zan Dobersek 2019-10-24 01:47:16 PDT
Created attachment 381787 [details]
Patch
Comment 2 Zan Dobersek 2019-10-24 01:58:53 PDT
Created attachment 381788 [details]
Patch for landing
Comment 3 Zan Dobersek 2019-10-24 02:07:00 PDT
Comment on attachment 381788 [details]
Patch for landing

Clearing flags on attachment: 381788

Committed r251534: <https://trac.webkit.org/changeset/251534>
Comment 4 Zan Dobersek 2019-10-24 02:07:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Radar WebKit Bug Importer 2019-10-24 02:09:04 PDT
<rdar://problem/56572554>
Comment 6 Pablo Saavedra 2019-10-24 02:27:31 PDT
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.
Comment 7 Keith Miller 2019-10-24 10:20:31 PDT
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.
Comment 8 Zan Dobersek 2019-10-29 08:05:43 PDT
(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.
Comment 9 Saam Barati 2019-10-30 18:41:09 PDT
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());
}