Skip to content

Commit df5c0e0

Browse files
authored
Handle sharedness in Heap2Local's Array2Struct (WebAssembly#8515)
Heap2Local uses an internal utility called Array2Struct to turn non-escaping array allocations into struct allocations so they can subsequently be optimized by the Heap2Local struct optimizations. It previously used a non-shared struct type, even for shared array types, but this could cause problems when the non-shared struct type became a non-shared null in places that required a shared type to validate. This could specifically occur when the allocation flowed into a StructCmpxchg `expected` field that needed to be a subtype of shared eqref. As a drive-by to make the regression test parse correctly, fix child-typer for StructCmpxchg to correctly handle sharedness in the expected field as well.
1 parent fe2fe01 commit df5c0e0

File tree

5 files changed

+70
-3
lines changed

5 files changed

+70
-3
lines changed

scripts/test/fuzzing.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@
8383
'unsubtyping-cmpxchg.wast',
8484
'struct-atomic-threads.wast',
8585
'type-refining-gufa-rmw.wast',
86+
'struct-cmpxchg-shared-expected.wast',
8687
'precompute-gc-atomics-rmw.wast',
8788
# contains too many segments to run in a wasm VM
8889
'limit-segments_disable-bulk-memory.wast',

src/ir/child-typer.h

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1002,8 +1002,12 @@ template<typename Subtype> struct ChildTyper : OverriddenVisitor<Subtype> {
10021002
assert(curr->index < fields.size());
10031003
note(&curr->ref, Type(*ht, Nullable));
10041004
auto type = fields[curr->index].type;
1005-
// TODO: (shared eq) as appropriate.
1006-
note(&curr->expected, type.isRef() ? Type(HeapType::eq, Nullable) : type);
1005+
auto expectedType = type;
1006+
if (expectedType.isRef()) {
1007+
expectedType =
1008+
Type(HeapTypes::eq.getBasic(type.getHeapType().getShared()), Nullable);
1009+
}
1010+
note(&curr->expected, expectedType);
10071011
note(&curr->replacement, type);
10081012
}
10091013

src/passes/Heap2Local.cpp

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1250,7 +1250,10 @@ struct Array2Struct : PostWalker<Array2Struct> {
12501250
for (Index i = 0; i < numFields; i++) {
12511251
fields.push_back(element);
12521252
}
1253-
structType = Struct(fields);
1253+
TypeBuilder typeBuilder(1);
1254+
typeBuilder[0] = Struct(fields);
1255+
typeBuilder[0].setShared(arrayType.getShared());
1256+
structType = (*typeBuilder.build())[0];
12541257

12551258
// Generate a StructNew to replace the ArrayNew*.
12561259
if (auto* arrayNew = allocation->dynCast<ArrayNew>()) {
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
;; NOTE: Assertions have been generated by update_lit_checks.py and should not be edited.
2+
3+
;; RUN: wasm-opt -all %s -S -o -| filecheck %s
4+
5+
(module
6+
;; CHECK: (type $shared-struct (shared (struct (field (mut (ref null (shared eq)))))))
7+
(type $shared-struct (shared (struct (field (mut (ref null (shared eq)))))))
8+
;; CHECK: (func $cmpxchg-shared-expected (type $1) (param $shared-struct (ref $shared-struct))
9+
;; CHECK-NEXT: (struct.atomic.rmw.cmpxchg $shared-struct 0
10+
;; CHECK-NEXT: (local.get $shared-struct)
11+
;; CHECK-NEXT: (struct.new_default $shared-struct)
12+
;; CHECK-NEXT: (unreachable)
13+
;; CHECK-NEXT: )
14+
;; CHECK-NEXT: )
15+
(func $cmpxchg-shared-expected (param $shared-struct (ref $shared-struct))
16+
(struct.atomic.rmw.cmpxchg $shared-struct 0
17+
(local.get $shared-struct)
18+
;; We should expect a shared eqref here, so we can parse this as written
19+
;; rather than dropping the operands and replacing them with unreachables.
20+
(struct.new_default $shared-struct)
21+
(unreachable)
22+
)
23+
)
24+
)

test/lit/passes/heap2local-rmw.wast

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1283,3 +1283,38 @@
12831283
)
12841284
)
12851285
)
1286+
1287+
(module
1288+
(type $array (shared (array i8)))
1289+
;; CHECK: (type $struct (shared (struct (field (mut (ref null (shared array)))))))
1290+
(type $struct (shared (struct (field (mut (ref null (shared array)))))))
1291+
1292+
;; CHECK: (type $1 (func (param (ref $struct))))
1293+
1294+
;; CHECK: (func $unreachable-shared-expected (type $1) (param $struct (ref $struct))
1295+
;; CHECK-NEXT: (drop
1296+
;; CHECK-NEXT: (struct.atomic.rmw.cmpxchg $struct 0
1297+
;; CHECK-NEXT: (local.get $struct)
1298+
;; CHECK-NEXT: (block (result (ref null (shared none)))
1299+
;; CHECK-NEXT: (ref.null (shared none))
1300+
;; CHECK-NEXT: )
1301+
;; CHECK-NEXT: (unreachable)
1302+
;; CHECK-NEXT: )
1303+
;; CHECK-NEXT: )
1304+
;; CHECK-NEXT: )
1305+
(func $unreachable-shared-expected (param $struct (ref $struct))
1306+
(drop
1307+
;; This cmpxchg will not be optimized because it is unreachable.
1308+
(struct.atomic.rmw.cmpxchg $struct 0
1309+
(local.get $struct)
1310+
;; This `expected` allocation does not escape, so it will be optimized.
1311+
;; When we convert the array to a struct, we have to make sure it
1312+
;; becomes a shared struct so that when we later optimize out the
1313+
;; allocation, we know to replace it with a shared null. If we don't, we
1314+
;; will end up with a non-shared null here, which would be invalid.
1315+
(array.new_fixed $array 0)
1316+
(unreachable)
1317+
)
1318+
)
1319+
)
1320+
)

0 commit comments

Comments
 (0)