Skip to content

Commit

Permalink
[JSC] We should have accept-any-value case generation for IC
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=259327
rdar://112502090

Reviewed by Michael Saboff.

When we generate IC for get-by-val / get-by-val-with-this, we check whether each IC needs Int32 / String / Symbol checks.
And if we find some of IC case requires it, then we do this check and generating code. But we are missing that we generate
accept-any-value case in this path (which is IndexedProxyObjectLoad). This is clearly wrong, and attached script is repeatedly
compiling IC because we are not generating IndexedProxyObjectLoad case.
And if this IC site is requiring some register spills, then it leads to release-assert-crash because

    1. It says doesJSCalls = true
    2. But not setting spillStateForJSCall

So, we will encounter empty spillStateForJSCall.
It is actually super hard to reproduce this issue, and we cannot find a case. But anyway, this fixes the obvious issue, which is
not generating listed IC, which is tested in the attached test.

* JSTests/stress/proxy-get-with-complex-string.js: Added.
(test):
* Source/JavaScriptCore/bytecode/InlineCacheCompiler.cpp:
(JSC::InlineCacheCompiler::regenerate):

Canonical link: https://commits.webkit.org/266164@main
  • Loading branch information
Constellation committed Jul 19, 2023
1 parent c7f1348 commit eaa5055
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 12 deletions.
11 changes: 11 additions & 0 deletions JSTests/stress/proxy-get-with-complex-string.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
function test(object, value) {
return object[value];
}
noInline(test);

var proxy = new Proxy({}, {});
var object = { hello: 42 };
for (var i = 0; i < 1e6; ++i) {
test(proxy, "hello");
test(object, "hello");
}
40 changes: 28 additions & 12 deletions Source/JavaScriptCore/bytecode/InlineCacheCompiler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2965,6 +2965,7 @@ AccessGenerationResult InlineCacheCompiler::regenerate(const GCSafeConcurrentJSL
switch (entry->type()) {
case AccessCase::Getter:
case AccessCase::Setter:
case AccessCase::ProxyObjectHas:
case AccessCase::ProxyObjectLoad:
case AccessCase::ProxyObjectStore:
case AccessCase::IndexedProxyObjectLoad:
Expand Down Expand Up @@ -2995,6 +2996,7 @@ AccessGenerationResult InlineCacheCompiler::regenerate(const GCSafeConcurrentJSL
bool needsInt32PropertyCheck = false;
bool needsStringPropertyCheck = false;
bool needsSymbolPropertyCheck = false;
bool acceptValueProperty = false;
for (auto& newCase : cases) {
if (!m_stubInfo->hasConstantIdentifier) {
if (newCase->requiresIdentifierNameMatch()) {
Expand All @@ -3004,6 +3006,8 @@ AccessGenerationResult InlineCacheCompiler::regenerate(const GCSafeConcurrentJSL
needsStringPropertyCheck = true;
} else if (newCase->requiresInt32PropertyCheck())
needsInt32PropertyCheck = true;
else
acceptValueProperty = true;
}
commit(locker, vm(), m_watchpoints, codeBlock, *m_stubInfo, *newCase);
allGuardedByStructureCheck &= newCase->guardedByStructureCheck(*m_stubInfo);
Expand Down Expand Up @@ -3054,13 +3058,13 @@ AccessGenerationResult InlineCacheCompiler::regenerate(const GCSafeConcurrentJSL
CCallHelpers::JumpList fallThrough;
if (needsInt32PropertyCheck || needsStringPropertyCheck || needsSymbolPropertyCheck) {
if (needsInt32PropertyCheck) {
CCallHelpers::Jump notInt32;
CCallHelpers::JumpList notInt32;

if (!m_stubInfo->propertyIsInt32) {
#if USE(JSVALUE64)
notInt32 = jit.branchIfNotInt32(m_stubInfo->propertyGPR());
notInt32.append(jit.branchIfNotInt32(m_stubInfo->propertyGPR()));
#else
notInt32 = jit.branchIfNotInt32(m_stubInfo->propertyTagGPR());
notInt32.append(jit.branchIfNotInt32(m_stubInfo->propertyTagGPR()));
#endif
}
JIT_COMMENT(jit, "Cases start (needsInt32PropertyCheck)");
Expand All @@ -3071,15 +3075,12 @@ AccessGenerationResult InlineCacheCompiler::regenerate(const GCSafeConcurrentJSL
generateWithGuard(*cases[i], fallThrough);
}

if (needsStringPropertyCheck || needsSymbolPropertyCheck) {
if (notInt32.isSet())
notInt32.link(&jit);
if (needsStringPropertyCheck || needsSymbolPropertyCheck || acceptValueProperty) {
notInt32.link(&jit);
fallThrough.link(&jit);
fallThrough.clear();
} else {
if (notInt32.isSet())
m_failAndRepatch.append(notInt32);
}
} else
m_failAndRepatch.append(notInt32);
}

if (needsStringPropertyCheck) {
Expand Down Expand Up @@ -3107,7 +3108,7 @@ AccessGenerationResult InlineCacheCompiler::regenerate(const GCSafeConcurrentJSL
generateWithGuard(*cases[i], fallThrough);
}

if (needsSymbolPropertyCheck) {
if (needsSymbolPropertyCheck || acceptValueProperty) {
notString.link(&jit);
fallThrough.link(&jit);
fallThrough.clear();
Expand Down Expand Up @@ -3136,7 +3137,22 @@ AccessGenerationResult InlineCacheCompiler::regenerate(const GCSafeConcurrentJSL
generateWithGuard(*cases[i], fallThrough);
}

m_failAndRepatch.append(notSymbol);
if (acceptValueProperty) {
notSymbol.link(&jit);
fallThrough.link(&jit);
fallThrough.clear();
} else
m_failAndRepatch.append(notSymbol);
}

if (acceptValueProperty) {
JIT_COMMENT(jit, "Cases start (remaining)");
for (unsigned i = cases.size(); i--;) {
fallThrough.link(&jit);
fallThrough.clear();
if (!cases[i]->requiresIdentifierNameMatch() && !cases[i]->requiresInt32PropertyCheck())
generateWithGuard(*cases[i], fallThrough);
}
}
} else {
// Cascade through the list, preferring newer entries.
Expand Down

0 comments on commit eaa5055

Please sign in to comment.