Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JSC] Always create StructureStubInfo for op_get_by_val #1565

Conversation

Constellation
Copy link
Member

@Constellation Constellation commented Jun 16, 2022

3576f09

[JSC] Always create StructureStubInfo for op_get_by_val
https://bugs.webkit.org/show_bug.cgi?id=241669
rdar://75146284

Reviewed by Saam Barati and Mark Lam.

DFG OSR exit requires StructureStubInfo for getter / setter calls. However very generic baseline JIT
op_get_by_val does not create StructureStubInfo. It is possible that OSR exit crashes because of this
missing StructureStubInfo. Let's consider the following edge case.

1. Now, Baseline detects that this is very generic op_get_by_val. So we do not create StructureStubInfo.
2. This function is inlined in DFG. And DFG emits IC for this GetByVal.
3. (2)'s DFG function collects information in DFG-level IC. And luckily, in this inlined call path, it was not so generic.
4. Then, due to different OSR exit or something, we recreate DFG code for this function with (2)'s inlining.
5. DFG detects that DFG-level IC has more specialized information. So it can inline getter call in this op_get_by_val.
6. Inside this getter, we perform OSR exit.
7. Looking into Baseline, and we found that there is no StructureStubInfo!

We always create StructureStubInfo. In very generic op_get_by_val case, we create this with tookSlowPath = true.
And we emit empty inline path to record doneLocation. So, OSR exit can jump to this place.

We also clean up StructureStubInfo code.

1. "start" is renamed to startLocation. And we do not record it in DataIC case since it is not necessary.
2. Rename inlineSize to inlineCodeSize.
3. Add some assertions to ensure that this path is not used for DataIC case.
4. We also record opcode value in the crashing RELEASE_ASSERT to get more information if this does not fix the issue.

* Source/JavaScriptCore/bytecode/InlineAccess.cpp:
(JSC::linkCodeInline):
(JSC::InlineAccess::generateArrayLength):
(JSC::InlineAccess::generateStringLength):
(JSC::InlineAccess::rewireStubAsJumpInAccessNotUsingInlineAccess):
(JSC::InlineAccess::rewireStubAsJumpInAccess):
(JSC::InlineAccess::resetStubAsJumpInAccess):
* Source/JavaScriptCore/bytecode/StructureStubInfo.cpp:
(JSC::StructureStubInfo::initializeFromUnlinkedStructureStubInfo):
(JSC::StructureStubInfo::initializeFromDFGUnlinkedStructureStubInfo):
* Source/JavaScriptCore/bytecode/StructureStubInfo.h:
(JSC::StructureStubInfo::inlineCodeSize const):
(JSC::StructureStubInfo::inlineSize const): Deleted.
* Source/JavaScriptCore/dfg/DFGInlineCacheWrapperInlines.h:
(JSC::DFG::InlineCacheWrapper<GeneratorType>::finalize):
* Source/JavaScriptCore/dfg/DFGJITCode.h:
* Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:
(JSC::DFG::callerReturnPC):
* Source/JavaScriptCore/jit/JIT.cpp:
(JSC::JIT::link):
* Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:
(JSC::JITInlineCacheGenerator::finalize):
(JSC::JITGetByValGenerator::generateEmptyPath):
* Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:
* Source/JavaScriptCore/jit/JITPropertyAccess.cpp:
(JSC::JIT::emit_op_get_by_val):
* JSTests/stress/get-by-val-generic-structurestubinfo.js: Added.
(let.program):
(runMono.let.o.get x):
(runMono):
(runPoly):

Canonical link: https://commits.webkit.org/251619@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295614 268f45cc-cd09-0410-ab3c-d52691b4dbfc

@Constellation Constellation requested a review from a team as a code owner June 16, 2022 01:56
@Constellation Constellation self-assigned this Jun 16, 2022
@Constellation Constellation added JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. WebKit Nightly Build labels Jun 16, 2022
@webkit-early-warning-system webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label Jun 16, 2022
@lauromoura
Copy link
Contributor

Sample trace from a EWS GTK crash for this:

