Skip to content

Commit e9f693d

Browse files
kripkenkripken
authored
[GC] Fix TypeRefining on StructGets without content but with a reachable ref (WebAssembly#7138)
If we see a StructGet with no content (the type it reads from has no writes) then we can make it unreachable. The old code literally just changed the type to unreachable, which would later work out with refinalization - but only if the StructGet's ref was unreachable. But it is possible for this situation to occur without that, and if so, this hit the validation error "can't have an unreachable node without an unreachable child". To fix this, merge all code paths that handle "impossible" situations, which simplifies things, and add this situation. This uncovered an existing bug where we noted default values of refs, but not non-refs (which could lead us to think that a field of a struct that only was ever created by struct.new_default, was never created at all). Fixed as well.
1 parent 7f62a42 commit e9f693d

File tree

2 files changed

+172
-62
lines changed

2 files changed

+172
-62
lines changed

‎src/passes/TypeRefining.cpp‎

Lines changed: 42 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -58,11 +58,13 @@ struct FieldInfoScanner
5858

5959
void
6060
noteDefault(Type fieldType, HeapType type, Index index, FieldInfo& info) {
61-
// Default values do not affect what the heap type of a field can be turned
62-
// into. Note them, however, as they force us to keep the type nullable.
61+
// Default values must be noted, so that we know there is content there.
6362
if (fieldType.isRef()) {
64-
info.note(Type(fieldType.getHeapType().getBottom(), Nullable));
63+
// All we need to note here is nullability (the field must remain
64+
// nullable), but not anything else about the type.
65+
fieldType = Type(fieldType.getHeapType().getBottom(), Nullable);
6566
}
67+
info.note(fieldType);
6668
}
6769

6870
void noteCopy(HeapType type, Index index, FieldInfo& info) {
@@ -270,71 +272,49 @@ struct TypeRefining : public Pass {
270272
return;
271273
}
272274

273-
if (curr->ref->type.isNull()) {
274-
// This get will trap. In theory we could leave this for later
275-
// optimizations to do, but we must actually handle it here, because
276-
// of the situation where this get's type is refined, and the input
277-
// type is the result of a refining:
278-
//
279-
// (struct.get $A ;; should be refined to something
280-
// (struct.get $B ;; just refined to nullref
281-
//
282-
// If the input has become a nullref then we can't just return out of
283-
// this function, as we'd be leaving a struct.get of $A with the
284-
// wrong type. But we can't find the right type since in Binaryen IR
285-
// we use the ref's type to see what is being read, and that just
286-
// turned into nullref. To avoid that corner case, just turn this code
287-
// into unreachable code now, and the later refinalize will turn all
288-
// the parents unreachable, avoiding any type-checking problems.
289-
Builder builder(*getModule());
290-
replaceCurrent(builder.makeSequence(builder.makeDrop(curr->ref),
291-
builder.makeUnreachable()));
292-
return;
275+
Type newFieldType;
276+
if (!curr->ref->type.isNull()) {
277+
auto oldType = curr->ref->type.getHeapType();
278+
newFieldType = parent.finalInfos[oldType][curr->index].getLUB();
293279
}
294280

295-
auto oldType = curr->ref->type.getHeapType();
296-
auto newFieldType = parent.finalInfos[oldType][curr->index].getLUB();
297-
if (Type::isSubType(newFieldType, curr->type)) {
298-
// This is the normal situation, where the new type is a refinement of
299-
// the old type. Apply that type so that the type of the struct.get
300-
// matches what is in the refined field. ReFinalize will later
301-
// propagate this to parents.
302-
//
303-
// Note that ReFinalize will also apply the type of the field itself
304-
// to a struct.get, so our doing it here in this pass is usually
305-
// redundant. But ReFinalize also updates other types while doing so,
306-
// which can cause a problem:
307-
//
308-
// (struct.get $A
309-
// (block (result (ref null $A))
310-
// (ref.null any)
311-
// )
312-
// )
313-
//
314-
// Here ReFinalize will turn the block's result into a bottom type,
315-
// which means it won't know a type for the struct.get at that point.
316-
// Doing it in this pass avoids that issue, as we have all the
317-
// necessary information. (ReFinalize will still get into the
318-
// situation where it doesn't know how to update the type of the
319-
// struct.get, but it will just leave the existing type - it assumes
320-
// no update is needed - which will be correct, since we've updated it
321-
// ourselves here, before.)
322-
curr->type = newFieldType;
323-
} else {
324-
// This instruction is invalid, so it must be the result of the
325-
// situation described above: we ignored the read during our
326-
// inference, and optimized accordingly, and so now we must remove it
327-
// to keep the module validating. It doesn't matter what we emit here,
328-
// since there are no struct.new or struct.sets for this type, so this
329-
// code is logically unreachable.
330-
//
331-
// Note that we emit an unreachable here, which changes the type, and
332-
// so we should refinalize. However, we will be refinalizing later
333-
// anyhow in updateTypes, so there is no need.
281+
if (curr->ref->type.isNull() || newFieldType == Type::unreachable ||
282+
!Type::isSubType(newFieldType, curr->type)) {
283+
// This get will trap, or cannot be reached: either the ref is null,
284+
// or the field is never written any contents, or the contents we see
285+
// are invalid (they passed through some fallthrough that will trap at
286+
// runtime). Emit unreachable code here.
334287
Builder builder(*getModule());
335288
replaceCurrent(builder.makeSequence(builder.makeDrop(curr->ref),
336289
builder.makeUnreachable()));
290+
return;
337291
}
292+
293+
// This is the normal situation, where the new type is a refinement of
294+
// the old type. Apply that type so that the type of the struct.get
295+
// matches what is in the refined field. ReFinalize will later
296+
// propagate this to parents.
297+
//
298+
// Note that ReFinalize will also apply the type of the field itself
299+
// to a struct.get, so our doing it here in this pass is usually
300+
// redundant. But ReFinalize also updates other types while doing so,
301+
// which can cause a problem:
302+
//
303+
// (struct.get $A
304+
// (block (result (ref null $A))
305+
// (ref.null any)
306+
// )
307+
// )
308+
//
309+
// Here ReFinalize will turn the block's result into a bottom type,
310+
// which means it won't know a type for the struct.get at that point.
311+
// Doing it in this pass avoids that issue, as we have all the
312+
// necessary information. (ReFinalize will still get into the
313+
// situation where it doesn't know how to update the type of the
314+
// struct.get, but it will just leave the existing type - it assumes
315+
// no update is needed - which will be correct, since we've updated it
316+
// ourselves here, before.)
317+
curr->type = newFieldType;
338318
}
339319
};
340320

‎test/lit/passes/type-refining.wast‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1574,3 +1574,133 @@
15741574
(ref.null $8)
15751575
)
15761576
)
1577+
1578+
;; Test for a bug where we made a struct.get unreachable because it was reading
1579+
;; a field that had no writes, but in a situation where it is invalid for the
1580+
;; struct.get to be unreachable.
1581+
(module
1582+
;; CHECK: (type $never (sub (struct (field i32))))
1583+
(type $never (sub (struct (field i32))))
1584+
1585+
;; CHECK: (rec
1586+
;; CHECK-NEXT: (type $optimizable (struct (field (mut nullfuncref))))
1587+
(type $optimizable (struct (field (mut (ref null func)))))
1588+
1589+
;; CHECK: (type $2 (func))
1590+
1591+
;; CHECK: (global $never (ref null $never) (ref.null none))
1592+
(global $never (export "never") (ref null $never)
1593+
;; Make the type $never public (if it were private, we'd optimize in a
1594+
;; different way that avoids the bug that this tests for).
1595+
(ref.null $never)
1596+
)
1597+
1598+
;; CHECK: (export "never" (global $never))
1599+
1600+
;; CHECK: (func $setup (type $2)
1601+
;; CHECK-NEXT: (drop
1602+
;; CHECK-NEXT: (struct.new $optimizable
1603+
;; CHECK-NEXT: (ref.null nofunc)
1604+
;; CHECK-NEXT: )
1605+
;; CHECK-NEXT: )
1606+
;; CHECK-NEXT: (drop
1607+
;; CHECK-NEXT: (block
1608+
;; CHECK-NEXT: (drop
1609+
;; CHECK-NEXT: (block (result (ref none))
1610+
;; CHECK-NEXT: (ref.as_non_null
1611+
;; CHECK-NEXT: (ref.null none)
1612+
;; CHECK-NEXT: )
1613+
;; CHECK-NEXT: )
1614+
;; CHECK-NEXT: )
1615+
;; CHECK-NEXT: (unreachable)
1616+
;; CHECK-NEXT: )
1617+
;; CHECK-NEXT: )
1618+
;; CHECK-NEXT: )
1619+
(func $setup
1620+
;; A struct.new, so that we have a field to refine (which avoids the pass
1621+
;; early-exiting).
1622+
(drop
1623+
(struct.new $optimizable
1624+
(ref.null func)
1625+
)
1626+
)
1627+
;; A struct.get that reads a $never, but the actual fallthrough value is none.
1628+
;; We never create this type, so the field has no possible content, and we can
1629+
;; replace this with an unreachable.
1630+
(drop
1631+
(struct.get $never 0
1632+
(block (result (ref $never))
1633+
(ref.as_non_null
1634+
(ref.null none)
1635+
)
1636+
)
1637+
)
1638+
)
1639+
)
1640+
)
1641+
1642+
;; Test that we note default values.
1643+
(module
1644+
(rec
1645+
;; CHECK: (rec
1646+
;; CHECK-NEXT: (type $A (sub (struct (field i32))))
1647+
(type $A (sub (struct (field i32))))
1648+
;; CHECK: (type $B (sub (struct (field i32))))
1649+
(type $B (sub (struct (field i32))))
1650+
)
1651+
;; CHECK: (type $2 (func (param (ref null $A) (ref null $B))))
1652+
1653+
;; CHECK: (type $optimizable (sub (struct (field (ref $2)))))
1654+
(type $optimizable (sub (struct (field funcref))))
1655+
1656+
;; CHECK: (elem declare func $test)
1657+
1658+
;; CHECK: (export "test" (func $test))
1659+
1660+
;; CHECK: (func $test (type $2) (param $x (ref null $A)) (param $y (ref null $B))
1661+
;; CHECK-NEXT: (drop
1662+
;; CHECK-NEXT: (struct.new $optimizable
1663+
;; CHECK-NEXT: (ref.func $test)
1664+
;; CHECK-NEXT: )
1665+
;; CHECK-NEXT: )
1666+
;; CHECK-NEXT: (drop
1667+
;; CHECK-NEXT: (struct.get $A 0
1668+
;; CHECK-NEXT: (struct.new $A
1669+
;; CHECK-NEXT: (i32.const 0)
1670+
;; CHECK-NEXT: )
1671+
;; CHECK-NEXT: )
1672+
;; CHECK-NEXT: )
1673+
;; CHECK-NEXT: (drop
1674+
;; CHECK-NEXT: (struct.get $B 0
1675+
;; CHECK-NEXT: (struct.new_default $B)
1676+
;; CHECK-NEXT: )
1677+
;; CHECK-NEXT: )
1678+
;; CHECK-NEXT: )
1679+
(func $test (export "test") (param $x (ref null $A)) (param $y (ref null $B))
1680+
;; Use $A, $B as params of this export, so they are public.
1681+
1682+
;; Make something for the pass to do, to avoid early-exit.
1683+
(drop
1684+
(struct.new $optimizable
1685+
(ref.func $test)
1686+
)
1687+
)
1688+
1689+
;; Get from a struct.new. We have nothing to optimize here. (In particular, we
1690+
;; cannot make this unreachable, as there is a value in the field, 0.)
1691+
(drop
1692+
(struct.get $A 0
1693+
(struct.new $A
1694+
(i32.const 0)
1695+
)
1696+
)
1697+
)
1698+
1699+
;; As above. Now the value in the field comes from a default value.
1700+
(drop
1701+
(struct.get $B 0
1702+
(struct.new_default $B)
1703+
)
1704+
)
1705+
)
1706+
)

0 commit comments

Comments
 (0)