Skip to content

Commit 830adb7

Browse files
author
Filip Pizlo
committed
FTL SwitchString slow case creates duplicate switch cases
https://bugs.webkit.org/show_bug.cgi?id=144634 Reviewed by Geoffrey Garen. The problem of duplicate switches is sufficiently annoying that I fixed the issue and also added mostly-debug-only asserts to catch such issues earlier. * bytecode/CallVariant.cpp: (JSC::variantListWithVariant): Assertion to prevent similar bugs. * ftl/FTLLowerDFGToLLVM.cpp: (JSC::FTL::LowerDFGToLLVM::switchStringRecurse): Assertion to prevent similar bugs. (JSC::FTL::LowerDFGToLLVM::switchStringSlow): This is the bug. * jit/BinarySwitch.cpp: (JSC::BinarySwitch::BinarySwitch): Assertion to prevent similar bugs. * jit/Repatch.cpp: (JSC::linkPolymorphicCall): Assertion to prevent similar bugs. * tests/stress/ftl-switch-string-slow-duplicate-cases.js: Added. This tests the FTL SwitchString bug. It was previously crashing every time. (foo): (cat): Canonical link: https://commits.webkit.org/162660@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@183825 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 89fa075 commit 830adb7

6 files changed

Lines changed: 135 additions & 3 deletions

File tree

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,26 @@
1+
2015-05-05 Filip Pizlo <fpizlo@apple.com>
2+
3+
FTL SwitchString slow case creates duplicate switch cases
4+
https://bugs.webkit.org/show_bug.cgi?id=144634
5+
6+
Reviewed by Geoffrey Garen.
7+
8+
The problem of duplicate switches is sufficiently annoying that I fixed the issue and also
9+
added mostly-debug-only asserts to catch such issues earlier.
10+
11+
* bytecode/CallVariant.cpp:
12+
(JSC::variantListWithVariant): Assertion to prevent similar bugs.
13+
* ftl/FTLLowerDFGToLLVM.cpp:
14+
(JSC::FTL::LowerDFGToLLVM::switchStringRecurse): Assertion to prevent similar bugs.
15+
(JSC::FTL::LowerDFGToLLVM::switchStringSlow): This is the bug.
16+
* jit/BinarySwitch.cpp:
17+
(JSC::BinarySwitch::BinarySwitch): Assertion to prevent similar bugs.
18+
* jit/Repatch.cpp:
19+
(JSC::linkPolymorphicCall): Assertion to prevent similar bugs.
20+
* tests/stress/ftl-switch-string-slow-duplicate-cases.js: Added. This tests the FTL SwitchString bug. It was previously crashing every time.
21+
(foo):
22+
(cat):
23+
124
2015-05-05 Basile Clement <basile_clement@apple.com>
225

326
Fix debug builds after r183812

‎Source/JavaScriptCore/bytecode/CallVariant.cpp‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
#include "CallVariant.h"
2828

2929
#include "JSCInlines.h"
30+
#include <wtf/ListDump.h>
3031

3132
namespace JSC {
3233

@@ -68,6 +69,19 @@ CallVariantList variantListWithVariant(const CallVariantList& list, CallVariant
6869
}
6970
if (!!variantToAdd)
7071
result.append(variantToAdd);
72+
73+
if (!ASSERT_DISABLED) {
74+
for (unsigned i = 0; i < result.size(); ++i) {
75+
for (unsigned j = i + 1; j < result.size(); ++j) {
76+
if (result[i] != result[j])
77+
continue;
78+
79+
dataLog("variantListWithVariant(", listDump(list), ", ", variantToAdd, ") failed: got duplicates in result: ", listDump(result), "\n");
80+
RELEASE_ASSERT_NOT_REACHED();
81+
}
82+
}
83+
}
84+
7185
return result;
7286
}
7387

‎Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@
5252
#include <atomic>
5353
#include <dlfcn.h>
5454
#include <llvm/InitializeLLVM.h>
55+
#include <unordered_set>
5556
#include <wtf/ProcessID.h>
5657

