[Exceptions] Fix cross-module Tag handling in interpreter

[kripken]

Make ExnData store a Tag*, allowing proper identification.

Like FuncData etc., move that logic into the interpreter.

[tlively]

Don't we need to handle an arbitrary number of imported tags?

[kripken]

This is just for the special imported tags from JS:

binaryen/scripts/fuzz_shell.js

Lines 386 to 391 in 959d522

	var wasmTag = imports['fuzzing-support']['wasmtag'] = new WebAssembly.Tag({
	'parameters': ['i32']
	});
	// The JSTag that represents a JS tag.
	imports['fuzzing-support']['jstag'] = WebAssembly.JSTag;

[tlively]

This probably also needs an instance self pointer to differentiate tags defined by different instances of the same module.

[kripken]

Good point, I suppose it does. I'll add a TODO (atm we don't fuzz or test with that afaik)

[tlively]

testsuite/try_table.wast depends on this (so we skip it atm).

[kripken]

Oh, good. Then we can enable that later. (I just prefer to keep this PR small.)

[tlively]

Shouldn't this be a general getImportedTag? Host tags shouldn't be special for any reason and it would be best to be consistent in how we handle imports. Specifically, we should follow the same patterns for getImportedFunction and whatever we do for tags.

[tlively]

(And btw we should also follow the same shared pattern for tables and memories as well.)

[kripken]

We have these special host tags in the fuzzer, which we don't for functions - so this pattern is very convenient here, but maybe not for functions.

Perhaps this could be refactored to make these host tags come from a special internal module, I considered that but it was a lot more work, and I wasn't sure it would lead to benefits.

I do agree it makes sense to unify this, but I'm not sure which way, so I'd like to leave that for later.

[tlively]

We have lots of host functions in the fuzzer as well, but we treat them as normal imports by wrapping the custom functionality in the FuncData's std::function. Tags wouldn't need that last part, since only their identity matters.

[kripken]

Yeah, we handle tags differently by having an instance of them (which we can take the address of), unlike those functions. It's possible to handle it otherwise but it would be a lot more work I think.

[tlively]

I think concretely the changes I would want to see here is to rename this function to getImportedTag and to add a ModuleRunnerBase::getExportedTag function that calls externalInterface->getImportedTag(tag) on imported tags. That would replace the while loop in getCanonicalTag to track down the definition of imported tags.

[kripken]

Ok, I tried to do that in the last two commits, PTAL

1. I thought you meant to generate an internal module etc. - that would be a lot more work, obviously.
2. But I don't quite think I follow you, as the loop in getCanonicalTag is still necessary?

[tlively]

If getCanonicalTag calls out to the ExternalInterface to retrieve imported tags, then in multi-instance scenarios that ExternalInterface will look up the instance providing the import and call getExportedTag on it (which might in turn call externalInterface->getImportedTag again if it is a re-exported tag). So the loop is replaced with a recursion.

[tlively]

It would be good to get the newly-imported spec tests for this running as well. In general spec tests are better for testing multi-module execution because the several modules can go in a single file.

[kripken]

Nice, looks like that relevant spec test passes! I enabled it now.

Is it worth removing the new test from this PR? I think an exec test is still useful to have for this, as extra coverage. The fuzz-exec output is also more explicit than spec test output.

[tlively]

Nice! I'm fine keeping the lit test, too.

[tlively]

I guess the spec test doesn't tickle the case where we would need a self pointer to differentiate the tags, so it would be good to add such a spectest upstream.

(I can look at this since I need to add some new upstream tests anyway for the function import cast stuff.)