Bug 238535 – [JSC] Store CodeBlock in caller side

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

Let's not store it in the callee side since the caller typically already knows well about it.
It makes unlinked code simpler.

[Yusuke Suzuki]

Created attachment 456088 [details]
Patch

[Yusuke Suzuki]

Created attachment 456103 [details]
Patch

[Yusuke Suzuki]

Created attachment 456110 [details]
Patch

[Yusuke Suzuki]

Created attachment 456157 [details]
Patch

[Yusuke Suzuki]

Created attachment 456176 [details]
Patch

[Yusuke Suzuki]

Created attachment 456193 [details]
Patch

[Yusuke Suzuki]

Created attachment 456199 [details]
Patch

[Yusuke Suzuki]

Created attachment 456303 [details]
Patch

[Yusuke Suzuki]

Created attachment 456307 [details]
Patch

[Yusuke Suzuki]

Created attachment 456317 [details]
Patch

[Yusuke Suzuki]

Created attachment 456325 [details]
Patch

[Yusuke Suzuki]

Created attachment 456395 [details]
Patch

[Yusuke Suzuki]

Created attachment 456447 [details]
Patch

[Yusuke Suzuki]

Created attachment 456642 [details]
Patch

[Saam Barati]

Comment on attachment 456642 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:9
> +        This helps unlinked Baseline and DFG since we no longer need to load CodeBlock from callee via costly dependent loads: unlinked ones

and LLInt

> Source/JavaScriptCore/jit/JIT.cpp:754
> +#if ASSERT_ENABLED
> +        probeDebug([=](Probe::Context& ctx) {
> +            CodeBlock* codeBlock = ctx.fp<CallFrame*>()->codeBlock();
> +            if (codeBlock->jitType() != JITType::BaselineJIT) {
> +                dataLogLn("FP ", RawPointer(ctx.fp<CallFrame*>()));
> +                RELEASE_ASSERT_NOT_REACHED();
> +            }
> +        });
> +#endif

Why do we only do this release assert for function code? It should always be correct. Same for DFG/FTL

> Source/JavaScriptCore/runtime/FunctionExecutable.h:302
> +        return 0;

can you RELEASE_ASSERT_NOT_REACHED here?

[Saam Barati]

Comment on attachment 456642 [details]
Patch

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

>> Source/JavaScriptCore/jit/JIT.cpp:754
>> +#endif
> 
> Why do we only do this release assert for function code? It should always be correct. Same for DFG/FTL

My unlinked baseline patch might've done things this way. But I think it's worth doing always.

[Yusuke Suzuki]

Comment on attachment 456642 [details]
Patch

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

Thank you!

>> Source/JavaScriptCore/ChangeLog:9
>> +        This helps unlinked Baseline and DFG since we no longer need to load CodeBlock from callee via costly dependent loads: unlinked ones
> 
> and LLInt

Fixed.

>>> Source/JavaScriptCore/jit/JIT.cpp:754
>>> +#endif
>> 
>> Why do we only do this release assert for function code? It should always be correct. Same for DFG/FTL
> 
> My unlinked baseline patch might've done things this way. But I think it's worth doing always.

Sounds good! Changed.

>> Source/JavaScriptCore/runtime/FunctionExecutable.h:302
>> +        return 0;
> 
> can you RELEASE_ASSERT_NOT_REACHED here?

Nice, fixed.

[Yusuke Suzuki]

Created attachment 456661 [details]
Patch

[Yusuke Suzuki]

Created attachment 456664 [details]
Patch

[Yusuke Suzuki]

Committed r292372 (249237@trunk): <https://commits.webkit.org/249237@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/91276034>

[Michael Catanzaro]

This broke s390x, see bug #238956