Skip to content

Commit

Permalink
Fix race condition for wasm referenced functions
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=250847
rdar://104332636

Reviewed by Mark Lam.

Fix race condition for updating referenced functions while compiling
wasm functions concurrently.

* JSTests/wasm/stress/referenced-function.js: Added.
(async let):
(j.catch):
* JSTests/wasm/stress/resources/funcref-race.wasm: Added.
* Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:
(JSC::Wasm::BBQPlan::compileFunction):
* Source/JavaScriptCore/wasm/WasmLLIntPlan.cpp:
(JSC::Wasm::LLIntPlan::didCompleteCompilation):
* Source/JavaScriptCore/wasm/WasmModuleInformation.h:
(JSC::Wasm::ModuleInformation::referencedFunctions const):
(JSC::Wasm::ModuleInformation::hasReferencedFunction const):
(JSC::Wasm::ModuleInformation::addReferencedFunction const):
* Source/WTF/WTF.xcodeproj/project.pbxproj:
* Source/WTF/wtf/CMakeLists.txt:
* Source/WTF/wtf/FixedBitVector.h: Added.
(WTF::FixedBitVector::concurrentAllocateOnce):
(WTF::FixedBitVector::concurrentTest):
(WTF::FixedBitVector::concurrentTestAndSet):
(WTF::FixedBitVector::testAndSet):
(WTF::FixedBitVector::test):
(WTF::FixedBitVector::findBit const):
(WTF::FixedBitVector::operator== const):
(WTF::FixedBitVector::hash const):
(WTF::FixedBitVector::get const):
(WTF::FixedBitVector::dump const):
(WTF::FixedBitVectorHash::hash):
(WTF::FixedBitVectorHash::equal):
* Tools/TestWebKitAPI/CMakeLists.txt:
* Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* Tools/TestWebKitAPI/Tests/WTF/FixedBitVector.cpp: Added.
(TestWebKitAPI::testFixedBitVectorSize):
(TestWebKitAPI::testFixedBitVectorTest):
(TestWebKitAPI::testFixedBitVectorTestAndSet):
(TestWebKitAPI::testFixedBitVectorConcurrentTest):
(TestWebKitAPI::testFixedBitVectorConcurrentTestAndSet):
(TestWebKitAPI::TEST):

Canonical link: https://commits.webkit.org/259323@main
  • Loading branch information
hyjorc1 committed Jan 25, 2023
1 parent b0db30d commit 527e2a0
Show file tree
Hide file tree
Showing 13 changed files with 508 additions and 6 deletions.
23 changes: 23 additions & 0 deletions JSTests/wasm/stress/referenced-function.js
@@ -0,0 +1,23 @@
let bytes = readFile('./resources/funcref-race.wasm', 'binary');
(async function () {
let importObject = {
m: {
ifn0() { },
ifn1() { },
ifn2() { },
t0: new WebAssembly.Table({ initial: 18, element: 'externref' }),
t1: new WebAssembly.Table({ initial: 84, element: 'anyfunc' }),
t2: new WebAssembly.Table({ initial: 2, element: 'externref' }),
t3: new WebAssembly.Table({ initial: 6, element: 'anyfunc' }),
t4: new WebAssembly.Table({ initial: 67, element: 'anyfunc', maximum: 579 }),
t5: new WebAssembly.Table({ initial: 39, element: 'externref', maximum: 690 }),
},
};
for (let j = 0; j < 10000; j++) {
try {
let i = await WebAssembly.instantiate(bytes, importObject);
i.instance.exports.fn10();
} catch (e) {
}
}
})();
Binary file added JSTests/wasm/stress/resources/funcref-race.wasm
Binary file not shown.
2 changes: 1 addition & 1 deletion Source/JavaScriptCore/wasm/WasmBBQPlan.cpp
Expand Up @@ -243,7 +243,7 @@ void BBQPlan::compileFunction(uint32_t functionIndex)
}
m_callees[functionIndex] = WTFMove(bbqCallee);

