Fix type updating for indirect call effects

[stevenfontanella]

Part of #8615. Fixes #8833 which was wrong. The earlier fix never touched newTypeEffects at all. It passed tests and fixed the breakage only incidentally by being maximally conservative and losing all indirect call effects in the case of a type update. Fix this logic and add unit tests to vet this code better.

Drive-by fixes:

• Clear indirectCallEffects during --discard-global-effects, before recomputing global effects, and if a pass's addsEffects() is true. Clearing it when recomputing global effects is necessary to remove stale entries for types that no longer exist after type rewriting (although it should make no difference to optimizations).
• Fix use-after-free in tests due to static storage of HeapTypes / Types + destroyAllTypesForTestingPurposesOnly. The static HeapType / Type in getMutI8Array and similar functions referred to an index in globalHeapTypeStore / globalTupleStore, which gets cleared in unit tests with destroyAllTypesForTestingPurposesOnly(), causing the index to refer to garbage. Change these to not use static storage so that they're always in sync with globalHeapTypeStore / globalTupleStore.

[kripken]

Reading

  std::unordered_map<HeapType, Index> typeIndices;

in wasm.h, I realize I have no idea what it means. We can't be storing the binary format type indices, can we..?

Can we add a comment to wasm.h to clarify what this is?

[kripken]

(that this field lacks a comment predates this PR, but if we are going to rely on it here, we might as well fix that here)

[stevenfontanella]

AFAICT it is the binary type index:

binaryen/src/wasm/wasm-binary.cpp

Lines 2888 to 2891 in 7ec528c

	// Record the type indices.
	for (Index i = 0; i < types.size(); ++i) {
	wasm.typeIndices.insert({types[i], i});
	}

.

Added a comment.

[kripken]

Hmm, but this can't be right for us here, unless I'm missing something? Yes, looks like these are the type indices read from the input, but that was at that time - optimizations can add types and remove them. Like typeNames above it, this is only imprecise metadata.

[stevenfontanella]

Good point, looks like we don't have anything keeping track of all of the module's types. And I see that collectHeapTypes needs to traverse the whole module to collect all of the types, so I think we can avoid this and instead only use the types from oldToNewTypes and indirectCallEffects when updating things in type-updating. Nothing that's not in this two maps would need to be updated. Will send an update.

[kripken]

So a missing key is "unknown" and a key with null is "explicitly unknown" - ? But what is the actual difference between the two? Does the latter imply we can never know it, or something like that..?

[kripken]

Maybe give an example of how this matters in practice, like a situation where we can optimize if effects are unknown, but cannot if they are explicitly unknown (or vice versa)

[stevenfontanella]

The distinction is only there for type updating, and doesn't make a difference in effects.h (you can see in the diff that the cases that read indirectCallEffects treat a not-present key and an entry containing null the same way). The example I mention in the comment is the only reason that it matters. I rephrased the comment slightly to point out that it doesn't make a difference when looking up effects, does that help?

This is another case where it'd be useful to have a top value for effects that represents 'all effects' but it doesn't look like a trivial change since we keep track of sets of globals etc.

[kripken]

Thanks, now I see.

In the example, who knows that B is a new type? That party could copy A's types, avoiding this complexity, I am hoping. E.g., when creating a new type in TypeSSA, we could do that.

[stevenfontanella]

I see your point, but I'm not sure which passes this applies to. It looks like TypeSSA only adds new types for structs and arrays (link), so I think it has no impact on indirectCallEffects (we never add a new type for something that could be the target of an indirect call).

In other cases, maybe a type is created or renamed, but I was thinking that those cases should go through TypeUpdating which is where I made my changes. Per our chat conversation, it sounded like TypeMerging might be the only case?

About avoiding the complexity, we could also avoid it by attaching effects directly to the expression: https://github.com/WebAssembly/binaryen/compare/fix-indirect-call-effects-2. I think I'd still be in favor of that method rather than this PR.

[kripken]

Yes, I guess TypeSSA doesn't do function types yet (but it might someday).

Yes, maybe handling this in TypeUpdating and TypeMerging is enough. Then can we do this there, rather than support null shared_ptr here? (Adding complexity in wasm.h means complexity in the entire mental model one needs to keep in mind when working on the IR, everywhere - I'm trying to keep it as simple as possible.)