[Wasm GC] Ignore unreachable sets for purposes of non-nullable locals

[kripken]

As pointed out by @tlively in #5599, we don't emit unreachable code in the binary
format, which means such sets can't help gets validate.

Fixes #5599

[kripken]

It turns out this is tricky. If we land this PR, then any pass that does a refinalize will need to fix up non-nullable locals, beceause refinalization can propagate unreachability - and with this PR, unreachability affects non-nullable local validation.

In practice , that means adding non-nullable fixups to 24 more passes (!) (all those that don't do it now, but do perform a refinalize). That's so many passes that probably we would want to just constantly run non-nullable fixups after every pass and not bother with requiresNonNullableLocalFixups() as a way to opt out, and we'd just suffer the slower compile times. But that seems a high cost for this particular corner case...

[tlively]

Another workaround would be to treat these local.sets specially in the binary writer and emit them even though we otherwise wouldn't.

Another fix would be in the spec/v8, where we could consider all locals to be initialized in unreachable code.

[tlively]

Filed WebAssembly/function-references#98 to discuss a spec change.

[kripken]

Another workaround would be to treat these local.sets specially in the binary writer and emit them even though we otherwise wouldn't.

Unfortunately, I found this wouldn't be simple, as atm we stop at the first block etc. child that is unreachable. So to do this, we'd need to continue onward to check for sets, which would add complexity and overhead.

[tlively]

What about the solution of continuing to skip emitting things even beyond the end of blocks, as long as those blocks are unnamed?

[kripken]

That doesn't seem simple. It would affect multiple functions, as e.g. visitPossibleBlockContents just sees a single block. So we'd need to pass that state around somehow. And we need to preserve the other properties that this code handles, like this:

binaryen/src/wasm-stack.h

Lines 237 to 241 in 5679e3b

	// children, since the latter won't be reached. This (together with logic in
	// the control flow visitors) also ensures that the final instruction in each
	// unreachable block is a source of unreachability, which means we don't need
	// to emit an extra `unreachable` before the end of the block to prevent type
	// errors.

It might be worth trying though, I'm just saying it's not obvious to me that it will work or work easily.

(OTOH, just running non-nullable local fixups, or just running dce before binary emitting, would definitely work and be very simple - but slow us down.)

[tlively]

This would all be much easier if we could separate iteration from other logic using generators built on C++20 coroutines :'(

[kripken]

I had the idea to notice in ReFinalize when we turn a set unreachable. That is rare, so doing more work in that case would be fine. And it's a change in one place (ReFinalize) and not in multiple passes. That works ok (with some fallout in LocalRefining, see code), but it doesn't address the more general problem, a set that is after unreachable code but not unreachable itself,

(module
 (func $2
  (local $3 (ref array))
  (drop
   (block (result nullref)
    (unreachable)
    (local.set $3
     (ref.as_non_null
      (ref.null none)
     )
    )
    (ref.null none)
   )
  )
  (drop
   (local.get $3)
  )
 )
)

That won't validate in v8 but it does work in Binaryen. Noticing that in ReFinalize is not simple and would, I think, add overhead (we'd need to track when we turn any child of anything unreachable, and see if there's a set after it).

[kripken]

As of #5677 we have a fuzzer workaround here, and nothing else seems urgent. Closing, and we'll see how the spec and other discussions go around this topic.