Skip to content

Commit 74ae98b

Browse files
kripkenkripken
authored
Fix regression from #7822 about exnrefs with nulls and unknowns (#7835)
RootLocation, added in #7822, was overly-ambitious in how much it simplified. It turns out that there are situations where we need the same type for both a null and an "unknown" of that type, and a single RootLocation is therefore not enough (the specific situation the fuzzer found involves exnref, but perhaps others are possible too). To fix this, refactor back to two locations, NullLocation and TypeLocation, where the latter is at least shared by exn and cont as a place with an unknown value of a type.
1 parent f9d4be9 commit 74ae98b

3 files changed

Lines changed: 126 additions & 28 deletions

File tree

‎src/ir/possible-contents.cpp‎

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -891,23 +891,16 @@ struct InfoCollector
891891
handleIndirectCall(curr, curr->target->type);
892892
}
893893

894-
// Creates a location for a root of a particular type, creating a RootLocation
895-
// and marking it as a root.
896-
Location getRootLocation(Type type, PossibleContents rootValue) {
897-
auto location = RootLocation{type};
898-
addRoot(location, rootValue);
894+
Location getTypeLocation(Type type) {
895+
auto location = TypeLocation{type};
896+
addRoot(location, PossibleContents::fromType(type));
899897
return location;
900898
}
901899

902-
// Creates a root location, settings its value by the type.
903-
Location getRootLocation(Type type) {
904-
return getRootLocation(type, PossibleContents::fromType(type));
905-
}
906-
907-
// Makes a root location containing a null.
908900
Location getNullLocation(Type type) {
909-
return getRootLocation(type,
910-
PossibleContents::literal(Literal::makeZero(type)));
901+
auto location = NullLocation{type};
902+
addRoot(location, PossibleContents::literal(Literal::makeZero(type)));
903+
return location;
911904
}
912905

913906
// Iterates over a list of children and adds links from them. The target of
@@ -1230,7 +1223,7 @@ struct InfoCollector
12301223
}
12311224

12321225
if (curr->catchRefs[tagIndex]) {
1233-
auto location = getRootLocation(Type(HeapType::exn, NonNullable));
1226+
auto location = getTypeLocation(Type(HeapType::exn, NonNullable));
12341227
info.links.push_back(
12351228
{location, getBreakTargetLocation(target, exnrefIndex)});
12361229
}
@@ -1317,7 +1310,7 @@ struct InfoCollector
13171310
auto targetType = findBreakTarget(target)->type;
13181311
assert(targetType.size() >= 1);
13191312
auto contType = targetType[targetType.size() - 1];
1320-
auto location = getRootLocation(contType);
1313+
auto location = getTypeLocation(contType);
13211314
info.links.push_back(
13221315
{location, getBreakTargetLocation(target, params.size())});
13231316
}

‎src/ir/possible-contents.h‎

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -500,17 +500,22 @@ struct TagLocation {
500500
}
501501
};
502502

503-
// A root value. This is used as the location of the default value of a var in a
504-
// function, a null written to a struct field in struct.new_with_default, an
505-
// exnref from a catch etc. - in all these cases, we know
506-
//
507-
// 1. The value (which might be a null, or "anything of this type").
508-
// 2. That the value will never change; we can learn nothing more here.
509-
// 3. That all roots of this type can share the same location.
510-
//
511-
struct RootLocation {
503+
// A null value of a particular type. For example, a nullable local reads from
504+
// the corresponding NullLocation, for its default value.
505+
struct NullLocation {
512506
Type type;
513-
bool operator==(const RootLocation& other) const {
507+
bool operator==(const NullLocation& other) const {
508+
return type == other.type;
509+
}
510+
};
511+
512+
// A location that contains anything of a particular type. This is used as a
513+
// root of the graph, a source of values we will never learn anything about, and
514+
// must assume they can be anything of that type (e.g., the return of a call to
515+
// an import).
516+
struct TypeLocation {
517+
Type type;
518+
bool operator==(const TypeLocation& other) const {
514519
return type == other.type;
515520
}
516521
};
@@ -552,7 +557,8 @@ using Location = std::variant<ExpressionLocation,
552557
SignatureResultLocation,
553558
DataLocation,
554559
TagLocation,
555-
RootLocation,
560+
NullLocation,
561+
TypeLocation,
556562
ConeReadLocation>;
557563

558564
} // namespace wasm
@@ -633,8 +639,14 @@ template<> struct hash<wasm::TagLocation> {
633639
}
634640
};
635641

