Skip to content

Commit 772891f

Browse files
aheejinaheejin
authored
Fix extra unreachable generation (#2266)
Currently various expressions handle this differently, and now we consistently follow this rules: --- For all non-control-flow value-returning instructions, if a type of an expression is unreachable, we emit an unreachable and don't emit the instruction itself. If we don't emit an unreachable, instructions that follow can have validation failure in wasm binary format. For example: ``` [unreachable] (f32.add [unreachable] (i32.eqz [unreachable] (unreachable) ) ... ) ``` This is a valid prgram in binaryen IR, because the unreachable type propagates out of an expression, making both i32.eqz and f32.add unreachable. But in binary format, this becomes: ``` unreachable i32.eqz f32.add ;; validation failure; it expects f32 but takes an i32! ``` And here f32.add causes validation failure in wasm validation. So in this case we add an unreachable to prevent following instructions to consume the current value (here i32.eqz). In actual tests, I used `global.get` to an f32 global, which does not return a value, instead of `f32.add`, because `f32.add` itself will not be emitted if one of argument is unreachable. --- So the changes are: - For instructions that don't return a value, removes unreachable emitting code if it exists. - Add the unreachable emitting code for value-returning instructions if there isn't one. - Check for unreachability only once after emitting all children for atomic instructions. Currently only atomic instructions check unreachability after visiting each children and bail out right after, which is valid, but not consistent with others. - Don't emit an extra unreachable after a return (and return_call). I guess it is unnecessary.
1 parent edf001f commit 772891f

7 files changed

Lines changed: 412 additions & 46 deletions

‎src/wasm-stack.h‎

Lines changed: 55 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -382,11 +382,33 @@ void BinaryenIRWriter<SubType>::visitCall(Call* curr) {
382382
for (auto* operand : curr->operands) {
383383
visit(operand);
384384
}
385-
emit(curr);
386-
// TODO FIXME: this and similar can be removed
387-
if (curr->type == unreachable) {
385+
386+
// For non-control-flow value-returning instructions, if the type of an
387+
// expression is unreachable, we emit an unreachable and don't emit the
388+
// instruction itself. If we don't emit an unreachable, instructions that
389+
// follow can have a validation failure in wasm binary format. For example:
390+
// [unreachable] (f32.add
391+
// [unreachable] (i32.eqz
392+
// [unreachable] (unreachable)
393+
// )
394+
// ...
395+
// )
396+
// This is a valid prgram in binaryen IR, because the unreachable type
397+
// propagates out of an expression, making both i32.eqz and f32.add
398+
// unreachable. But in binary format, this becomes:
399+
// unreachable
400+
// i32.eqz
401+
// f32.add ;; validation failure; it takes an i32!
402+
// And here f32.add causes validation failure in wasm validation. So in this
403+
// case we add an unreachable to prevent following instructions to consume
404+
// the current value (here i32.eqz).
405+
//
406+
// The same applies for other expressions.
407+
if (curr->type == unreachable && !curr->isReturn) {
388408
emitUnreachable();
409+
return;
389410
}
411+
emit(curr);
390412
}
391413

392414
template<typename SubType>
@@ -395,10 +417,11 @@ void BinaryenIRWriter<SubType>::visitCallIndirect(CallIndirect* curr) {
395417
visit(operand);
396418
}
397419
visit(curr->target);
398-
emit(curr);
399-
if (curr->type == unreachable) {
420+
if (curr->type == unreachable && !curr->isReturn) {
400421
emitUnreachable();
422+
return;
401423
}
424+
emit(curr);
402425
}
403426

404427
template<typename SubType>
@@ -409,10 +432,11 @@ void BinaryenIRWriter<SubType>::visitLocalGet(LocalGet* curr) {
409432
template<typename SubType>
410433
void BinaryenIRWriter<SubType>::visitLocalSet(LocalSet* curr) {
411434
visit(curr->value);
412-
emit(curr);
413-
if (curr->type == unreachable) {
435+
if (curr->isTee() && curr->type == unreachable) {
414436
emitUnreachable();
437+
return;
415438
}
439+
emit(curr);
416440
}
417441

418442
template<typename SubType>
@@ -430,7 +454,6 @@ template<typename SubType>
430454
void BinaryenIRWriter<SubType>::visitLoad(Load* curr) {
431455
visit(curr->ptr);
432456
if (curr->type == unreachable) {
433-
// don't even emit it; we don't know the right type
434457
emitUnreachable();
435458
return;
436459
}
@@ -441,27 +464,14 @@ template<typename SubType>
441464
void BinaryenIRWriter<SubType>::visitStore(Store* curr) {
442465
visit(curr->ptr);
443466
visit(curr->value);
444-
if (curr->type == unreachable) {
445-
// don't even emit it; we don't know the right type
446-
emitUnreachable();
447-
return;
448-
}
449467
emit(curr);
450468
}
451469

452470
template<typename SubType>
453471
void BinaryenIRWriter<SubType>::visitAtomicRMW(AtomicRMW* curr) {
454472
visit(curr->ptr);
455-
// stop if the rest isn't reachable anyhow
456-
if (curr->ptr->type == unreachable) {
457-
return;
458-
}
459473
visit(curr->value);
460-
if (curr->value->type == unreachable) {
461-
return;
462-
}
463474
if (curr->type == unreachable) {
464-
// don't even emit it; we don't know the right type
465475
emitUnreachable();
466476
return;
467477
}
@@ -471,20 +481,9 @@ void BinaryenIRWriter<SubType>::visitAtomicRMW(AtomicRMW* curr) {
471481
template<typename SubType>
472482
void BinaryenIRWriter<SubType>::visitAtomicCmpxchg(AtomicCmpxchg* curr) {
473483
visit(curr->ptr);
474-
// stop if the rest isn't reachable anyhow
475-
if (curr->ptr->type == unreachable) {
476-
return;
477-
}
478484
visit(curr->expected);
479-
if (curr->expected->type == unreachable) {
480-
return;
481-
}
482485
visit(curr->replacement);
483-
if (curr->replacement->type == unreachable) {
484-
return;
485-
}
486486
if (curr->type == unreachable) {
487-
// don't even emit it; we don't know the right type
488487
emitUnreachable();
489488
return;
490489
}
@@ -494,16 +493,10 @@ void BinaryenIRWriter<SubType>::visitAtomicCmpxchg(AtomicCmpxchg* curr) {
494493
template<typename SubType>
495494
void BinaryenIRWriter<SubType>::visitAtomicWait(AtomicWait* curr) {
496495
visit(curr->ptr);
497-
// stop if the rest isn't reachable anyhow
498-
if (curr->ptr->type == unreachable) {
499-
return;
500-
}
501496
visit(curr->expected);
502-
if (curr->expected->type == unreachable) {
503-
return;
504-
}
505497
visit(curr->timeout);
506-
if (curr->timeout->type == unreachable) {
498+
if (curr->type == unreachable) {
499+
emitUnreachable();
507500
return;
508501
}
509502
emit(curr);
@@ -512,12 +505,9 @@ void BinaryenIRWriter<SubType>::visitAtomicWait(AtomicWait* curr) {
512505
template<typename SubType>
513506
void BinaryenIRWriter<SubType>::visitAtomicNotify(AtomicNotify* curr) {
514507
visit(curr->ptr);
515-
// stop if the rest isn't reachable anyhow
516-
if (curr->ptr->type == unreachable) {
517-
return;
518-
}
519508
visit(curr->notifyCount);
520-
if (curr->notifyCount->type == unreachable) {
509+
if (curr->type == unreachable) {
510+
emitUnreachable();
521511
return;
522512
}
523513
emit(curr);
@@ -526,20 +516,32 @@ void BinaryenIRWriter<SubType>::visitAtomicNotify(AtomicNotify* curr) {
526516
template<typename SubType>
527517
void BinaryenIRWriter<SubType>::visitSIMDExtract(SIMDExtract* curr) {
528518
visit(curr->vec);
519+
if (curr->type == unreachable) {
520+
emitUnreachable();
521+
return;
522+
}
529523
emit(curr);
530524
}
531525

532526
template<typename SubType>
533527
void BinaryenIRWriter<SubType>::visitSIMDReplace(SIMDReplace* curr) {
534528
visit(curr->vec);
535529
visit(curr->value);
530+
if (curr->type == unreachable) {
531+
emitUnreachable();
532+
return;
533+
}
536534
emit(curr);
537535
}
538536

539537
template<typename SubType>
540538
void BinaryenIRWriter<SubType>::visitSIMDShuffle(SIMDShuffle* curr) {
541539
visit(curr->left);
542540
visit(curr->right);
541+
if (curr->type == unreachable) {
542+
emitUnreachable();
543+
return;
544+
}
543545
emit(curr);
544546
}
545547

@@ -548,13 +550,21 @@ void BinaryenIRWriter<SubType>::visitSIMDBitselect(SIMDBitselect* curr) {
548550
visit(curr->left);
549551
visit(curr->right);
550552
visit(curr->cond);
553+
if (curr->type == unreachable) {
554+
emitUnreachable();
555+
return;
556+
}
551557
emit(curr);
552558
}
553559

554560
template<typename SubType>
555561
void BinaryenIRWriter<SubType>::visitSIMDShift(SIMDShift* curr) {
556562
visit(curr->vec);
557563
visit(curr->shift);
564+
if (curr->type == unreachable) {
565+
emitUnreachable();
566+
return;
567+
}
558568
emit(curr);
559569
}
560570

‎test/extra-unreachable.wast‎

Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,180 @@
1+
(module
2+
(type $ii (param i32) (result i32))
3+
(memory (shared 1 1))
4+
(table 0 funcref)
5+
(global $g (mut f32) (f32.const 0))
6+
7+
(func $foo (param i32) (result i32) (i32.const 0))
8+
9+
(func $test_function_block
10+
;; block, which has unreachable type, can be omitted here because it is the
11+
;; only expression within a function. We emit an extra unreachable at the
12+
;; end.
13+
(block
14+
(unreachable)
15+
(nop)
16+
)
17+
)
18+
19+
(func $test
20+
;; block has unreachable type. We emit an unreachable at the end of the
21+
;; block and also outside the block too.
22+
(block
23+
(i32.eqz (unreachable))
24+
)
25+
26+
;; If an if's condition is unreachable, don't emit the if and emit an
27+
;; unreachable instead.
28+
(if
29+
(unreachable)
30+
(nop)
31+
(nop)
32+
)
33+
34+
;; If an if is unreachable, i.e., the both sides are unreachable, we emit
35+
;; an extra unreachable after the if.
36+
(if
37+
(i32.const 1)
38+
(unreachable)
39+
(unreachable)
40+
)
41+
42+
;; If a br_if's type is unreachable, emit an extra unreachable after it
43+
(block
44+
(br_if 0 (unreachable))
45+
)
46+
47+
;; If a br_table is not reachable, emit an unreachable instead
48+
(block $l
49+
(block $default
50+
(br_table $l $default
51+
(unreachable)
52+
)
53+
)
54+
)
55+
56+
;; For all tests below, when a value-returning expression is unreachable,
57+
;; emit an unreachable instead of the operation, to prevent it from being
58+
;; consumed by the next operation.
59+
;;
60+
;; Here global.set is used to consume the value. If an unreachable is not
61+
;; emitted, the instruction itself will be consumed by global.set and will
62+
;; result in a type validation failure, because it expects a f32 but gets an
63+
;; i32. The reason we use global.set is the instruction does not return a
64+
;; value, so it will be emitted even if its argument is unreachable.
65+
66+
;; call / call_indirect
67+
(global.set $g
68+
(call $foo (unreachable))
69+
)
70+
(global.set $g
71+
(call_indirect (type $ii) (unreachable))
72+
)
73+
74+
;; unary
75+
(global.set $g
76+
(i32.eqz (unreachable))
77+
)
78+
79+
;; binary
80+
(global.set $g
81+
(i32.add
82+
(unreachable)
83+
(i32.const 0)
84+
)
85+
)
86+
87+
;; select
88+
(global.set $g
89+
(select
90+
(unreachable)
91+
(i32.const 0)
92+
(i32.const 0)
93+
)
94+
)
95+
96+
;; load
97+
(global.set $g
98+
(i32.load (unreachable))
99+
)
100+
(global.set $g
101+
(i32.atomic.load (unreachable))
102+
)
103+
104+
;; atomic.rmw
105+
(global.set $g
106+
(i32.atomic.rmw.add
107+
(unreachable)
108+
(i32.const 0)
109+
)
110+
)
111+
112+
;; atomic.cmpxchg
113+
(global.set $g
114+
(i32.atomic.rmw.cmpxchg
115+
(unreachable)
116+
(i32.const 0)
117+
(i32.const 0)
118+
)
119+
)
120+
121+
;; atomic.wait
122+
(global.set $g
123+
(i32.atomic.wait
124+
(unreachable)
125+
(i32.const 0)
126+
(i64.const 0)
127+
)
128+
)
129+
130+
;; atomic.notify
131+
(global.set $g
132+
(atomic.notify
133+
(unreachable)
134+
(i32.const 0)
135+
)
136+
)
137+
138+
;; SIMD extract
139+
(global.set $g
140+
(i32x4.extract_lane 0
141+
(unreachable)
142+
)
143+
)
144+
145+
;; SIMD replace
146+
(global.set $g
147+
(i32x4.replace_lane 0
148+
(unreachable)
149+
(i32.const 0)
150+
)
151+
)
152+
153+
;; SIMD shuffle
154+
(global.set $g
155+
(v8x16.shuffle 0 17 2 19 4 21 6 23 8 25 10 27 12 29 14 31
156+
(unreachable)
157+
(unreachable)
158+
)
159+
)
160+
161+
;; SIMD bitselect
162+
(global.set $g
163+
(v128.bitselect
164+
(unreachable)
165+
(unreachable)
166+
(unreachable)
167+
)
168+
)
169+
170+
;; SIMD shift
171+
(global.set $g
172+
(i32x4.shl
173+
(unreachable)
174+
(i32.const 0)
175+
)
176+
)
177+
178+
;; TODO Add exception handling instructions
179+
)
180+
)

0 commit comments

Comments
 (0)