Verification error after function inlining

[igoriakovlev]

Hi,
It seems binaryen (7d5d24f) creates invalid blocks after function body inlining.

Steps to reproduce:

1. Download wasm file from here and unpack index.wasm file
2. Run wasm-opt --enable-gc --enable-reference-types --enable-exception-handling --enable-bulk-memory --enable-nontrapping-float-to-int -O4 index.wasm -o indexOpt.wasm
3. Get error:
   [wasm-validator error in function kotlin.wasm.internal.jsToKotlinAnyAdapter] unexpected false: non-final block elements returning a value must be drop()ed (binaryen's autodrop option might help you), on (block $__inlined_func$kotlin.wasm.internal.externRefToAny$53 (result (ref null $kotlin.Any)) (block $label$1 (result anyref) (br $__inlined_func$kotlin.wasm.internal.externRefToAny$53 (br_on_cast_fail $label$1 $kotlin.Any (extern.internalize (local.get $0) ) ) ) ) (call $kotlin.wasm.internal.jsToKotlinAnyAdapter (call $kotlin.wasm.internal.jsCheckIsNullOrUndefinedAdapter (call $fimport$8 (local.get $0) (extern.externalize (struct.new $kotlin.wasm.internal.JsExternalBox (global.get $kotlin.wasm.internal.JsExternalBox.vtable) (ref.null none) (i32.const 888) (i32.const 0) (local.get $0) ) ) ) ) ) ) (on index 0: 0x13081ce08 ), type: anyref Fatal: error after opts

It seems br_on_cast_fail return value is not dropped (the original wasm file contains this drop)

With -O3 everything works fine.
V8 and JsShell works fine.

[kripken]

This is a limitation of "flat" mode, which is used in -O4. In that mode we assume that blocks do not have return values, which is much flatter and simpler, and we need to be in that form to do some control flow optimizations that we do in -O4. The problem that is hit in this wasm module is that it uses br_on_cast_fail, and that instruction always sends a value to the target block - it cannot be removed, unlike br and switch.

We could add logic to lower br_on_cast, br_on_cast_fail to simpler sequences (br_on_cast => if cast then br else return value). However, that would make them less efficient, so it's not clear to me if it's worth it.

As a workaround for now, you can use -O3 or avoid using those branch instructions.

How much of a benefit do you get from -O4 vs -O3? If it's large then that could motivate lowering br_on_cast*.

[redianthus]

I'm hitting the same problem in Wasocaml:

$ wasm-opt -O4 --enable-gc --enable-tail-call --enable-reference-types --enable-multivalue --enable-exception-handling a.out.wasm -o a.out.optimised.wasm
[wasm-validator error in function 2] unexpected false: non-final block elements returning a value must be drop()ed (binaryen's autodrop option might help you), on 
(block
 (block $label$1 (result (ref $array.0))
  (return
   (ref.i31
    (array.len
     (ref.cast (ref $array.1)
      (br_on_cast $label$1 (ref eq) (ref $array.0)
       (local.get $0)
      )
     )
    )
   )
  )
 )
 (unreachable)
)
(on index 0:
0x56520e876a48
), type: (ref $array.0)
[wasm-validator error in function 20] unexpected false: non-final block elements returning a value must be drop()ed (binaryen's autodrop option might help you), on 
(block
 (block $label$8 (result (ref $array.0))
  (drop
   (br_on_cast $label$8 (ref eq) (ref $array.0)
    (local.get $0)
   )
  )
  (return_call $17
   (local.get $0)
   (local.get $1)
  )
 )
 (unreachable)
)
(on index 0:
0x56520e878c78
), type: (ref $array.0)
[wasm-validator error in function 23] unexpected false: non-final block elements returning a value must be drop()ed (binaryen's autodrop option might help you), on 
(block $label$7
 (block $label$8 (result (ref $array.0))
  (drop
   (br_on_cast $label$8 (ref eq) (ref $array.0)
    (local.get $0)
   )
  )
  (return_call $24
   (local.get $0)
   (local.get $1)
  )
 )
 (drop
  (unreachable)
 )
)
(on index 0:
0x56520e879858
), type: (ref $array.0)
Fatal: error after opts

As suggested per:

binaryen's autodrop option might help you

I tried to use this option but couldn't find it. Does it still exist?

[kripken]

@zapashcanon

Does [autodrop] still exist?

Yes, you can use it from the C API (or directly from C++),

binaryen/src/binaryen-c.h

Lines 3049 to 3052 in 0be8d5e

	// Auto-generate drop() operations where needed. This lets you generate code
	// without worrying about where they are needed. (It is more efficient to do it
	// yourself, but simpler to use autodrop).
	BINARYEN_API void BinaryenModuleAutoDrop(BinaryenModuleRef module);

But as the comment there says, it is better to emit the drops yourself.

If you are seeing those errors as the result of optimizations, then that sounds like a compiler bug. Is that what you reported in #6989?

[redianthus]

Yes, you can use it from the C API (or directly from C++),

Oh OK, I see. I'm not using Binaryen through the API. Is there a plan to add a CLI flag for it?

But as the comment there says, it is better to emit the drops yourself.
If you are seeing those errors as the result of optimizations, then that sounds like a compiler bug. Is that what you reported in #6989?

No, I believe it is a different issue than #6989.

The program is produced after merging many modules together through wasm-merge. I tried running the same command on all input modules and it seems the issue is already present in one of them. I'll try to fix this and see if it's also introduced by wasm-merge.

[kripken]

The program is produced after merging many modules together through wasm-merge. I tried running the same command on all input modules and it seems the issue is already present in one of them. I'll try to fix this and see if it's also introduced by wasm-merge.

Definitely sounds like a compiler bug, either in wasm-merge or earlier. If you can't figure it out, feel free to file the smallest testcase you can get for it, and I can take a look.

I'm not using Binaryen through the API. Is there a plan to add a CLI flag for it?

That operation transforms invalid IR into valid IR, so it only makes sense to do while generating the IR in the first place. So it can't be a CLI flag - it is meant to help in another type of usage. Anyhow, as this is a compiler bug, I think the suggestion to run AutoDrop is not relevant for this issue here.