Skip to content

Commit

Permalink
Fix direct comparisons with unshared basic heap types (#6845)
Browse files Browse the repository at this point in the history
Audit the remaining ocurrences of `== HeapType::` and fix those that did
not handle shared types correctly. Add tests for some of the fixes;
others are NFC but clarify the code.
  • Loading branch information
tlively committed Aug 16, 2024
1 parent 958ff41 commit 95a4d5d
Show file tree
Hide file tree
Showing 8 changed files with 152 additions and 30 deletions.
2 changes: 1 addition & 1 deletion src/passes/Precompute.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ struct Precompute
return;
}
} else if (singleValue.type.isRef() &&
singleValue.type.getHeapType() == HeapType::func) {
singleValue.type.getHeapType().isSignature()) {
if (auto* r = curr->value->template dynCast<RefFunc>()) {
r->func = singleValue.getFunc();
r->finalize();
Expand Down
13 changes: 9 additions & 4 deletions src/passes/StringLowering.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -291,8 +291,9 @@ struct StringLowering : public StringGathering {
// singleton rec group.
std::vector<Type> params, results;
auto fix = [](Type t) {
if (t.isRef() && t.getHeapType() == HeapType::string) {
t = Type(HeapType::ext, t.getNullability());
if (t.isRef() && t.getHeapType().isMaybeShared(HeapType::string)) {
auto share = t.getHeapType().getShared();
t = Type(HeapTypes::ext.getBasic(share), t.getNullability());
}
return t;
};
Expand Down Expand Up @@ -495,9 +496,13 @@ struct StringLowering : public StringGathering {
void noteSubtype(Expression* a, Type b) {
// This is the case we care about: if |a| is a null that must be a
// subtype of ext then we fix that up.
if (b.isRef() && b.getHeapType().getTop() == HeapType::ext) {
if (!b.isRef()) {
return;
}
HeapType top = b.getHeapType().getTop();
if (top.isMaybeShared(HeapType::ext)) {
if (auto* null = a->dynCast<RefNull>()) {
null->finalize(HeapType::noext);
null->finalize(HeapTypes::noext.getBasic(top.getShared()));
}
}
}
Expand Down
8 changes: 5 additions & 3 deletions src/tools/wasm-ctor-eval.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -834,11 +834,13 @@ struct CtorEvalExternalInterface : EvallingModuleRunner::ExternalInterface {
// logic here, we save the original (possible externalized) value, and then
// look at the internals from here on out.
Literal original = value;
if (value.type.isRef() && value.type.getHeapType() == HeapType::ext) {
if (value.type.isRef() &&
value.type.getHeapType().isMaybeShared(HeapType::ext)) {
value = value.internalize();

// We cannot serialize truly external things, only data and i31s.
assert(value.isData() || value.type.getHeapType() == HeapType::i31);
assert(value.isData() ||
value.type.getHeapType().isMaybeShared(HeapType::i31));
}

// GC data (structs and arrays) must be handled with the special global-
Expand Down Expand Up @@ -920,7 +922,7 @@ struct CtorEvalExternalInterface : EvallingModuleRunner::ExternalInterface {
Expression* ret = builder.makeGlobalGet(definingGlobalName, value.type);
if (original != value) {
// The original is externalized.
assert(original.type.getHeapType() == HeapType::ext);
assert(original.type.getHeapType().isMaybeShared(HeapType::ext));
ret = builder.makeRefAs(ExternConvertAny, ret);
}
return ret;
Expand Down
6 changes: 4 additions & 2 deletions src/wasm-builder.h
Original file line number Diff line number Diff line change
Expand Up @@ -1387,8 +1387,10 @@ class Builder {
if (curr->type.isNullable() && curr->type.isNull()) {
return ExpressionManipulator::refNull(curr, curr->type);
}
if (curr->type.isRef() && curr->type.getHeapType() == HeapType::i31) {
Expression* ret = makeRefI31(makeConst(0));
if (curr->type.isRef() &&
curr->type.getHeapType().isMaybeShared(HeapType::i31)) {
Expression* ret =
makeRefI31(makeConst(0), curr->type.getHeapType().getShared());
if (curr->type.isNullable()) {
// To keep the type identical, wrap it in a block that adds nullability.
ret = makeBlock({ret}, curr->type);
Expand Down
2 changes: 1 addition & 1 deletion src/wasm/literal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,7 @@ bool Literal::operator==(const Literal& other) const {
return gcData == other.gcData;
}
assert(type.getHeapType().isBasic());
if (type.getHeapType().getBasic(Unshared) == HeapType::i31) {
if (type.getHeapType().isMaybeShared(HeapType::i31)) {
return i32 == other.i32;
}
WASM_UNREACHABLE("unexpected type");
Expand Down
2 changes: 1 addition & 1 deletion src/wasm/wasm-validator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2769,7 +2769,7 @@ void FunctionValidator::visitCallRef(CallRef* curr) {
getModule()->features.hasGC(), curr, "call_ref requires gc [--enable-gc]");
if (curr->target->type == Type::unreachable ||
(curr->target->type.isRef() &&
curr->target->type.getHeapType() == HeapType::nofunc)) {
curr->target->type.getHeapType().isMaybeShared(HeapType::nofunc))) {
return;
}
if (shouldBeTrue(curr->target->type.isFunction(),
Expand Down
113 changes: 95 additions & 18 deletions test/lit/ctor-eval/extern.wast
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.
;; RUN: wasm-ctor-eval %s --ctors=test1,test2,test3 --kept-exports=test1,test2,test3 --quiet -all -S -o - | filecheck %s
;; RUN: wasm-ctor-eval %s --ctors=test1,test1-shared,test2,test2-shared,test3,test3-shared \
;; RUN: --kept-exports=test1,test1-shared,test2,test2-shared,test3,test3-shared --quiet -all -S -o - | filecheck %s

(module
;; CHECK: (type $array (array (mut i8)))
(type $array (array (mut i8)))
;; CHECK: (type $shared-array (shared (array (mut i8))))
(type $shared-array (shared (array (mut i8))))

;; CHECK: (type $struct (struct (field externref)))
(type $struct (struct (field externref)))
;; CHECK: (type $shared-struct (shared (struct (field (ref null (shared extern))))))
(type $shared-struct (shared (struct (field (ref null (shared extern))))))

(export "test1" (func $test1))
(export "test2" (func $test2))
(export "test3" (func $test3))

(func $test1 (result externref)
(func $test1 (export "test1") (result externref)
;; This will remain almost the same, even though we eval it, since the
;; serialization of an externalized i31 is what is written here. But the add
;; will be evalled out.
Expand All @@ -26,7 +27,19 @@
)
)

(func $test2 (result externref)
(func $test1-shared (export "test1-shared") (result (ref null (shared extern)))
;; Same as above, but now the i31 is shared.
(extern.convert_any
(ref.i31_shared
(i32.add
(i32.const 41)
(i32.const 1)
)
)
)
)

(func $test2 (export "test2") (result externref)
;; This will be evalled into an externalization of a global.get.
(extern.convert_any
(array.new_fixed $array 3
Expand All @@ -37,7 +50,18 @@
)
)

(func $test3 (result anyref)
(func $test2-shared (export "test2-shared") (result (ref null (shared extern)))
;; Same as above, but now the array is shared.
(extern.convert_any
(array.new_fixed $shared-array 3
(i32.const 1)
(i32.const 2)
(i32.const 3)
)
)
)

(func $test3 (export "test3") (result anyref)
;; This will add a global that contains an externalization operation.
(struct.new $struct
(extern.convert_any
Expand All @@ -47,46 +71,99 @@
)
)
)

(func $test3-shared (export "test3-shared") (result (ref null (shared any)))
;; Same as above, but now the struct and i31 are shared.
(struct.new $shared-struct
(extern.convert_any
(ref.i31_shared
(i32.const 1)
)
)
)
)
)

;; CHECK: (type $2 (func (result externref)))
;; CHECK: (type $4 (func (result externref)))

;; CHECK: (type $5 (func (result (ref null (shared extern)))))

;; CHECK: (type $6 (func (result anyref)))

;; CHECK: (type $3 (func (result anyref)))
;; CHECK: (type $7 (func (result (ref null (shared any)))))

;; CHECK: (global $ctor-eval$global (ref $array) (array.new_fixed $array 3
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: (i32.const 2)
;; CHECK-NEXT: (i32.const 3)
;; CHECK-NEXT: ))

;; CHECK: (global $ctor-eval$global_1 (ref $struct) (struct.new $struct
;; CHECK: (global $ctor-eval$global_1 (ref $shared-array) (array.new_fixed $shared-array 3
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: (i32.const 2)
;; CHECK-NEXT: (i32.const 3)
;; CHECK-NEXT: ))

;; CHECK: (global $ctor-eval$global_2 (ref $struct) (struct.new $struct
;; CHECK-NEXT: (extern.convert_any
;; CHECK-NEXT: (ref.i31
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: ))

;; CHECK: (export "test1" (func $test1_3))
;; CHECK: (global $ctor-eval$global_3 (ref $shared-struct) (struct.new $shared-struct
;; CHECK-NEXT: (extern.convert_any
;; CHECK-NEXT: (ref.i31_shared
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: ))

;; CHECK: (export "test1" (func $test1_6))

;; CHECK: (export "test1-shared" (func $test1-shared_7))

;; CHECK: (export "test2" (func $test2_8))

;; CHECK: (export "test2" (func $test2_4))
;; CHECK: (export "test2-shared" (func $test2-shared_9))

;; CHECK: (export "test3" (func $test3_5))
;; CHECK: (export "test3" (func $test3_10))

;; CHECK: (func $test1_3 (type $2) (result externref)
;; CHECK: (export "test3-shared" (func $test3-shared_11))

;; CHECK: (func $test1_6 (type $4) (result externref)
;; CHECK-NEXT: (extern.convert_any
;; CHECK-NEXT: (ref.i31
;; CHECK-NEXT: (i32.const 42)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )

;; CHECK: (func $test2_4 (type $2) (result externref)
;; CHECK: (func $test1-shared_7 (type $5) (result (ref null (shared extern)))
;; CHECK-NEXT: (extern.convert_any
;; CHECK-NEXT: (ref.i31_shared
;; CHECK-NEXT: (i32.const 42)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )

;; CHECK: (func $test2_8 (type $4) (result externref)
;; CHECK-NEXT: (extern.convert_any
;; CHECK-NEXT: (global.get $ctor-eval$global)
;; CHECK-NEXT: )
;; CHECK-NEXT: )

;; CHECK: (func $test3_5 (type $3) (result anyref)
;; CHECK-NEXT: (global.get $ctor-eval$global_1)
;; CHECK: (func $test2-shared_9 (type $5) (result (ref null (shared extern)))
;; CHECK-NEXT: (extern.convert_any
;; CHECK-NEXT: (global.get $ctor-eval$global_1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )

;; CHECK: (func $test3_10 (type $6) (result anyref)
;; CHECK-NEXT: (global.get $ctor-eval$global_2)
;; CHECK-NEXT: )

;; CHECK: (func $test3-shared_11 (type $7) (result (ref null (shared any)))
;; CHECK-NEXT: (global.get $ctor-eval$global_3)
;; CHECK-NEXT: )
36 changes: 36 additions & 0 deletions test/lit/passes/precompute-ref-func.wast
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.
;; RUN: wasm-opt %s -all --precompute -S -o - | filecheck %s

(module
;; CHECK: (type $0 (func (result funcref)))

;; CHECK: (type $shared-func (shared (func (result (ref null (shared func))))))
(type $shared-func (shared (func (result (ref null (shared func))))))
;; CHECK: (elem declare func $test $test-shared)

;; CHECK: (func $test (type $0) (result funcref)
;; CHECK-NEXT: (return
;; CHECK-NEXT: (ref.func $test)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $test (result funcref)
(block
(return
(ref.func $test)
)
)
)

;; CHECK: (func $test-shared (type $shared-func) (result (ref null (shared func)))
;; CHECK-NEXT: (return
;; CHECK-NEXT: (ref.func $test-shared)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $test-shared (type $shared-func)
(block
(return
(ref.func $test-shared)
)
)
)
)

0 comments on commit 95a4d5d

Please sign in to comment.