[Wasm Exceptions] Fix binary parsing of a normal break to a try in a singleton

[kripken]

(Contains #3574 as I couldn't figure out how to make a PR in this repo that is relative to a fork...)

The fix here is to remove the code with

// maybe we don't need a block here?

That would remove a try's block if we thought it wasn't needed. However,
it just checked for exception branches, but not normal branches, which are
also possible.

At that location, we don't have a good way to find out if the block has other
branches to it aside from scanning its contents. So this PR just gives up on
doing so, which means we add an unnecessary block if the optimizer is not
run. If this matters we could make the binary parser more complicated by
remembering whether a block had branches in the past, but I'm not sure if
it's worth it.

Found by emscripten-core/emscripten#13485 when also running binaryen on the wasm output. Original testcase:

extern bool getBoolean();

int main() {
  try {
    throw 0;
  } catch(int) {
    while (getBoolean()) {
      getBoolean();
    }
  }
}

STR:

$ emcc a.cpp -c -fwasm-exceptions -Os && wasm-opt a.o -all
warning: linking section is present, so this is not a standard wasm file - binaryen cannot handle this properly!
[wasm-validator error in function 0] unexpected false: all break targets must be valid, on 
(br_if $label$4
 (i32.eqz
  (local.get $1)
 )
)
Fatal: error validating input

The specific problem there (and in the attached testcase) is

        try  ;; label = @3
          loop  ;; label = @4
            call 4
            local.set 1
            local.get 1
            i32.eqz
            br_if 1 (;@3;)

That br_if targets the try. The loop is a singleton child of the try,
and so the code would remove the block and replace it with the
loop, leaving a break to a non-existent target.

[aheejin]

Thank you for fixing this! Yeah, I see what I missed.. By the way I think this can be a minimal test case that reproduces the bug. Would using this be simpler?
break-target-try.tar.gz

This was generated from this wat file, using wat2wasm:

(module
  (type (;0;) (func))
  (type (;1;) (func (param i32)))
  (func (;0;) (type 0)
    try  ;; label = @1
      br 0 (;@1;)
    catch 0
      drop
    end)
  (event (;0;) (type 1) (param i32)))

[tlively]

I assume this is just to document where br_if_to_try.wasm comes from? If so, it would be good to add a comment with the command to reproduce br_if_to_try.wasm in the future.

[tlively]

Oh, even better to go with @aheejin's suggested test case.

[kripken]

Thanks for the testcase!

Yes, the bug should happen for any singleton there, so a Loop was not needed, and most of the other stuff...

[aheejin]

Nit: This contains br, not br_if, so maybe the file name is misleading..?

[aheejin]

Also if this does not need any "passes", maybe we can put the wasm file in test/ ? (I'm not really sure what the convention is, I guess you know better though)

[kripken]

Good points, yes, this is better in test/. Will move and simplify as you suggest.

[aheejin]

I guess we don't need this file anymore?