Skip to content

Commit

Permalink
[Wasm-GC] Fix write barrier bug in BBQ array.set
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=267381

Reviewed by Justin Michaud.

Fixes a bug in the patch for bug245405. The write barriers in these cases were
in the right place, but the condition to check for them was wrong (because BBQ
values use I64 type kind for Ref types). The condition now uses the type index
to look up the type.

* JSTests/wasm/gc/bug267381.js: Added.
(i.assert.eq.m2.exports):
* Source/JavaScriptCore/wasm/WasmBBQJIT.cpp:
(JSC::Wasm::BBQJIT::addArrayNewFixed):
(JSC::Wasm::BBQJIT::addArraySet):

Canonical link: https://commits.webkit.org/272923@main
  • Loading branch information
takikawa committed Jan 11, 2024
1 parent ea28020 commit 6decd84
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 6 deletions.
57 changes: 57 additions & 0 deletions JSTests/wasm/gc/bug267381.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
//@ runWebAssemblySuite("--useWebAssemblyTypedFunctionReferences=true", "--useWebAssemblyGC=true", "--useWebAssemblyExtendedConstantExpressions=true")

import * as assert from "../assert.js";
import { instantiate } from "./wast-wrapper.js";

{
const m1 = instantiate(`
(module
(type (struct (field i32)))
(type (array (mut (ref null 0))))
(func (export "maker") (result (ref 1))
(array.new_default 1 (i32.const 5))))
`);

const arr = m1.exports.maker();
assert.isObject(arr);

// Do a GC to ensure the array is an old object.
gc();

const m2 = instantiate(`
(module
(type (struct (field i32)))
(type (array (mut (ref null 0))))
(func (export "set") (param (ref 1) i32)
(array.set 1 (local.get 0) (local.get 1) (struct.new 0 (i32.const 42))))
(func (export "get") (param (ref 1) i32) (result i32)
(struct.get 0 0 (array.get 1 (local.get 0) (local.get 1)))))
`);

for (var i = 0; i < 5; i++)
m2.exports.set(arr, i);

// Do an eden GC to test write barriers.
edenGC();

for (var i = 0; i < 5; i++)
assert.eq(m2.exports.get(arr, i), 42);
}

// Test array.new_fixed case.
{
const m = instantiate(`
(module
(type (struct (field i32)))
(type (array (mut (ref null 0))))
(func (export "maker") (result (ref 1))
(array.new_fixed 1 2 (struct.new 0 (i32.const 42)) (struct.new 0 (i32.const 42))))
(func (export "get") (param (ref 1) i32) (result i32)
(struct.get 0 0 (array.get 1 (local.get 0) (local.get 1)))))
`);

const arr = m.exports.maker();
gc();
assert.eq(m.exports.get(arr, 0), 42);
assert.eq(m.exports.get(arr, 1), 42);
}
11 changes: 5 additions & 6 deletions Source/JavaScriptCore/wasm/WasmBBQJIT.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3959,13 +3959,12 @@ class BBQJIT {
Location resultLocation = allocate(result);
emitMove(allocationResult, resultLocation);

// It's sufficient to check if the first arg is a reftype as they all are or none are.
if (args.size() > 0 && (args[0].type() == TypeKind::Ref || args[0].type() == TypeKind::RefNull))
emitWriteBarrier(resultLocation.asGPR());

// If args.isEmpty() then allocationResult.asTemp() == result.asTemp() so we will consume our result.
if (args.size())
if (args.size()) {
consume(allocationResult);
if (isRefType(getArrayElementType(typeIndex).unpacked()))
emitWriteBarrier(resultLocation.asGPR());
}

LOG_INSTRUCTION("ArrayNewFixed", typeIndex, args.size(), RESULT(result));
return { };
Expand Down Expand Up @@ -4124,7 +4123,7 @@ class BBQJIT {

emitArraySetUnchecked(typeIndex, arrayref, index, value);

if (value.type() == TypeKind::Ref || value.type() == TypeKind::RefNull)
if (isRefType(getArrayElementType(typeIndex).unpacked()))
emitWriteBarrier(arrayLocation.asGPR());
consume(arrayref);

Expand Down

0 comments on commit 6decd84

Please sign in to comment.