Bug 205232 – DFG/FTL should be able to exit to the middle of a bytecode

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

DFG/FTL should be able to exit to the middle of a bytecode

[Keith Miller]

Created attachment 385671 [details]
Patch

[Keith Miller]

Created attachment 385672 [details]
Patch

[Keith Miller]

Created attachment 385674 [details]
Patch

[Keith Miller]

Created attachment 385676 [details]
Patch

[Keith Miller]

Created attachment 385677 [details]
Patch

[Keith Miller]

Created attachment 385679 [details]
Patch

[Keith Miller]

Created attachment 385680 [details]
Patch

[Keith Miller]

Comment on attachment 385680 [details]
Patch

Weird all the non-apply tests are passing locally for me... will investigate

[Keith Miller]

Created attachment 386022 [details]
Patch

[Keith Miller]

Created attachment 386025 [details]
Patch

[Keith Miller]

Created attachment 386029 [details]
Patch

[Keith Miller]

Created attachment 386031 [details]
Patch

[Keith Miller]

Created attachment 386034 [details]
Patch

[Keith Miller]

> ../../Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:575:56: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare]
>                                  for (unsigned i = 0; i < inlineCallFrame->argumentCountIncludingThis - 1; ++i) {

Wait what?? inlineCallFrame->argumentCountIncludingThis is unsigned so the result of `inlineCallFrame->argumentCountIncludingThis - 1` should also be unsigned!

Ugh

[Keith Miller]

Created attachment 386040 [details]
Patch

[Keith Miller]

Created attachment 386041 [details]
Patch

[Keith Miller]

Created attachment 386047 [details]
Patch

[Keith Miller]

Created attachment 386053 [details]
Patch

[Keith Miller]

Created attachment 386054 [details]
Patch

[Keith Miller]

Created attachment 386056 [details]
Patch

[Keith Miller]

Created attachment 386057 [details]
Patch

[Keith Miller]

Created attachment 386062 [details]
Patch

[Keith Miller]

Created attachment 386118 [details]
Patch

[Saam Barati]

Comment on attachment 386118 [details]
Patch

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

Wowza. I really like this patch. I'm gonna hold off on r+ until some of my comments/questions/concerns are addressed

> Source/JavaScriptCore/ChangeLog:19
> +        When the DFG OSR exits if there are any active checkpoints inline

you should explain what a checkpoint is before this sentence. Might also be worth saying how you packed in checkpoint index into bytecode index

> Source/JavaScriptCore/ChangeLog:34
> +        refers to virtual registers as a int as possible. Instead

"a int" => "an int"
"Instead" => "Instead, "

> Source/JavaScriptCore/ChangeLog:36
> +        couple of new classes/enum added to JSC:

"enum" => "enums"

> Source/JavaScriptCore/ChangeLog:56
> +        3) FTL::SelectPredictability is a new enum that describes to the
> +        FTL whether or not we think a select is going to be
> +        predictable. SelectPredictability has four options: Unpredictable,
> +        Predictable, LeftLikely, and RightLikely. Unpredictable means we
> +        think a branch predictor won't do a good job guessing this value
> +        so we should compile the select to a cmov. The other options mean
> +        we either think we are going to pick the same value every time or
> +        there's a reasonable chance the branch predictor will be able to
> +        guess the value.

this is a random point. Why does this patch include this?

> Source/JavaScriptCore/ChangeLog:61
> +        getters twice if we OSR exit during from LoadVarargs.

"during from" => "during" or "from"

> Source/JavaScriptCore/bytecode/BytecodeIndex.h:51
> +    static constexpr uint32_t numberOfCheckpoints = 4;

This should be called maxNumberOfCheckpoints

> Source/JavaScriptCore/bytecode/FullBytecodeLiveness.h:37
> +// Note: Full bytecode liveness does not track any information about the liveness of temps.

"temps" => "tmps"

> Source/JavaScriptCore/bytecode/Operands.h:41
> +enum OperandKind { ArgumentOperand, LocalOperand, TmpOperand };

why not "enum class OperandKind {Argument, Local, Tmp}"?

> Source/JavaScriptCore/bytecode/Operands.h:140
> -        m_values.fill(initialValue, numArguments + numLocals);
> +        m_values.grow(numArguments + numLocals + numTmps);
> +        m_values.fill(initialValue);

why this change?