5758
#if ENABLE(FTL_NATIVE_CALL_INLINING)
@@ -6476,6 +6477,8 @@ class LowerDFGToLLVM {
64766477

64776478
Vector<SwitchCase> switchCases;
64786479
for (unsigned i = 0; i < characterCases.size(); ++i) {
6480+
if (i)
6481+
DFG_ASSERT(m_graph, m_node, characterCases[i - 1].character < characterCases[i].character);
64796482
switchCases.append(SwitchCase(
64806483
m_out.constInt8(characterCases[i].character), characterBlocks[i], Weight()));
64816484
}
@@ -6506,12 +6509,47 @@ class LowerDFGToLLVM {
65066509
StringJumpTable& table = codeBlock()->stringSwitchJumpTable(data->switchTableIndex);
65076510

65086511
Vector<SwitchCase> cases;
6512+
std::unordered_set<int32_t> alreadyHandled; // These may be negative, or zero, or probably other stuff, too. We don't want to mess with HashSet's corner cases and we don't really care about throughput here.
65096513
for (unsigned i = 0; i < data->cases.size(); ++i) {
6514+
// FIXME: The fact that we're using the bytecode's switch table means that the
6515+
// following DFG IR transformation would be invalid.
6516+
//
6517+
// Original code:
6518+
// switch (v) {
6519+
// case "foo":
6520+
// case "bar":
6521+
// things();
6522+
// break;
6523+
// default:
6524+
// break;
6525+
// }
6526+
//
6527+
// New code:
6528+
// switch (v) {
6529+
// case "foo":
6530+
// instrumentFoo();
6531+
// goto _things;
6532+
// case "bar":
6533+
// instrumentBar();
6534+
// _things:
6535+
// things();
6536+
// break;
6537+
// default:
6538+
// break;
6539+
// }
6540+
//
6541+
// Luckily, we don't currently do any such transformation. But it's kind of silly that
6542+
// this is an issue.
6543+
// https://bugs.webkit.org/show_bug.cgi?id=144635
6544+
65106545
DFG::SwitchCase myCase = data->cases[i];
65116546
StringJumpTable::StringOffsetTable::iterator iter =
65126547
table.offsetTable.find(myCase.value.stringImpl());
65136548
DFG_ASSERT(m_graph, m_node, iter != table.offsetTable.end());
65146549

6550+
if (!alreadyHandled.insert(iter->value.branchOffset).second)
6551+
continue;
6552+
65156553
cases.append(SwitchCase(
65166554
m_out.constInt32(iter->value.branchOffset),
65176555
lowBlock(myCase.target.block), Weight(myCase.target.count)));

‎Source/JavaScriptCore/jit/BinarySwitch.cpp‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,12 @@ BinarySwitch::BinarySwitch(GPRReg value, const Vector<int64_t>& cases, Type type
4646

4747
for (unsigned i = 0; i < cases.size(); ++i)
4848
m_cases.append(Case(cases[i], i));
49+
4950
std::sort(m_cases.begin(), m_cases.end());
51+
52+
for (unsigned i = 1; i < m_cases.size(); ++i)
53+
RELEASE_ASSERT(m_cases[i - 1] < m_cases[i]);
54+
5055
build(0, false, m_cases.size());
5156
}
5257

‎Source/JavaScriptCore/jit/Repatch.cpp‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
#include "StructureRareDataInlines.h"
5050
#include "StructureStubClearingWatchpoint.h"
5151
#include "ThunkGenerators.h"
52+
#include <wtf/CommaPrinter.h>
5253
#include <wtf/ListDump.h>
5354
#include <wtf/StringPrintStream.h>
5455

@@ -1828,15 +1829,35 @@ void linkPolymorphicCall(
18281829
if (callerCodeBlock->jitType() != JITCode::topTierJIT())
18291830
fastCounts = std::make_unique<uint32_t[]>(callCases.size());
18301831

1831-
for (size_t i = callCases.size(); i--;) {
1832+
for (size_t i = 0; i < callCases.size(); ++i) {
18321833
if (fastCounts)
18331834
fastCounts[i] = 0;
18341835

18351836
CallVariant variant = callCases[i].variant();
1837+
int64_t newCaseValue;
18361838
if (isClosureCall)
1837-
caseValues[i] = bitwise_cast<intptr_t>(variant.executable());
1839+
newCaseValue = bitwise_cast<intptr_t>(variant.executable());
18381840
else
1839-
caseValues[i] = bitwise_cast<intptr_t>(variant.function());
1841+
newCaseValue = bitwise_cast<intptr_t>(variant.function());
1842+
1843+
if (!ASSERT_DISABLED) {
1844+
for (size_t j = 0; j < i; ++j) {
1845+
if (caseValues[j] != newCaseValue)
1846+
continue;
1847+
1848+
dataLog("ERROR: Attempt to add duplicate case value.\n");
1849+
dataLog("Existing case values: ");
1850+
CommaPrinter comma;
1851+
for (size_t k = 0; k < i; ++k)
1852+
dataLog(comma, caseValues[k]);
1853+
dataLog("\n");
1854+
dataLog("Attempting to add: ", newCaseValue, "\n");
1855+
dataLog("Variant list: ", listDump(callCases), "\n");
1856+
RELEASE_ASSERT_NOT_REACHED();
1857+
}
1858+
}
1859+
1860+
caseValues[i] = newCaseValue;
18401861
}
18411862

18421863
GPRReg fastCountsBaseGPR =

‎Source/JavaScriptCore/tests/stress/ftl-switch-string-slow-duplicate-cases.js‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
function foo(s) {
2+
switch (s) {
3+
case "ƑẦǏŁ":
4+
case "ÌŅ":
5+
case "ṤĻŐⱲ":
6+
case "ṔÄȚĦ":
7+
return 42;
8+
case "due":
9+
case "to":
10+
case "16-bit":
11+
case "strings":
12+
return 43;
13+
default:
14+
return 44;
15+
}
16+
}
17+
18+
noInline(foo);
19+
20+
function cat(a, b) {
21+
return a + b;
22+
}
23+
24+
for (var i = 0; i < 10000; ++i) {
25+
var result = foo(cat("16-", "bit"));
26+
if (result != 43)
27+
throw "Error: bad result (1): " + result;
28+
result = foo("ƑẦǏŁ");
29+
if (result != 42)
30+
throw "Error: bad result (2): " + result;
31+
}

0 commit comments

Comments
 (0)