Open world `--gufa` bug

[mkustermann]

See the attached repro. Unpack

% tar xvzf issue1.tar.gz
issue1.mjs
issue1.wasm
issue1_module1.wasm
issue1.sh
pkg/dart2wasm/bin/run_wasm.js

Then optimize & run

% bash issue1.sh
unittest-suite-wait-for-done
wasm-function[7]:0x736: RuntimeError: unreachable
RuntimeError: unreachable
    at wasm://wasm/f733f216:wasm-function[7]:0x736
    at wasm://wasm/f733f216:wasm-function[24]:0xa6c
    at wasm://wasm/00030116:wasm-function[128]:0x5668
    at wasm://wasm/00030116:wasm-function[310]:0xa156
    at wasm://wasm/00030116:wasm-function[216]:0x78a5
    at wasm://wasm/00030116:wasm-function[153]:0x62a7
    at wasm://wasm/00030116:wasm-function[222]:0x7a54
    at wasm://wasm/00030116:wasm-function[143]:0x5cbd
    at wasm://wasm/00030116:wasm-function[142]:0x5c38
    at /tmp/a/issue1.mjs:129:49

1 pending unhandled Promise rejection(s) detected.

Now do the same (unpack & run) but before running, modify issue1.sh script to remove --gufa and we get

% vim issue1.sh
... remove --gufa flags ...
% bash issue1.sh
unittest-suite-wait-for-done
A: hello
B: heyworld
B: heyfoo
unittest-suite-success

issue1.tar.gz

/cc @kripken

[kripken]

@tlively what ends up happening here is that we have this:

     (struct.get $type_21 3
      (call_indirect $timport$1 (type $type_20)

the table, as its name suggests, is imported. So we are calling a function of type 20 in the table, and it returns a struct of type 21. However, ModuleUtils::getPublicHeapTypes() does not consider this type public. GUFA depends on this to tell us what types are private - and therefore fully known - and this one seems private to it. So it (mis)optimizes.

Do we intentionally not define such a type as "public"? The definition is

// Collect all the heap types visible on the module boundary that cannot be
// changed. TODO: For open world use cases, this needs to include all subtypes
// of public types as well.
std::vector<HeapType> getPublicHeapTypes(Module& wasm);

In this case, the type is arguably "not on the boundary" and can be changed without breaking validation. I.e. it isn't clear to me if our goal in this function is avoiding validation errors, or runtime errors - public in the validation sense or in the execution sense.

This can be fixed by either

1. Make ModuleUtils::getPublicHeapTypes() find public tables, then scan the wasm again and find call_indirects on those tables, and anything sent to those calls or returning from them is public.
2. GUFA could look not just at ModuleUtils::getPublicHeapTypes() but also consider as public any type that is accessible from call_indirects.

1 would affect a lot of other passes. WDYT?

[tlively]

We should definitely have a consistent definition of public types across all passes. Is the table here a funcref table?

The most correct thing to do here would be to resolve that TODO and make all subtypes of public types also be public. In this case, if the table is a funcref table, that would mean all defined function types are public. That's terrible, but it's the only truly correct behavior in an open world.

Doing something like finding all the call_indirects with public tables might be an interesting compromise, but I worry that it's just kicking the can down the road. call_indirect on public tables is just a particular instance of the general problem that abstract types on the module boundary can be freely cast to defined types inside or outside the open-world module.

[kripken]

Yes, it is a funcref table. Oh, are you saying that we do scan call_indirect types as public, but it is just that we don't handle subtypes? I don't see such call_indirect scanning in the code at all, but you are more familiar with it, maybe I'm reading it wrong.

[kripken]

But I do agree we should also implement that TODO, it sounds necessary. Were you planning to get to it?

[tlively]

Oh, are you saying that we do scan call_indirect types as public, but it is just that we don't handle subtypes?

No, what we currently scan is the table type, so we see that func is public. But we don't then traverse its subtypes and mark them public, which is what the TODO is about.

I can take a look at making this change.

[kripken]

No, what we currently scan is the table type, so we see that func is public.

Oh, I see... I was looking for call_indirect handling, not table handling. The latter would be more precise but I guess it only works for call_indirect and not other table usage. We could consider this as an optimization later I guess.

I can take a look at making this change.

Thanks!