> Source/JavaScriptCore/bytecode/Operands.h:240
> +                m_values[localIndex(oldNumLocals + i)] = ensuredValue;

this seems like a semantic change and slightly sketchy you're not constructing a new one of each entry. I think you should keep this "= T()", and let the compiler optimize away

> Source/JavaScriptCore/bytecode/Operands.h:244
> +    void ensureTmps(size_t size, const T& ensuredValue = T())

same comment

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3233
> +        ASSERT(m_codeBlock->hasCheckpoints());

we don't have check points for tail call forward varargs too?

> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:861
> +
> +

style nit: only one newline

> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:956
> +                                unsigned frameArgumentCount = static_cast<unsigned>(inlineCallFrame->argumentCountIncludingThis - 1);

this is needed?

> Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:65
> +    InsertionSet& insertionSet, Node* arguments, unsigned nodeIndex, NodeOrigin origin, bool addThis)

style nit: This should be an enum, not bool, according to WK style

> Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:146
> +            insertionSet.insertConstantForUse(nodeIndex, origin, jsNumber(addThis), Int32Use));

nit: jsNumber(static_cast<int32_t>(addThis)) so people don't go crazy reading this LOC

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:285
> +    void progressCheckpoint()

nit: progressCheckpoint => progressToNextCheckpoint?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:423
> +    Node* getLocalOrTmp(Operand operand)

maybe file a bug on a rename here? Should we name the node Get or GetOperand instead of keeping it GetLocal? (Same w/ SetLocal)

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:635
> +        ASSERT(operand.isTmp() || !operand.virtualRegister().isConstant());

nit: maybe just add an isConstant() to Operand that is this logic?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1130
> +    // The max number of temps used for forwarding data to an OSR exit to checkpoint.

"to an OSR exit to checkpoint"? Did you mean to have the second "to"?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1662
> +    ensureTmps((m_inlineStackTop->m_inlineCallFrame ? m_inlineStackTop->m_inlineCallFrame->tmpOffset : 0) + m_inlineStackTop->m_codeBlock->numTmps() + codeBlock->numTmps());

nit: Why not put this after the new inline stack entry constructor below, and just have it be:
ensureTmps(m_inlineStackTop->m_inlineCallFrame->tmpOffset + codeBLock->numTmps())

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1963
> +            auto argCountTmp = m_inlineStackTop->remapOperand(Operand(TmpOperand, OpCallVarargs::argCount));
> +            setDirect(argCountTmp, addToGraph(VarargsLength, OpInfo(data), arguments));

can we have a version that doesn't require us to call setDirect? Like a set that does remap for us?

> Source/JavaScriptCore/dfg/DFGCPSRethreadingPhase.cpp:497
> +        RELEASE_ASSERT_NOT_REACHED();

is this needed? Wouldn't the switch be a compile error?

> Source/JavaScriptCore/dfg/DFGClobberize.h:818
> +        write(AbstractHeap(Stack, data->count));

I thought you said we don't do this anymore?

> Source/JavaScriptCore/dfg/DFGCommonData.cpp:59
> +    return DisposableCallSiteIndex(index);

why all these DisposableCallSiteIndex changes? How is that right?

> Source/JavaScriptCore/dfg/DFGForAllKills.h:78
> +        if (after.bytecodeIndex().checkpoint()) {

the code inside this "if" is assuming that these checkpoints belong to the same bytecode offset. Is that an ok assumption to make? Maybe assert that?

I think this is an ok assumption, since when the DFG does code motion, it changes forExit origin, and this code is using forExit

> Source/JavaScriptCore/dfg/DFGGraph.cpp:1222
> +    if (verbose)
> +        dataLog("Ran out of stack, returning true.\n");
> +    return true;    
>      
>      RELEASE_ASSERT_NOT_REACHED();

what's going on here? Looks wrong...

> Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp:181
> +                flush(Operand(TmpOperand, tmp));

nit: You use this constructor in a few places, would be nice to just have "Operand::tmp(tmp)"

> Source/JavaScriptCore/dfg/DFGNodeType.h:77
> +    /* These are used in SSA form to represent to track */\

track?

> Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:246
> +    case VarargsLength: {
> +        break;
> +    }

why?

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:641
> +                addSideState(reinterpret_cast<CallFrame*>(reinterpret_cast<char*>(callFrame) + inlineCallFrame->returnPCOffset() - sizeof(CPURegister)), callBytecodeIndex, inlineCallFrame->tmpOffset);

there isn't an offset just for call frame?

> Source/JavaScriptCore/dfg/DFGOperations.cpp:2957
> +    for (uint32_t i = length - 1; i < mandatoryMinimum; ++i)

why - 1 now?

> Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:524
> +                        RELEASE_ASSERT(node->op() == VarargsLength || node->op() == LoadVarargs || node->op() == ForwardVarargs);

as I said in clobberize comment, does this actually write to that stack slot?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:-7370
> -    // https://bugs.webkit.org/show_bug.cgi?id=141448

you should dupe https://bugs.webkit.org/show_bug.cgi?id=141448 to this :-)

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7388
> +    m_jit.store32(argumentCountIncludingThis, JITCompiler::payloadFor(data->machineCount));

seems like LoadVarargs should write to this. And VarargsLength should not. In clobberize

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9572
> +            m_out.bitOr(m_out.isZero32(lengthIncludingThis), m_out.above(lengthIncludingThis, m_out.constInt32(data->limit)))); 

