Remove createDyncallWrapper and its usage

[sbc100]

This change effectively reverts #17328.

IIUC the dynCall_xxx helpers for a given side module should always be exported by that side module. i.e. for all possible indirect calls a dynCall_xxx wrapper should already exist either in the main module or the side module.

[sbc100]

@hoodmane, can you explain your use case a little more and why #17328 was needed in the first place? How were you able to create a side module without the corresponding dynCall_xxx exports? Is it possible you didn't use emcc to link the side module perhaps and therefore the binaryen pass for creating these exports was never run? If so, then I think the solution is to use binaryen to create them at build time, rather than this method of adding them at runtime.

[sbc100]

Its also possible that you were using RTLD_LOCAL, in which case you use case should have been fixed by #22625.

[sbc100]

@hoodmane, could you take a look? I don't want to remove this feature you added if its going to break your use case.

[hoodmane]

I'll try to look into it later this week, but if it causes a problem we can always just revert it downstream. Emscripten is quite easy to patch thankfully.

As far as I understand my original messages, the problem only comes up from Rust shared libraries when -sWASM_BIGINT isn't passed? Pyodide always uses -sWASM_BIGINT these days, so I don't think it will be negatively affected. And hopefully now that we've merged the -lc fix to rust-lang/libc we'll be able to switch Rust itself to link with -sWASM_BIGINT.

[sbc100]

I'll try to look into it later this week, but if it causes a problem we can always just revert it downstream. Emscripten is quite easy to patch thankfully.

As far as I understand my original messages, the problem only comes up from Rust shared libraries when -sWASM_BIGINT isn't passed? Pyodide always uses -sWASM_BIGINT these days, so I don't think it will be negatively affected. And hopefully now that we've merged the -lc fix to rust-lang/libc we'll be able to switch Rust itself to link with -sWASM_BIGINT.

Right, if that is the case I would hope we wouldn't have to support that use case upstream. i.e. linking side modules built with -sWASM_BIGINT and main modules built without (or vice versa). Its seems reasonable that flags such as -sWASM_BIGINT should agree across the whole program.

[hoodmane]

flags such as -sWASM_BIGINT should agree across the whole program.

To be clear, I was never attempting to use .so files with a different setting than the main module. Using RUSTFLAGS=-Clink-arg=-sWASM_BIGINT -Clink-arg=-sSIDE_MODULE I make sure that Rust links a shared library with -sWASM_BIGINT. The original problem was coming up when both main module and (Rust) side module were not using -sWASM_BIGINT.

[sbc100]

flags such as -sWASM_BIGINT should agree across the whole program.

To be clear, I was never attempting to use .so files with a different setting than the main module. Using RUSTFLAGS=-Clink-arg=-sWASM_BIGINT -Clink-arg=-sSIDE_MODULE I make sure that Rust links a shared library with -sWASM_BIGINT. The original problem was coming up when both main module and (Rust) side module were not using -sWASM_BIGINT.

Ah, I think that should no longer be an issue after #22625. The side module should always export any dynCall helpers that it could need (same with the main module).

[sbc100]

flags such as -sWASM_BIGINT should agree across the whole program.

To be clear, I was never attempting to use .so files with a different setting than the main module. Using RUSTFLAGS=-Clink-arg=-sWASM_BIGINT -Clink-arg=-sSIDE_MODULE I make sure that Rust links a shared library with -sWASM_BIGINT. The original problem was coming up when both main module and (Rust) side module were not using -sWASM_BIGINT.

Ah, I think that should no longer be an issue after #22625. The side module should always export any dynCall helpers that it could need (same with the main module).

To confirm do you use RTDL_LOCAL when loading these side module (#22625 was specifically for that mode).

[kripken]

lgtm if the current discussion does not find a remaining use for this.

[sbc100]

I'll wait to hear back from @hoodmane if RTDL_LOCAL was being used.

[hoodmane]

Yeah we're using RTLD_LOCAL. So I think there are several reasons we don't need this.

[hoodmane]

Thanks for checking @sbc100!

[sbc100]

Yeah we're using RTLD_LOCAL. So I think there are several reasons we don't need this.

Awesome. In that case I feel pretty confident landing this change since I believe #22625 handled that case.