636-
template<> struct hash<wasm::RootLocation> {
637-
size_t operator()(const wasm::RootLocation& loc) const {
642+
template<> struct hash<wasm::NullLocation> {
643+
size_t operator()(const wasm::NullLocation& loc) const {
644+
return std::hash<wasm::Type>{}(loc.type);
645+
}
646+
};
647+
648+
template<> struct hash<wasm::TypeLocation> {
649+
size_t operator()(const wasm::TypeLocation& loc) const {
638650
return std::hash<wasm::Type>{}(loc.type);
639651
}
640652
};

‎test/lit/passes/gufa-eh-null-type.wast‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.
2+
3+
;; RUN: foreach %s %t wasm-opt -all --gufa -S -o - | filecheck %s
4+
5+
;; The function here will use a NullLocation for (ref exn) as well as a
6+
;; TypeLocation for that very same type. We should not get confused by that (see
7+
;; below).
8+
9+
(module
10+
;; CHECK: (type $func (func))
11+
(type $func (func))
12+
13+
;; CHECK: (tag $tag (type $func))
14+
(tag $tag (type $func))
15+
16+
;; CHECK: (export "func" (func $func))
17+
18+
;; CHECK: (func $func (type $func)
19+
;; CHECK-NEXT: (local $1 (ref exn))
20+
;; CHECK-NEXT: (drop
21+
;; CHECK-NEXT: (block $block (result (ref exn))
22+
;; CHECK-NEXT: (try_table (catch_all_ref $block)
23+
;; CHECK-NEXT: (throw $tag)
24+
;; CHECK-NEXT: )
25+
;; CHECK-NEXT: (return)
26+
;; CHECK-NEXT: )
27+
;; CHECK-NEXT: )
28+
;; CHECK-NEXT: (try
29+
;; CHECK-NEXT: (do
30+
;; CHECK-NEXT: (nop)
31+
;; CHECK-NEXT: )
32+
;; CHECK-NEXT: (catch $tag
33+
;; CHECK-NEXT: (drop
34+
;; CHECK-NEXT: (block
35+
;; CHECK-NEXT: (drop
36+
;; CHECK-NEXT: (block $block0 (result (ref noexn))
37+
;; CHECK-NEXT: (local.tee $1
38+
;; CHECK-NEXT: (unreachable)
39+
;; CHECK-NEXT: )
40+
;; CHECK-NEXT: (br_on_non_null $block0
41+
;; CHECK-NEXT: (ref.null noexn)
42+
;; CHECK-NEXT: )
43+
;; CHECK-NEXT: (drop
44+
;; CHECK-NEXT: (unreachable)
45+
;; CHECK-NEXT: )
46+
;; CHECK-NEXT: (unreachable)
47+
;; CHECK-NEXT: )
48+
;; CHECK-NEXT: )
49+
;; CHECK-NEXT: (unreachable)
50+
;; CHECK-NEXT: )
51+
;; CHECK-NEXT: )
52+
;; CHECK-NEXT: )
53+
;; CHECK-NEXT: )
54+
;; CHECK-NEXT: )
55+
(func $func (export "func")
56+
(local $1 (ref exn))
57+
;; This part ends up generating a TypeLocation for (ref exn).
58+
;;
59+
;; This code should not be mis-optimized. In particular, it should not trap.
60+
(drop
61+
(block $block (result (ref exn))
62+
(try_table (catch_all_ref $block)
63+
(throw $tag)
64+
)
65+
(return)
66+
)
67+
)
68+
(try
69+
(do
70+
(nop)
71+
)
72+
(catch $tag
73+
(drop
74+
(block $block (result (ref exn))
75+
;; This part ends up generating a NullLocation for (ref exn).
76+
;;
77+
;; Due to the unreachable here, we will end up modifying this code to add
78+
;; more unreachables.
79+
(br_on_non_null $block
80+
(local.tee $1
81+
(unreachable)
82+
)
83+
(ref.null noexn)
84+
)
85+
(drop
86+
(local.get $1)
87+
)
88+
)
89+
)
90+
)
91+
)
92+
)
93+
)

0 commit comments

Comments
 (0)