why would it be zero? DFG doesn't seem to do this?

> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:488
> +                    sideState->tmps[i] = tmpScratch[i + tmpOffset];

these are already boxed? This seems different than in the DFG, where you had to do value recovery. Is that right? Why not make the DFG also put the boxed value in the buffer before you read it out, so that DFG OSR exit code is cleaner?

> Source/JavaScriptCore/interpreter/CallFrame.cpp:108
> +    return CallSiteIndex::fromBits(unsafeCallSiteAsRawBits() & (std::numeric_limits<uint32_t>::max() >> BytecodeIndex::checkpointShift));

why not construct a BytecodeIndex and then call the helper function?

This looks wrong anyways. I think you meant to mask off the checkpoint index. But that begs the question, why do you need to do that? That's wrong for DFG/FTL CallSiteIndices, right? Those are just indices into an array that we bound check in the sampling profiler.

> Source/JavaScriptCore/interpreter/CallFrame.h:115
> +        static constexpr int headerSizeInRegisters = static_cast<int>(CallFrameSlot::argumentCount) + 1;

why is this static_cast needed if you overload the operators to work with CallFrameSlot?

> Source/JavaScriptCore/interpreter/CheckpointOSRExitSideState.h:37
> +    T getTmp(size_t offset) 

his is only used for JSValue. Let's just make this return JSValue.

> Source/JavaScriptCore/interpreter/CheckpointOSRExitSideState.h:52
> +    void* nextInstructionJumpTarget;

this is unused

> Source/JavaScriptCore/interpreter/Interpreter.cpp:545
> +            // FIXME: We should support exception handling in checkpoints.

hat do you mean by this?
Please file a bug if it's important

> Source/JavaScriptCore/jit/JITExceptions.cpp:53
> -        CRASH();
> +        WTFBreakpointTrap();

why?

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1969
> +    unsigned argumentCountIncludingThis = sideState.getTmp<JSValue>(Opcode::argCount).asUInt32();

let's just make getTmp return JSValue

Also, let's rename argCount to argCount to argCountIncludingThis for consistency

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1973
> +        loadVarargs(globalObject, buffer, callFrame->r(bytecode.m_arguments).jsValue(), 0, argumentCountIncludingThis - 1);

is zero the right offset here? Isn't it based on something in the opcode?

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1992
> +inline SlowPathReturnType dispatchNextInstruction(CodeBlock* codeBlock, InstructionStream::Ref pc)

nit: I'd name this "dispatchToNextInstruction"?

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:2064
> +        ASSERT_WITH_MESSAGE(pc.next()->opcodeID() == op_ret, "We strongly assume all tail calls are followed by an op_ret.");

why does it really matter though? I don't see how it changes this code

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1780
> +
> +

nit: undo

> Source/JavaScriptCore/runtime/VM.cpp:1031
> +void VM::addCheckpointOSRSideState(CallFrame* callFrame, std::unique_ptr<CheckpointOSRExitSideState>&& payload)

Don't you need to visit these things?

[Keith Miller]

Comment on attachment 386118 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:19
>> +        When the DFG OSR exits if there are any active checkpoints inline
> 
> you should explain what a checkpoint is before this sentence. Might also be worth saying how you packed in checkpoint index into bytecode index