Thread 1 (Thread 0x7fcecbfff640 (LWP 39990)):
#0  0x00007fcf384e8e0e in WTFCrash () at /app/webkit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.1.so.0
#1  0x00007fcf37c35605 in JSC::JIT::link() () at /app/webkit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.1.so.0
#2  0x00007fcf37c370b9 in JSC::JIT::compileAndLinkWithoutFinalizing(JSC::JITCompilationEffort) () at /app/webkit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.1.so.0
#3  0x00007fcf37c0000f in JSC::BaselineJITPlan::compileInThreadImpl() () at /app/webkit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.1.so.0
#4  0x00007fcf37ce342a in JSC::JITPlan::compileInThread(JSC::JITWorklistThread*) () at /app/webkit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.1.so.0
#5  0x00007fcf37d21663 in JSC::JITWorklistThread::work() () at /app/webkit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.1.so.0
#6  0x00007fcf384eb1c5 in WTF::Detail::CallableWrapper<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::{lambda()#1}, void>::call() () at /app/webkit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.1.so.0
#7  0x00007fcf3851857a in WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) () at /app/webkit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.1.so.0
#8  0x00007fcf3858c1c9 in WTF::wtfThreadEntryPoint(void*) () at /app/webkit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.1.so.0
#9  0x00007fcf347ee3ba in start_thread (arg=0x7fcecbfff640) at pthread_create.c:481
#10 0x00007fcf34285953 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

@Constellation Constellation removed merging-blocked Applied to prevent a change from being merged JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. WebKit Nightly Build labels Jun 16, 2022
@Constellation Constellation force-pushed the eng/JSC-Always-create-StructureStubInfo-for-op_get_by_val branch from 12133e7 to a91e4d7 Compare June 16, 2022 18:42
@Constellation Constellation added JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. WebKit Nightly Build labels Jun 16, 2022
@Constellation Constellation force-pushed the eng/JSC-Always-create-StructureStubInfo-for-op_get_by_val branch from a91e4d7 to 98e5ba1 Compare June 16, 2022 18:46
@saambarati
Copy link
Contributor

Found a test case:

let program = `
    function foo(o, p) {
        return o[p];
    }
    noInline(foo);

    function runMono() {
        let o = {
            get x() {
                if ($vm.ftlTrue()) OSRExit(); 
                return 42; 
            }
        };
        for (let i = 0; i < 1000000; ++i) {
            foo(o, "x") ;
        }
    }

    function runPoly() {
        let o = {
            a: 1,
            b: 2,
            c: 4,
            d: 4,
            e: 4,
            f: 4,
            g: 4,
        };
        for (let i = 0; i < 1000000; ++i) {
            foo(o, "a");
            foo(o, "b");
            foo(o, "c");
            foo(o, "d");
            foo(o, "e");
            foo(o, "f");
            foo(o, "g");
            foo(o, "h");
            foo(o, "i");
        }
    }
`;

let g1 = runString(program);
g1.runPoly();

let g2 = runString(program);
g2.runMono();

run with JSC_getByValICMaxNumberOfIdentifiers=2 JSC_useConcurrentJIT=0

@Constellation Constellation force-pushed the eng/JSC-Always-create-StructureStubInfo-for-op_get_by_val branch 2 times, most recently from 078dcf7 to 37e9b85 Compare June 16, 2022 20:49
@Constellation Constellation force-pushed the eng/JSC-Always-create-StructureStubInfo-for-op_get_by_val branch from 37e9b85 to d58a847 Compare June 16, 2022 20:53
Copy link
Contributor

@saambarati saambarati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Source/JavaScriptCore/jit/JITPropertyAccess.cpp Outdated Show resolved Hide resolved
Copy link

@MenloDorian MenloDorian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me too

Source/JavaScriptCore/jit/JITPropertyAccess.cpp Outdated Show resolved Hide resolved
Source/JavaScriptCore/jit/JITPropertyAccess.cpp Outdated Show resolved Hide resolved
@Constellation Constellation force-pushed the eng/JSC-Always-create-StructureStubInfo-for-op_get_by_val branch from d58a847 to 4c91626 Compare June 16, 2022 21:26
@Constellation Constellation added the merge-queue Applied to send a pull request to merge-queue label Jun 16, 2022
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/JSC-Always-create-StructureStubInfo-for-op_get_by_val branch from 4c91626 to 3576f09 Compare June 16, 2022 23:09
@webkit-early-warning-system webkit-early-warning-system merged commit 3576f09 into WebKit:main Jun 16, 2022
@webkit-early-warning-system
Copy link
Collaborator

Committed r295614 (251619@main): https://commits.webkit.org/251619@main

Reviewed commits have been landed. Closing PR #1565 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system removed the merge-queue Applied to send a pull request to merge-queue label Jun 16, 2022
@Constellation Constellation deleted the eng/JSC-Always-create-StructureStubInfo-for-op_get_by_val branch June 16, 2022 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants