Add assertion checking if too many args are passed into exports.

[brendandahl]

Fixes #21348

[brendandahl]

I'm guessing this will pick up some failures in other places...

[sbc100]

Nice!

I wonder if we will see any tests fail because of this :)

There could be some cases where a developer might be relying on the VM to automatically inject zero values for missing arguments.. so it might be safer to only report too many arguments and not too few?

[brendandahl]

I wonder if we will see any tests fail because of this :)

There could be some cases where a developer might be relying on the VM to automatically inject zero values for missing arguments.. so it might be safer to only report too many arguments and not too few?

Looks like a number of tests fail where we rely on that behavior. I'll change it to just check for too many arguments.

[sbc100]

I wonder if we will see any tests fail because of this :)
There could be some cases where a developer might be relying on the VM to automatically inject zero values for missing arguments.. so it might be safer to only report too many arguments and not too few?

Looks like a number of tests fail where we rely on that behavior. I'll change it to just check for too many arguments.

I know ASYNCIFY relies on this.. was there anything else?

[brendandahl]

I've looked through a few and it seems the majority come from _emscripten_thread_init.

[brendandahl]

We could try to fix them to be "more correct", but code size could be worse and it would change the behavior of existings APIs. Any thoughts?

[sbc100]

I've looked through a few and it seems the majority come from _emscripten_thread_init.

Is that because the entry point for a pthread sometimes has zero args and sometimes as 1 argument? I think that is hard to work around sadly :( Luckily the zero arg form is technically undefined behaviour so we could just disallow that.

[sbc100]

We could try to fix them to be "more correct", but code size could be worse and it would change the behavior of existings APIs. Any thoughts?

I don't think its worth adding complexity but perhaps there are some cases that are actually bugs.

[brendandahl]

I've looked through a few and it seems the majority come from _emscripten_thread_init.

Is that because the entry point for a pthread sometimes has zero args and sometimes as 1 argument? I think that is hard to work around sadly :( Luckily the zero arg form is technically undefined behaviour so we could just disallow that.

It's mainly from here:

emscripten/src/worker.js

Line 232 in a95c44e

Module['__emscripten_thread_init'](e.data.pthread_ptr, /*is_main=*/0, /*is_runtime=*/0, /*can_block=*/1);

we don't pass in the last two arguments int default_stacksize, int start_profiling

[sbc100]

I've looked through a few and it seems the majority come from _emscripten_thread_init.

Is that because the entry point for a pthread sometimes has zero args and sometimes as 1 argument? I think that is hard to work around sadly :( Luckily the zero arg form is technically undefined behaviour so we could just disallow that.

It's mainly from here:

emscripten/src/worker.js

Line 232 in a95c44e

Module['__emscripten_thread_init'](e.data.pthread_ptr, /*is_main=*/0, /*is_runtime=*/0, /*can_block=*/1);

we don't pass in the last two arguments int default_stacksize, int start_profiling

That one might be easy enough to fix..

[sbc100]

I don't see how this is any different to bool(pre_arg) which itself is identical to pre_arg in an if statement, no?

[brendandahl]

There's an assignment above that does call_args = pre_arg, then we append to call_args which modifies pre_arg as well.

[brendandahl]

Alternatively, we could instead clone/copy pre_arg to call_args so modifying one does not modify the other.

[sbc100]

Ah, in that case I think its better to do call_args = list(pre_arg) or call_args = copy(pre_arg) above.

[sbc100]

oops call_args = pre_arg.copy()

[sbc100]

LGTM.

Maybe update the PR title/descrption to mention that we only check for too-many and not too-few and why (because there can be valid use case for passing too few). Maybe add a comment before the assertion to that effect too?