True. I added a comment above.

>> Source/JavaScriptCore/ChangeLog:34
>> +        refers to virtual registers as a int as possible. Instead
> 
> "a int" => "an int"
> "Instead" => "Instead, "

Fixed.

>> Source/JavaScriptCore/ChangeLog:36
>> +        couple of new classes/enum added to JSC:
> 
> "enum" => "enums"

Fixed.

>> Source/JavaScriptCore/ChangeLog:61
>> +        getters twice if we OSR exit during from LoadVarargs.
> 
> "during from" => "during" or "from"

Fixed.

>> Source/JavaScriptCore/bytecode/Operands.h:41
>> +enum OperandKind { ArgumentOperand, LocalOperand, TmpOperand };
> 
> why not "enum class OperandKind {Argument, Local, Tmp}"?

Sure!

>> Source/JavaScriptCore/bytecode/Operands.h:140
>> +        m_values.fill(initialValue);
> 
> why this change?

I think because FastBitVector didn't support it. I can make it work if you want but it probably doesn't matter for perf.

>> Source/JavaScriptCore/bytecode/Operands.h:240
>> +                m_values[localIndex(oldNumLocals + i)] = ensuredValue;
> 
> this seems like a semantic change and slightly sketchy you're not constructing a new one of each entry. I think you should keep this "= T()", and let the compiler optimize away

This only runs if ensuredValue != T(). And if it doesn't need initialization it presumably has a reasonable copy constructor.

>> Source/JavaScriptCore/bytecode/Operands.h:244
>> +    void ensureTmps(size_t size, const T& ensuredValue = T())
> 
> same comment

Ditto.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3233
>> +        ASSERT(m_codeBlock->hasCheckpoints());
> 
> we don't have check points for tail call forward varargs too?

No, because when "forwarding varargsing" getting the length is not observable.

>> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:956
>> +                                unsigned frameArgumentCount = static_cast<unsigned>(inlineCallFrame->argumentCountIncludingThis - 1);
> 
> this is needed?

Yeah, GCC seems to have a weird bug where it treats all bitfields as signed so the rhs becomes an int and we get an error. Although, this particular one might be unnecessary.

>> Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:65
>> +    InsertionSet& insertionSet, Node* arguments, unsigned nodeIndex, NodeOrigin origin, bool addThis)
> 
> style nit: This should be an enum, not bool, according to WK style

It's not in the style guide! https://webkit.org/code-style-guidelines/ :P

>> Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:146
>> +            insertionSet.insertConstantForUse(nodeIndex, origin, jsNumber(addThis), Int32Use));
> 
> nit: jsNumber(static_cast<int32_t>(addThis)) so people don't go crazy reading this LOC

Sure.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:285
>> +    void progressCheckpoint()
> 
> nit: progressCheckpoint => progressToNextCheckpoint?

Sure.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:423
>> +    Node* getLocalOrTmp(Operand operand)
> 
> maybe file a bug on a rename here? Should we name the node Get or GetOperand instead of keeping it GetLocal? (Same w/ SetLocal)

Sure. https://bugs.webkit.org/show_bug.cgi?id=205489

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:635
>> +        ASSERT(operand.isTmp() || !operand.virtualRegister().isConstant());
> 
> nit: maybe just add an isConstant() to Operand that is this logic?

Done.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1130
>> +    // The max number of temps used for forwarding data to an OSR exit to checkpoint.
> 
> "to an OSR exit to checkpoint"? Did you mean to have the second "to"?

Fixed.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1963
>> +            setDirect(argCountTmp, addToGraph(VarargsLength, OpInfo(data), arguments));
> 
> can we have a version that doesn't require us to call setDirect? Like a set that does remap for us?

That seems reasonable but can we do it in a later patch? I'm not sure complicating this patch more is worthwhile. https://bugs.webkit.org/show_bug.cgi?id=205490

>> Source/JavaScriptCore/dfg/DFGCPSRethreadingPhase.cpp:497
>> +        RELEASE_ASSERT_NOT_REACHED();
> 
> is this needed? Wouldn't the switch be a compile error?

Not on MSVC!

>> Source/JavaScriptCore/dfg/DFGClobberize.h:818
>> +        write(AbstractHeap(Stack, data->count));
> 
> I thought you said we don't do this anymore?

Whoops, will remove.

