Avoid generating ref.as_non_null for table initializers in fuzzer

[stevenfontanella]

Fixes #8911. ref.as_non_null is not a constant expression, so it's not allowed in table initializers. Repro output with this PR.

See the equivalent code in setupGlobals.

[tlively]

I guess setupGlobals() must already do something similar, but what happens to instructions that reference the table whose type is changed? How are they updated to account for the new type?

Can we add a new .wast file to the test suite that the fuzzer will pick up as initial contents that will trigger this new code path?

[stevenfontanella]

Good point. I looked into this, and it looks like in the source module, it would be possible to have an imported global/table of a non-nullable type that can't possibly be initialized in a constant expression. So when !preserveImportsAndExports is the case, we try to replace it with a module-defined global/table and hit the fallback code in setupGlobals() / finalizeTable() in this PR. So the repro from #8911 would still fail if we add another expression that references the global/table in a way that the type matters.

I'm not fully clear on what the use case for !preserveImportsAndExports is. The fixes I can think of are to just give up in this case and exit, or we can change the type to nullable, then change all usages of the global/table and add the ref.as_non_null cast there instead. Any thoughts?

[stevenfontanella]

I tried to repro the setupGlobals() bug from main and I weirdly got a different error than expected:

cat seed.wast:

(module
  (import "env" "global" (global $g (ref exn)))
  (func $test (result (ref exn))
    (global.get $g)
  )
)

bin/wasm-opt -ttf -if seed.wast -all /dev/null -o /dev/null

[wasm-validator error in module] unexpected false: global init must be constant, on
g
 (global $g (ref exn) (ref.as_non_null
  (ref.null noexn)
 ))

So weirdly the fallback code in setupGlobals didn't seem to take effect at all. But even if it had, it seems that it would change the type in a way that causes $test to fail to type check.

[tlively]

I'm not fully clear on what the use case for !preserveImportsAndExports is.

This is the default case, where the fuzzer just wants to generate some arbitrary module and there is no need to preserve the import/export interface of the abitrarily chosen initial module.

The fixes I can think of are to just give up in this case and exit, or we can change the type to nullable, then change all usages of the global/table and add the ref.as_non_null cast there instead. Any thoughts?

I don't think we have precedent for the fuzzer just giving up for a chosen initial contents, and I don't think we would want to start doing that. We do have precedent for adding test cases to a block list that prevents the fuzzer from using them as initial contents, though. We can either continue relying on that mechanism and just leave the fuzzer unable to fix up this case for now, or we can change all the usages as you said.

[stevenfontanella]

I don't think we have precedent for the fuzzer just giving up for a chosen initial contents, and I don't think we would want to start doing that.

But the fuzzer is already generating invalid code and panicking for this case today, it's just not giving up cleanly. I'm just saying that the option is to remove the (incorrect) code that tries to fixup module-defined globals that can't be initialized with a constant expression, and exit with an error instead.

I would lean towards doing this option and then punt the transformation for non-initializable globals/tables for later if it's needed. It seems that this case is broken today anyway. In the case of waitqueue, the outcome would be that we wouldn't be able to fuzz modules that import a non-nullable waitqueue, which is the same as with most other refs already. At least I'd prefer to implement the waitqueue fuzzing this way first to begin with and then later fix up every case if it's needed. Does that sound reasonable?

[tlively]

But the fuzzer is already generating invalid code and panicking for this case today, it's just not giving up cleanly.

But presumably not when run from scripts/fuzz_opt.py, otherwise we would have noticed this earlier, right?

But yeah, cleaning up the incorrect code and exiting with an error would still be an improvement over the status quo, good point.