if (m_exportedFunctionIndices.contains(functionIndex) || m_moduleInformation->referencedFunctions().contains(functionIndex)) {
if (m_exportedFunctionIndices.contains(functionIndex) || m_moduleInformation->hasReferencedFunction(functionIndex)) {
Locker locker { m_lock };
TypeIndex typeIndex = m_moduleInformation->internalFunctionTypeIndices[functionIndex];
const TypeDefinition& signature = TypeInformation::get(typeIndex).expand();
Expand Down
2 changes: 1 addition & 1 deletion Source/JavaScriptCore/wasm/WasmLLIntPlan.cpp
Expand Up @@ -162,7 +162,7 @@ void LLIntPlan::didCompleteCompilation()
return;

for (uint32_t functionIndex = 0; functionIndex < m_moduleInformation->functions.size(); functionIndex++) {
if (m_exportedFunctionIndices.contains(functionIndex) || m_moduleInformation->referencedFunctions().contains(functionIndex)) {
if (m_exportedFunctionIndices.contains(functionIndex) || m_moduleInformation->hasReferencedFunction(functionIndex)) {
TypeIndex typeIndex = m_moduleInformation->internalFunctionTypeIndices[functionIndex];
const TypeDefinition& signature = TypeInformation::get(typeIndex).expand();
CCallHelpers jit;
Expand Down
12 changes: 8 additions & 4 deletions Source/JavaScriptCore/wasm/WasmModuleInformation.h
Expand Up @@ -30,7 +30,7 @@
#include "WasmBranchHints.h"
#include "WasmFormat.h"

#include <wtf/BitVector.h>
#include <wtf/FixedBitVector.h>
#include <wtf/HashMap.h>

namespace JSC { namespace Wasm {
Expand Down Expand Up @@ -91,8 +91,10 @@ struct ModuleInformation : public ThreadSafeRefCounted<ModuleInformation> {

const TableInformation& table(unsigned index) const { return tables[index]; }

const BitVector& referencedFunctions() const { return m_referencedFunctions; }
void addReferencedFunction(unsigned index) const { m_referencedFunctions.set(index); }
void initializeReferencedFunctionsTracker() const { m_referencedFunctions = FixedBitVector(functionIndexSpaceSize()); }
const FixedBitVector& referencedFunctions() const { return m_referencedFunctions; }
bool hasReferencedFunction(unsigned index) const { return m_referencedFunctions.test(index); }
void addReferencedFunction(unsigned index) const { m_referencedFunctions.concurrentTestAndSet(index); }

bool isDeclaredFunction(uint32_t index) const { return m_declaredFunctions.contains(index); }
void addDeclaredFunction(uint32_t index) { m_declaredFunctions.set(index); }
Expand Down Expand Up @@ -160,7 +162,9 @@ struct ModuleInformation : public ThreadSafeRefCounted<ModuleInformation> {

BitVector m_declaredFunctions;
BitVector m_declaredExceptions;
mutable BitVector m_referencedFunctions;
mutable FixedBitVector m_referencedFunctions;
// FIXME: We should use a synchronous mechanism for `m_clobberingTailCalls`
// to prevent potential race condition. https://bugs.webkit.org/show_bug.cgi?id=251124
BitVector m_clobberingTailCalls;
};

Expand Down
3 changes: 3 additions & 0 deletions Source/JavaScriptCore/wasm/WasmSectionParser.cpp
Expand Up @@ -232,6 +232,9 @@ auto SectionParser::parseFunction() -> PartialResult
m_info->functions.uncheckedAppend({ start, end, false, false, Vector<uint8_t>() });
}

// Note that `initializeReferencedFunctionsTracker` should only be used after both parseImport and parseFunction
// finish updating importFunctionTypeIndices and internalFunctionTypeIndices.
m_info->initializeReferencedFunctionsTracker();
return { };
}

Expand Down
4 changes: 4 additions & 0 deletions Source/WTF/WTF.xcodeproj/project.pbxproj
Expand Up @@ -836,6 +836,7 @@
FEDACD3D1630F83F00C69634 /* StackStats.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FEDACD3B1630F83F00C69634 /* StackStats.cpp */; };
FEEA4DF9216D7BE400AC0602 /* StackPointer.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FEEA4DF8216D7BE400AC0602 /* StackPointer.cpp */; };
FF895F11282AB96400E7BAE8 /* CompactRefPtr.h in Headers */ = {isa = PBXBuildFile; fileRef = FF895F10282AB96400E7BAE8 /* CompactRefPtr.h */; settings = {ATTRIBUTES = (Private, ); }; };
FF910E982979FE6F00D1A24D /* FixedBitVector.h in Headers */ = {isa = PBXBuildFile; fileRef = FF910E972979FE6F00D1A24D /* FixedBitVector.h */; settings = {ATTRIBUTES = (Private, ); }; };
FFEE46F42825DF80003BC981 /* CompactPtr.h in Headers */ = {isa = PBXBuildFile; fileRef = FFEE46F32825DF80003BC981 /* CompactPtr.h */; settings = {ATTRIBUTES = (Private, ); }; };
/* End PBXBuildFile section */

Expand Down Expand Up @@ -1766,6 +1767,7 @@
FEF295BF20B49DCB00CF283A /* UTF8ConversionError.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = UTF8ConversionError.h; sourceTree = "<group>"; };
FF0A436588954F3CB07DBECA /* StdList.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = StdList.h; sourceTree = "<group>"; };
FF895F10282AB96400E7BAE8 /* CompactRefPtr.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = CompactRefPtr.h; sourceTree = "<group>"; };
FF910E972979FE6F00D1A24D /* FixedBitVector.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = FixedBitVector.h; sourceTree = "<group>"; };
FFEE46F32825DF80003BC981 /* CompactPtr.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = CompactPtr.h; sourceTree = "<group>"; };
/* End PBXFileReference section */

Expand Down Expand Up @@ -2046,6 +2048,7 @@
0F9D335C165DBA73005AD387 /* FilePrintStream.h */,
A331D95A21F24992009F02AA /* FileSystem.cpp */,
A331D95921F24992009F02AA /* FileSystem.h */,
FF910E972979FE6F00D1A24D /* FixedBitVector.h */,
E3E0F04F26197157004640FC /* FixedVector.h */,
33479C1C27236F2000B2E1B7 /* FixedWidthDouble.h */,
0F2B66A517B6B4F700A7AE3F /* FlipBytes.h */,
Expand Down Expand Up @@ -3008,6 +3011,7 @@
DD3DC96E27A4BF8E007E5B61 /* FilePrintStream.h in Headers */,
DD3DC8F827A4BF8E007E5B61 /* FileSystem.h in Headers */,
DDF306FB27C086CC006A526F /* fixed-dtoa.h in Headers */,
FF910E982979FE6F00D1A24D /* FixedBitVector.h in Headers */,
DD3DC96527A4BF8E007E5B61 /* FixedVector.h in Headers */,
339B7B1127C45EF50072BF9A /* FixedWidthDouble.h in Headers */,
DD3DC8D327A4BF8E007E5B61 /* FlipBytes.h in Headers */,
Expand Down
3 changes: 3 additions & 0 deletions Source/WTF/wtf/BitVector.h
Expand Up @@ -39,6 +39,8 @@ class CachedBitVector;

namespace WTF {

class FixedBitVector;

// This is a space-efficient, resizeable bitvector class. In the common case it
// occupies one word, but if necessary, it will inflate this one word to point
// to a single chunk of out-of-line allocated storage to store an arbitrary number
Expand Down Expand Up @@ -361,6 +363,7 @@ class BitVector final {

private:
friend class JSC::CachedBitVector;
friend class FixedBitVector;

static unsigned bitsInPointer()
{
Expand Down
1 change: 1 addition & 0 deletions Source/WTF/wtf/CMakeLists.txt
Expand Up @@ -81,6 +81,7 @@ set(WTF_PUBLIC_HEADERS
FastTLS.h
FilePrintStream.h
FileSystem.h
FixedBitVector.h
FixedVector.h
FixedWidthDouble.h
FlipBytes.h
Expand Down
186 changes: 186 additions & 0 deletions Source/WTF/wtf/FixedBitVector.h
@@ -0,0 +1,186 @@
/*
* Copyright (C) 2023 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
* 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in the
* documentation and/or other materials provided with the distribution.
*
* THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY
* EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
* PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR
* CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
* EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
* PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
* PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
* OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/

#pragma once

#include <wtf/Assertions.h>
#include <wtf/Atomics.h>
#include <wtf/BitVector.h>
#include <wtf/HashTraits.h>
#include <wtf/PrintStream.h>
#include <wtf/StdIntExtras.h>
#include <wtf/StdLibExtras.h>

namespace WTF {

class FixedBitVector final {
WTF_MAKE_FAST_ALLOCATED;
using WordType = uintptr_t;

public:
FixedBitVector() = default;

FixedBitVector(size_t size)
: m_bitVector(size)
{
}

bool concurrentTestAndSet(size_t bitIndex, Dependency = Dependency());
bool concurrentTestAndClear(size_t bitIndex, Dependency = Dependency());

bool testAndSet(size_t bitIndex);
bool testAndClear(size_t bitIndex);
bool test(size_t bitIndex);

// Note that BitVector will be in inline mode with fixed size when
// the BitVector is constructed with size less or equal to `maxInlineBits`.
size_t size() const { return m_bitVector.size(); }

size_t findBit(size_t startIndex, bool value) const;

bool operator==(const FixedBitVector&) const;

unsigned hash() const;

void dump(PrintStream& out) const;

BitVector::iterator begin() const { return m_bitVector.begin(); }
BitVector::iterator end() const { return m_bitVector.end(); }

private:
static constexpr unsigned wordSize = sizeof(WordType) * 8;
static constexpr WordType one = 1;

BitVector m_bitVector;
};

ALWAYS_INLINE bool FixedBitVector::concurrentTestAndSet(size_t bitIndex, Dependency dependency)
{
if (UNLIKELY(bitIndex >= size()))
return false;

WordType mask = one << (bitIndex % wordSize);
size_t wordIndex = bitIndex / wordSize;
WordType* data = dependency.consume(m_bitVector.bits()) + wordIndex;
return !bitwise_cast<Atomic<WordType>*>(data)->transactionRelaxed(
[&](WordType& value) -> bool {
if (value & mask)
return false;

value |= mask;
return true;
});
}

ALWAYS_INLINE bool FixedBitVector::concurrentTestAndClear(size_t bitIndex, Dependency dependency)
{
if (UNLIKELY(bitIndex >= size()))
return false;

WordType mask = one << (bitIndex % wordSize);
size_t wordIndex = bitIndex / wordSize;
WordType* data = dependency.consume(m_bitVector.bits()) + wordIndex;
return bitwise_cast<Atomic<WordType>*>(data)->transactionRelaxed(
[&](WordType& value) -> bool {
if (!(value & mask))
return false;

value &= ~mask;
return true;
});
}

ALWAYS_INLINE bool FixedBitVector::testAndSet(size_t bitIndex)
{
if (UNLIKELY(bitIndex >= size()))
return false;

WordType mask = one << (bitIndex % wordSize);
size_t wordIndex = bitIndex / wordSize;
WordType* bits = m_bitVector.bits();
bool previousValue = bits[wordIndex] & mask;
bits[wordIndex] |= mask;
return previousValue;
}

ALWAYS_INLINE bool FixedBitVector::testAndClear(size_t bitIndex)
{
if (UNLIKELY(bitIndex >= size()))
return false;

WordType mask = one << (bitIndex % wordSize);
size_t wordIndex = bitIndex / wordSize;
WordType* bits = m_bitVector.bits();
bool previousValue = bits[wordIndex] & mask;
bits[wordIndex] &= ~mask;
return previousValue;
}

ALWAYS_INLINE bool FixedBitVector::test(size_t bitIndex)
{
if (UNLIKELY(bitIndex >= size()))
return false;

WordType mask = one << (bitIndex % wordSize);
size_t wordIndex = bitIndex / wordSize;
return m_bitVector.bits()[wordIndex] & mask;
}

ALWAYS_INLINE size_t FixedBitVector::findBit(size_t startIndex, bool value) const
{
return m_bitVector.findBit(startIndex, value);
}

ALWAYS_INLINE bool FixedBitVector::operator==(const FixedBitVector& other) const
{
return m_bitVector == other.m_bitVector;
}

ALWAYS_INLINE unsigned FixedBitVector::hash() const
{
return m_bitVector.hash();
}

ALWAYS_INLINE void FixedBitVector::dump(PrintStream& out) const
{
m_bitVector.dump(out);
}

struct FixedBitVectorHash {
static unsigned hash(const FixedBitVector& vector) { return vector.hash(); }
static bool equal(const FixedBitVector& a, const FixedBitVector& b) { return a == b; }
static constexpr bool safeToCompareToEmptyOrDeleted = false;
};

template<typename T> struct DefaultHash;
template<> struct DefaultHash<FixedBitVector> : FixedBitVectorHash {
};

template<> struct HashTraits<FixedBitVector> : public CustomHashTraits<FixedBitVector> {
};

} // namespace WTF

using WTF::FixedBitVector;
1 change: 1 addition & 0 deletions Tools/TestWebKitAPI/CMakeLists.txt
Expand Up @@ -48,6 +48,7 @@ set(TestWTF_SOURCES
Tests/WTF/EnumTraits.cpp
Tests/WTF/Expected.cpp
Tests/WTF/FileSystem.cpp
Tests/WTF/FixedBitVector.cpp
Tests/WTF/FixedVector.cpp
Tests/WTF/Function.cpp
Tests/WTF/HashCountedSet.cpp
Expand Down
4 changes: 4 additions & 0 deletions Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
Expand Up @@ -1245,6 +1245,7 @@
FEC2A85624CEB65F00ADBC35 /* PropertySlot.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FEC2A85524CEB65F00ADBC35 /* PropertySlot.cpp */; };
FF10AAF92831B1E600B9875B /* CompactPtr.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FF10AAF82831B1E600B9875B /* CompactPtr.cpp */; };
FF5D0CB4283221AD00F3278A /* CompactRefPtr.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FF5D0CB3283221AD00F3278A /* CompactRefPtr.cpp */; };
FF910EA5297A0D1100D1A24D /* FixedBitVector.cpp in Sources */ = {isa = PBXBuildFile; fileRef = FF910E9D297A0D1100D1A24D /* FixedBitVector.cpp */; };
/* End PBXBuildFile section */

/* Begin PBXContainerItemProxy section */
Expand Down Expand Up @@ -3532,6 +3533,7 @@
FF10AAF82831B1E600B9875B /* CompactPtr.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = CompactPtr.cpp; sourceTree = "<group>"; };
FF25942A2834B7A7006892D6 /* AlignedRefLogger.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = AlignedRefLogger.h; sourceTree = "<group>"; };
FF5D0CB3283221AD00F3278A /* CompactRefPtr.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = CompactRefPtr.cpp; sourceTree = "<group>"; };
FF910E9D297A0D1100D1A24D /* FixedBitVector.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = FixedBitVector.cpp; sourceTree = "<group>"; };
/* End PBXFileReference section */

/* Begin PBXFrameworksBuildPhase section */
Expand Down Expand Up @@ -5079,6 +5081,7 @@
1AF7B21D1D6CD12E008C126C /* EnumTraits.cpp */,
AD7C434C1DD2A5470026888B /* Expected.cpp */,
A310827121F296EC00C28B97 /* FileSystem.cpp */,
FF910E9D297A0D1100D1A24D /* FixedBitVector.cpp */,
E3210518261979F300157C67 /* FixedVector.cpp */,
9310CD361EF708FB0050FFE0 /* Function.cpp */,
83DB79671EF63B3C00BFA5E5 /* Function.cpp */,
Expand Down Expand Up @@ -6044,6 +6047,7 @@
1AF7B21F1D6CD14D008C126C /* EnumTraits.cpp in Sources */,
AD7C434D1DD2A54E0026888B /* Expected.cpp in Sources */,
A310827221F296FF00C28B97 /* FileSystem.cpp in Sources */,
FF910EA5297A0D1100D1A24D /* FixedBitVector.cpp in Sources */,
E3210519261979F300157C67 /* FixedVector.cpp in Sources */,
9310CD381EF708FB0050FFE0 /* Function.cpp in Sources */,
6BFD294C1D5E6C1D008EC968 /* HashCountedSet.cpp in Sources */,
Expand Down

0 comments on commit 527e2a0

Please sign in to comment.