>> Source/JavaScriptCore/dfg/DFGCommonData.cpp:59
>> +    return DisposableCallSiteIndex(index);
> 
> why all these DisposableCallSiteIndex changes? How is that right?

DisposableCallSiteIndex is what I'm using to refer to non-bytecode offset CallSites. Maybe that should be a different class from Disposable?

>> Source/JavaScriptCore/dfg/DFGForAllKills.h:78
>> +        if (after.bytecodeIndex().checkpoint()) {
> 
> the code inside this "if" is assuming that these checkpoints belong to the same bytecode offset. Is that an ok assumption to make? Maybe assert that?
> 
> I think this is an ok assumption, since when the DFG does code motion, it changes forExit origin, and this code is using forExit

That was my thinking as well but an assert seems reasonable.

>> Source/JavaScriptCore/dfg/DFGGraph.cpp:1222
>>      RELEASE_ASSERT_NOT_REACHED();
> 
> what's going on here? Looks wrong...

Whoops, I reorganized the loop but forgot to delete the now unreachable RELEASE_ASSERT_NOT_REACHED().

>> Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp:181
>> +                flush(Operand(TmpOperand, tmp));
> 
> nit: You use this constructor in a few places, would be nice to just have "Operand::tmp(tmp)"

Fair enough. Added.

>> Source/JavaScriptCore/dfg/DFGNodeType.h:77
>> +    /* These are used in SSA form to represent to track */\
> 
> track?

Whoops was going to add comments on what these did but I didn't get around to it. Will do in a follow up patch.

>> Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp:246
>> +    }
> 
> why?

Because this doesn't set any side state. The tmp is set by the subsequent MovHint.

>> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:641
>> +                addSideState(reinterpret_cast<CallFrame*>(reinterpret_cast<char*>(callFrame) + inlineCallFrame->returnPCOffset() - sizeof(CPURegister)), callBytecodeIndex, inlineCallFrame->tmpOffset);
> 
> there isn't an offset just for call frame?

Not one that works here that I know of. There's CallFrame::returnPCOffset() but that doesn't handle the inline call frame offset.

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:2957
>> +    for (uint32_t i = length - 1; i < mandatoryMinimum; ++i)
> 
> why - 1 now?

Because we're handing the length + this now. I'll change the variable name.

>> Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:524
>> +                        RELEASE_ASSERT(node->op() == VarargsLength || node->op() == LoadVarargs || node->op() == ForwardVarargs);
> 
> as I said in clobberize comment, does this actually write to that stack slot?

No, fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9572
>> +            m_out.bitOr(m_out.isZero32(lengthIncludingThis), m_out.above(lengthIncludingThis, m_out.constInt32(data->limit)))); 
> 
> why would it be zero? DFG doesn't seem to do this?

Because VarargsLength could have overflowed so you'd wrap around to zero. Yeah, DFG does the same logic but just as two separate speculations.

>> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:488
>> +                    sideState->tmps[i] = tmpScratch[i + tmpOffset];
> 
> these are already boxed? This seems different than in the DFG, where you had to do value recovery. Is that right? Why not make the DFG also put the boxed value in the buffer before you read it out, so that DFG OSR exit code is cleaner?

Yeah, the FTL does it the right way. I'll add a FIXME to switch the DFG to using the same system as the FTL. The code could probably all be deduplicated anyway.

>> Source/JavaScriptCore/interpreter/CallFrame.cpp:108
>> +    return CallSiteIndex::fromBits(unsafeCallSiteAsRawBits() & (std::numeric_limits<uint32_t>::max() >> BytecodeIndex::checkpointShift));
> 
> why not construct a BytecodeIndex and then call the helper function?
> 
> This looks wrong anyways. I think you meant to mask off the checkpoint index. But that begs the question, why do you need to do that? That's wrong for DFG/FTL CallSiteIndices, right? Those are just indices into an array that we bound check in the sampling profiler.

Yeah, I think this will give the wrong line of code. I fixed it. I also think the OSR exit code will put the wrong bytecode index into the CallFrame so I fixed that too.

>> Source/JavaScriptCore/interpreter/CheckpointOSRExitSideState.h:37
>> +    T getTmp(size_t offset) 
> 
> his is only used for JSValue. Let's just make this return JSValue.

Sure.

>> Source/JavaScriptCore/interpreter/CheckpointOSRExitSideState.h:52
>> +    void* nextInstructionJumpTarget;
> 
> this is unused

