Skip to content

Commit 04eee21

Browse files
fitzgenfitzgen
authored
Fix a bug where arrays of GC refs were misreporting their count of GC edges (#10453)
* Fix a bug where arrays of GC refs were misreporting their count of GC edges They were accidentally reporting the size of their GC reference elements, rather than the count. * fix miri ignore attr
1 parent 4abef51 commit 04eee21

File tree

8 files changed

+277
-13
lines changed

8 files changed

+277
-13
lines changed

‎crates/cranelift/src/gc/enabled.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1137,12 +1137,11 @@ fn emit_array_size(
11371137
func_env: &mut FuncEnvironment<'_>,
11381138
builder: &mut FunctionBuilder<'_>,
11391139
array_layout: &GcArrayLayout,
1140-
init: ArrayInit<'_>,
1140+
len: ir::Value,
11411141
) -> ir::Value {
11421142
let base_size = builder
11431143
.ins()
11441144
.iconst(ir::types::I32, i64::from(array_layout.base_size));
1145-
let len = init.len(&mut builder.cursor());
11461145

11471146
// `elems_size = len * elem_size`
11481147
//
@@ -1155,6 +1154,7 @@ fn emit_array_size(
11551154
// i64 values, doing a 64-bit multiplication, and then checking the high
11561155
// 32 bits of the multiplication's result. If the high 32 bits are not
11571156
// all zeros, then the multiplication overflowed.
1157+
debug_assert_eq!(builder.func.dfg.value_type(len), ir::types::I32);
11581158
let len = builder.ins().uextend(ir::types::I64, len);
11591159
let elems_size_64 = builder
11601160
.ins()

‎crates/cranelift/src/gc/enabled/drc.rs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -320,9 +320,10 @@ impl GcCompiler for DrcCompiler {
320320

321321
// First, compute the array's total size from its base size, element
322322
// size, and length.
323-
let size = emit_array_size(func_env, builder, &array_layout, init);
323+
let len = init.len(&mut builder.cursor());
324+
let size = emit_array_size(func_env, builder, &array_layout, len);
324325
let num_gc_refs = if array_layout.elems_are_gc_refs {
325-
size
326+
len
326327
} else {
327328
builder.ins().iconst(ir::types::I32, 0)
328329
};

‎crates/cranelift/src/gc/enabled/null.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,8 @@ impl GcCompiler for NullCompiler {
166166

167167
// First, compute the array's total size from its base size, element
168168
// size, and length.
169-
let size = emit_array_size(func_env, builder, &array_layout, init);
169+
let len = init.len(&mut builder.cursor());
170+
let size = emit_array_size(func_env, builder, &array_layout, len);
170171

171172
// Next, allocate the array.
172173
assert!(align.is_power_of_two());

‎tests/all/gc.rs‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1171,3 +1171,61 @@ fn drc_transitive_drop_nested_arrays_tree() -> Result<()> {
11711171

11721172
Ok(())
11731173
}
1174+
1175+
#[test]
1176+
#[cfg_attr(miri, ignore)]
1177+
fn drc_traces_the_correct_number_of_gc_refs_in_arrays() -> Result<()> {
1178+
let _ = env_logger::try_init();
1179+
1180+
let mut config = Config::new();
1181+
config.wasm_function_references(true);
1182+
config.wasm_gc(true);
1183+
config.collector(Collector::DeferredReferenceCounting);
1184+
1185+
let engine = Engine::new(&config)?;
1186+
let mut store = Store::new(&engine, ());
1187+
1188+
// The DRC collector was mistakenly reporting that arrays of GC refs had
1189+
// `size_of(elems)` outgoing edges, rather than `len(elems)` edges. None of
1190+
// our existing tests happened to trigger this bug because although we were
1191+
// tricking the collector into tracing unallocated GC heap memory, it was
1192+
// all zeroed out and was treated as null GC references. We can avoid that
1193+
// in this regression test by first painting the heap with a large poison
1194+
// value before we start allocating arrays, so that if the GC tries tracing
1195+
// bogus heap memory, it finds very large GC ref heap indices and ultimately
1196+
// tries to follow them outside the bounds of the GC heap, which (before
1197+
// this bug was fixed) would lead to a panic.
1198+
1199+
let array_i8_ty = ArrayType::new(&engine, FieldType::new(Mutability::Var, StorageType::I8));
1200+
let array_i8_pre = ArrayRefPre::new(&mut store, array_i8_ty);
1201+
1202+
{
1203+
let mut store = RootScope::new(&mut store);
1204+
1205+
// Spray a poison pattern across the heap.
1206+
let len = 1_000_000;
1207+
let _poison = ArrayRef::new(&mut store, &array_i8_pre, &Val::I32(-1), len);
1208+
}
1209+
1210+
// Make sure the poison array is collected.
1211+
store.gc();
1212+
1213+
// Allocate and then collect an array of GC refs from Wasm. This should not
1214+
// trick the collector into tracing any poison and panicking.
1215+
let module = Module::new(
1216+
&engine,
1217+
r#"
1218+
(module
1219+
(type $ty (array (mut anyref)))
1220+
(start $f)
1221+
(func $f
1222+
(drop (array.new $ty (ref.null any) (i32.const 1_000)))
1223+
)
1224+
)
1225+
"#,
1226+
)?;
1227+
let _instance = Instance::new(&mut store, &module, &[])?;
1228+
store.gc();
1229+
1230+
Ok(())
1231+
}

‎tests/disas/gc/drc/array-new-fixed-of-gc-refs.wat‎

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
;;! target = "x86_64"
2+
;;! flags = "-W function-references,gc -C collector=drc"
3+
;;! test = "optimize"
4+
5+
(module
6+
(type $ty (array (mut anyref)))
7+
8+
(func (param anyref anyref anyref) (result (ref $ty))
9+
(array.new_fixed $ty 3 (local.get 0) (local.get 1) (local.get 2))
10+
)
11+
)
12+
;; function u0:0(i64 vmctx, i64, i32, i32, i32) -> i32 tail {
13+
;; ss0 = explicit_slot 4, align = 4
14+
;; ss1 = explicit_slot 4, align = 4
15+
;; ss2 = explicit_slot 4, align = 4
16+
;; gv0 = vmctx
17+
;; gv1 = load.i64 notrap aligned readonly gv0+8
18+
;; gv2 = load.i64 notrap aligned gv1+16
19+
;; gv3 = vmctx
20+
;; sig0 = (i64 vmctx, i32, i32, i32, i32) -> i64 tail
21+
;; fn0 = colocated u1:27 sig0
22+
;; stack_limit = gv2
23+
;;
24+
;; block0(v0: i64, v1: i64, v2: i32, v3: i32, v4: i32):
25+
;; v131 = stack_addr.i64 ss2
26+
;; store notrap v2, v131
27+
;; v132 = stack_addr.i64 ss1
28+
;; store notrap v3, v132
29+
;; v133 = stack_addr.i64 ss0
30+
;; store notrap v4, v133
31+
;; v171 = iconst.i64 0
32+
;; @0025 trapnz v171, user18 ; v171 = 0
33+
;; @0025 v7 = iconst.i32 28
34+
;; v172 = iconst.i32 12
35+
;; @0025 v12 = uadd_overflow_trap v7, v172, user18 ; v7 = 28, v172 = 12
36+
;; v173 = iconst.i32 -1476395005
37+
;; @0025 v16 = iconst.i32 0
38+
;; @0025 v17 = iconst.i32 8
39+
;; @0025 v18 = call fn0(v0, v173, v16, v12, v17), stack_map=[i32 @ ss2+0, i32 @ ss1+0, i32 @ ss0+0] ; v173 = -1476395005, v16 = 0, v17 = 8
40+
;; @0025 v6 = iconst.i32 3
41+
;; @0025 v21 = load.i64 notrap aligned readonly can_move v0+40
42+
;; @0025 v19 = ireduce.i32 v18
43+
;; @0025 v22 = uextend.i64 v19
44+
;; @0025 v23 = iadd v21, v22
45+
;; v136 = iconst.i64 24
46+
;; @0025 v24 = iadd v23, v136 ; v136 = 24
47+
;; @0025 store notrap aligned v6, v24 ; v6 = 3
48+
;; v130 = load.i32 notrap v131
49+
;; v138 = iconst.i32 1
50+
;; @0025 v29 = band v130, v138 ; v138 = 1
51+
;; @0025 v30 = icmp eq v130, v16 ; v16 = 0
52+
;; @0025 v31 = uextend.i32 v30
53+
;; @0025 v32 = bor v29, v31
54+
;; @0025 brif v32, block3, block2
55+
;;
56+
;; block2:
57+
;; @0025 v37 = uextend.i64 v130
58+
;; @0025 v96 = iconst.i64 8
59+
;; @0025 v39 = uadd_overflow_trap v37, v96, user1 ; v96 = 8
60+
;; @0025 v41 = uadd_overflow_trap v39, v96, user1 ; v96 = 8
61+
;; @0025 v94 = load.i64 notrap aligned readonly can_move v0+48
62+
;; @0025 v42 = icmp ule v41, v94
63+
;; @0025 trapz v42, user1
64+
;; @0025 v43 = iadd.i64 v21, v39
65+
;; @0025 v44 = load.i64 notrap aligned v43
66+
;; v158 = iconst.i64 1
67+
;; @0025 v45 = iadd v44, v158 ; v158 = 1
68+
;; @0025 store notrap aligned v45, v43
69+
;; @0025 jump block3
70+
;;
71+
;; block3:
72+
;; v126 = load.i32 notrap v131
73+
;; v181 = iconst.i64 28
74+
;; v187 = iadd.i64 v23, v181 ; v181 = 28
75+
;; @0025 store notrap aligned little v126, v187
76+
;; v125 = load.i32 notrap v132
77+
;; v211 = iconst.i32 1
78+
;; v212 = band v125, v211 ; v211 = 1
79+
;; v213 = iconst.i32 0
80+
;; v214 = icmp eq v125, v213 ; v213 = 0
81+
;; @0025 v60 = uextend.i32 v214
82+
;; @0025 v61 = bor v212, v60
83+
;; @0025 brif v61, block5, block4
84+
;;
85+
;; block4:
86+
;; @0025 v66 = uextend.i64 v125
87+
;; v215 = iconst.i64 8
88+
;; @0025 v68 = uadd_overflow_trap v66, v215, user1 ; v215 = 8
89+
;; @0025 v70 = uadd_overflow_trap v68, v215, user1 ; v215 = 8
90+
;; v216 = load.i64 notrap aligned readonly can_move v0+48
91+
;; @0025 v71 = icmp ule v70, v216
92+
;; @0025 trapz v71, user1
93+
;; @0025 v72 = iadd.i64 v21, v68
94+
;; @0025 v73 = load.i64 notrap aligned v72
95+
;; v217 = iconst.i64 1
96+
;; @0025 v74 = iadd v73, v217 ; v217 = 1
97+
;; @0025 store notrap aligned v74, v72
98+
;; @0025 jump block5
99+
;;
100+
;; block5:
101+
;; v121 = load.i32 notrap v132
102+
;; v135 = iconst.i64 32
103+
;; v194 = iadd.i64 v23, v135 ; v135 = 32
104+
;; @0025 store notrap aligned little v121, v194
105+
;; v120 = load.i32 notrap v133
106+
;; v218 = iconst.i32 1
107+
;; v219 = band v120, v218 ; v218 = 1
108+
;; v220 = iconst.i32 0
109+
;; v221 = icmp eq v120, v220 ; v220 = 0
110+
;; @0025 v89 = uextend.i32 v221
111+
;; @0025 v90 = bor v219, v89
112+
;; @0025 brif v90, block7, block6
113+
;;
114+
;; block6:
115+
;; @0025 v95 = uextend.i64 v120
116+
;; v222 = iconst.i64 8
117+
;; @0025 v97 = uadd_overflow_trap v95, v222, user1 ; v222 = 8
118+
;; @0025 v99 = uadd_overflow_trap v97, v222, user1 ; v222 = 8
119+
;; v223 = load.i64 notrap aligned readonly can_move v0+48
120+
;; @0025 v100 = icmp ule v99, v223
121+
;; @0025 trapz v100, user1
122+
;; @0025 v101 = iadd.i64 v21, v97
123+
;; @0025 v102 = load.i64 notrap aligned v101
124+
;; v224 = iconst.i64 1
125+
;; @0025 v103 = iadd v102, v224 ; v224 = 1
126+
;; @0025 store notrap aligned v103, v101
127+
;; @0025 jump block7
128+
;;
129+
;; block7:
130+
;; v116 = load.i32 notrap v133
131+
;; v196 = iconst.i64 36
132+
;; v202 = iadd.i64 v23, v196 ; v196 = 36
133+
;; @0025 store notrap aligned little v116, v202
134+
;; @0029 jump block1
135+
;;
136+
;; block1:
137+
;; @0029 return v19
138+
;; }

‎tests/disas/gc/drc/array-new-fixed.wat‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,21 +21,21 @@
2121
;; block0(v0: i64, v1: i64, v2: i64, v3: i64, v4: i64):
2222
;; v45 = iconst.i64 0
2323
;; @0025 trapnz v45, user18 ; v45 = 0
24-
;; @0025 v6 = iconst.i32 32
24+
;; @0025 v7 = iconst.i32 32
2525
;; v46 = iconst.i32 24
26-
;; @0025 v12 = uadd_overflow_trap v6, v46, user18 ; v6 = 32, v46 = 24
26+
;; @0025 v12 = uadd_overflow_trap v7, v46, user18 ; v7 = 32, v46 = 24
2727
;; @0025 v15 = iconst.i32 -1476395008
2828
;; @0025 v13 = iconst.i32 0
2929
;; @0025 v18 = iconst.i32 8
3030
;; @0025 v19 = call fn0(v0, v15, v13, v12, v18) ; v15 = -1476395008, v13 = 0, v18 = 8
31-
;; @0025 v7 = iconst.i32 3
31+
;; @0025 v6 = iconst.i32 3
3232
;; @0025 v22 = load.i64 notrap aligned readonly can_move v0+40
3333
;; @0025 v20 = ireduce.i32 v19
3434
;; @0025 v23 = uextend.i64 v20
3535
;; @0025 v24 = iadd v22, v23
3636
;; v37 = iconst.i64 24
3737
;; @0025 v25 = iadd v24, v37 ; v37 = 24
38-
;; @0025 store notrap aligned v7, v25 ; v7 = 3
38+
;; @0025 store notrap aligned v6, v25 ; v6 = 3
3939
;; v34 = iconst.i64 32
4040
;; v59 = iadd v24, v34 ; v34 = 32
4141
;; @0025 store notrap aligned little v2, v59

‎tests/disas/gc/null/array-new-fixed-of-gc-refs.wat‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
;;! target = "x86_64"
2+
;;! flags = "-W function-references,gc -C collector=null"
3+
;;! test = "optimize"
4+
5+
(module
6+
(type $ty (array (mut anyref)))
7+
8+
(func (param anyref anyref anyref) (result (ref $ty))
9+
(array.new_fixed $ty 3 (local.get 0) (local.get 1) (local.get 2))
10+
)
11+
)
12+
;; function u0:0(i64 vmctx, i64, i32, i32, i32) -> i32 tail {
13+
;; gv0 = vmctx
14+
;; gv1 = load.i64 notrap aligned readonly gv0+8
15+
;; gv2 = load.i64 notrap aligned gv1+16
16+
;; gv3 = vmctx
17+
;; stack_limit = gv2
18+
;;
19+
;; block0(v0: i64, v1: i64, v2: i32, v3: i32, v4: i32):
20+
;; v59 = iconst.i64 0
21+
;; @0025 trapnz v59, user18 ; v59 = 0
22+
;; @0025 v7 = iconst.i32 12
23+
;; @0025 v12 = uadd_overflow_trap v7, v7, user18 ; v7 = 12, v7 = 12
24+
;; @0025 v14 = iconst.i32 -134217728
25+
;; @0025 v15 = band v12, v14 ; v14 = -134217728
26+
;; @0025 trapnz v15, user18
27+
;; @0025 v17 = load.i64 notrap aligned readonly v0+56
28+
;; @0025 v18 = load.i32 notrap aligned v17
29+
;; v60 = iconst.i32 7
30+
;; @0025 v21 = uadd_overflow_trap v18, v60, user18 ; v60 = 7
31+
;; v67 = iconst.i32 -8
32+
;; @0025 v23 = band v21, v67 ; v67 = -8
33+
;; @0025 v24 = uadd_overflow_trap v23, v12, user18
34+
;; @0025 v25 = uextend.i64 v24
35+
;; @0025 v29 = load.i64 notrap aligned readonly can_move v0+48
36+
;; @0025 v30 = icmp ule v25, v29
37+
;; @0025 trapz v30, user18
38+
;; @0025 v33 = iconst.i32 -1476395008
39+
;; v68 = bor v12, v33 ; v33 = -1476395008
40+
;; @0025 v27 = load.i64 notrap aligned readonly can_move v0+40
41+
;; @0025 v31 = uextend.i64 v23
42+
;; @0025 v32 = iadd v27, v31
43+
;; @0025 store notrap aligned v68, v32
44+
;; @0025 v36 = load.i64 notrap aligned readonly can_move v0+64
45+
;; @0025 v37 = load.i32 notrap aligned readonly can_move v36
46+
;; @0025 store notrap aligned v37, v32+4
47+
;; @0025 store notrap aligned v24, v17
48+
;; @0025 v6 = iconst.i32 3
49+
;; v48 = iconst.i64 8
50+
;; @0025 v38 = iadd v32, v48 ; v48 = 8
51+
;; @0025 store notrap aligned v6, v38 ; v6 = 3
52+
;; v50 = iconst.i64 12
53+
;; v76 = iadd v32, v50 ; v50 = 12
54+
;; @0025 store notrap aligned little v2, v76
55+
;; v78 = iconst.i64 16
56+
;; v84 = iadd v32, v78 ; v78 = 16
57+
;; @0025 store notrap aligned little v3, v84
58+
;; v86 = iconst.i64 20
59+
;; v92 = iadd v32, v86 ; v86 = 20
60+
;; @0025 store notrap aligned little v4, v92
61+
;; @0029 jump block1
62+
;;
63+
;; block1:
64+
;; v101 = band.i32 v21, v67 ; v67 = -8
65+
;; @0029 return v101
66+
;; }

‎tests/disas/gc/null/array-new-fixed.wat‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,9 @@
1919
;; block0(v0: i64, v1: i64, v2: i64, v3: i64, v4: i64):
2020
;; v58 = iconst.i64 0
2121
;; @0025 trapnz v58, user18 ; v58 = 0
22-
;; @0025 v6 = iconst.i32 16
22+
;; @0025 v7 = iconst.i32 16
2323
;; v59 = iconst.i32 24
24-
;; @0025 v12 = uadd_overflow_trap v6, v59, user18 ; v6 = 16, v59 = 24
24+
;; @0025 v12 = uadd_overflow_trap v7, v59, user18 ; v7 = 16, v59 = 24
2525
;; @0025 v14 = iconst.i32 -134217728
2626
;; @0025 v15 = band v12, v14 ; v14 = -134217728
2727
;; @0025 trapnz v15, user18
@@ -46,10 +46,10 @@
4646
;; @0025 v37 = load.i32 notrap aligned readonly can_move v36
4747
;; @0025 store notrap aligned v37, v32+4
4848
;; @0025 store notrap aligned v24, v17
49-
;; @0025 v7 = iconst.i32 3
49+
;; @0025 v6 = iconst.i32 3
5050
;; v46 = iconst.i64 8
5151
;; @0025 v38 = iadd v32, v46 ; v46 = 8
52-
;; @0025 store notrap aligned v7, v38 ; v7 = 3
52+
;; @0025 store notrap aligned v6, v38 ; v6 = 3
5353
;; v71 = iconst.i64 16
5454
;; v77 = iadd v32, v71 ; v71 = 16
5555
;; @0025 store notrap aligned little v2, v77

0 commit comments

Comments
 (0)