Removed.

>> Source/JavaScriptCore/interpreter/Interpreter.cpp:545
>> +            // FIXME: We should support exception handling in checkpoints.
> 
> hat do you mean by this?
> Please file a bug if it's important

We don't support a catch block inside a single bytecode right now. We will probably have to do this for iteration_close in the future anyway.

>> Source/JavaScriptCore/jit/JITExceptions.cpp:53
>> +        WTFBreakpointTrap();
> 
> why?

Because CRASH() doesn't do a breakpoint, it loads 0xbbadbeef... :/

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1969
>> +    unsigned argumentCountIncludingThis = sideState.getTmp<JSValue>(Opcode::argCount).asUInt32();
> 
> let's just make getTmp return JSValue
> 
> Also, let's rename argCount to argCount to argCountIncludingThis for consistency

Done.

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1973
>> +        loadVarargs(globalObject, buffer, callFrame->r(bytecode.m_arguments).jsValue(), 0, argumentCountIncludingThis - 1);
> 
> is zero the right offset here? Isn't it based on something in the opcode?

Oh, yeah, that's right. I'll add a test where we do return foo.call(...args); in strict mode then OSR exit due to the length.

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1992
>> +inline SlowPathReturnType dispatchNextInstruction(CodeBlock* codeBlock, InstructionStream::Ref pc)
> 
> nit: I'd name this "dispatchToNextInstruction"?

Done.

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:2064
>> +        ASSERT_WITH_MESSAGE(pc.next()->opcodeID() == op_ret, "We strongly assume all tail calls are followed by an op_ret.");
> 
> why does it really matter though? I don't see how it changes this code

It does matter because we dispatch to the next bytecode so if it's not a ret then we will be running whatever random bytecode is after the tail call.

>> Source/JavaScriptCore/runtime/VM.cpp:1031
>> +void VM::addCheckpointOSRSideState(CallFrame* callFrame, std::unique_ptr<CheckpointOSRExitSideState>&& payload)
> 
> Don't you need to visit these things?

Yes. Added during Heap::gatherConservativeRoots()

[Keith Miller]

Created attachment 386352 [details]
Patch

[Saam Barati]

Comment on attachment 386118 [details]
Patch

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

>>> Source/JavaScriptCore/dfg/DFGCommonData.cpp:59
>>> +    return DisposableCallSiteIndex(index);
>> 
>> why all these DisposableCallSiteIndex changes? How is that right?
> 
> DisposableCallSiteIndex is what I'm using to refer to non-bytecode offset CallSites. Maybe that should be a different class from Disposable?

Yeah this change looks wrong since callsiteindex is an index into a vector in DFG/FTL. Disposable is meant for things that go away, like ICs

[Keith Miller]

Committed r253896: <https://trac.webkit.org/changeset/253896>

[Radar WebKit Bug Importer]

<rdar://problem/58171565>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 205582

[Alexey Proskuryakov]

Keith fixed the build in https://trac.webkit.org/r253903, we didn't roll back.

[Paulo Matos]

This patch unfortunately breaks the build on both arm and mips.
The problem is related with a non-existing function:

../../Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:338:93: error: no matching function for call to ‘JSC::CodeBlock::instructions(JSC::BytecodeIndex&)’
         const Instruction* instruction = jit.baselineCodeBlock()->instructions(bytecodeIndex).at().ptr();

[Keith Miller]

(In reply to Paulo Matos from comment #32)
> This patch unfortunately breaks the build on both arm and mips.
> The problem is related with a non-existing function:
> 
> ../../Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:338:93: error:
> no matching function for call to
> ‘JSC::CodeBlock::instructions(JSC::BytecodeIndex&)’
>          const Instruction* instruction =
> jit.baselineCodeBlock()->instructions(bytecodeIndex).at().ptr();

Oh, whoops, the bytecodeIndex needs to be the parameter to the at() not the instructions() call.

[Fujii Hironori]

Reopened. This was reverted by r254632 (Bug 206301).

[Caio Lima]

(In reply to Fujii Hironori from comment #34)
> Reopened. This was reverted by r254632 (Bug 206301).

I think this was relanded on https://bugs.webkit.org/show_bug.cgi?id=206361.

[Fujii Hironori]

(In reply to Caio Lima from comment #35)

Oops. Thanks.