Enable EXPORT_ALL in main modules with the wasm backend #10079

kripken · 2019-12-19T23:07:02Z

Fastcomp adds all the JS library to the asmLibraryArg object which is
provided as "env" to wasm modules, which lets them access those methods.
In the wasm backend we use a more straightforward method of flipping
the EXPORT_ALL flag, which provides those methods on Module, which is
used to look up imports as necessary.

EXPORT_ALL is also noticed by metadce (when present, it doesn't try to
remove wasm exports) so this likely fixes another bug as well, see the
metadce output changes (the additions were metadce'd out before,
possibly incorrectly if a side module needs them).

Fixes #9793

sbc100 · 2019-12-19T23:24:01Z

I'm confused though because you explicitly disabled EXPORT_ALL for MAIN_MODULE in #7371

kripken · 2019-12-19T23:34:22Z

Oh good point, I forgot that... well, I guess we need a very different approach here then...

sbc100 · 2019-12-20T02:43:16Z

This is all very confusing. I would have thought that EXPORT_ALL should be enabled by default with MAIN_MODULE=1 and disabled with MAIN_MODULE=2. Does that make sense?

So I think that #7371 maybe should have only effected the =1 case, and not the =2 case?

kripken · 2019-12-20T03:29:11Z

Yeah, the behavior of =2 might need to be different here.

The Module object is used to interface between main and side modules, is perhaps the tricky thing here. So exporting onto Module, which is what EXPORT_ALL does, can assist dynamic linking. But fastcomp does this differently, it just adds them all on the env argument for imports to wasm, which is more direct, and more compact. Perhaps we should just do that for upstream too, and maybe that should only be done in =1. In =2 the user would need to specify which JS library things to export, using EXPORTED_RUNTIME_METHODS. So this would generalize the 1/2 behavior of "do it all vs let the user specify which" from exporting compiled code to JS library code. What do you think?

sbc100 · 2019-12-20T20:10:32Z

I agree that approach.

I'm still a little confused about the downside of EXPORT_ALL in the case of MAIN_MODULE=1 because I don't understand what you mean by "fastcomp does this differently, it just adds them all on the env argument for imports to wasm, which is more direct". Is this a potential missing optimization for the llvm backend path? Or does it not make sense there?

It sounds like t here are two differnt levels of export here and that EXPORT_ALL is controlling both of them?

sbc100 · 2019-12-20T20:13:28Z

emcc.py

+      # the EXPORT_ALL flag, which provides those methods on Module, which is
+      # used to look up imports as necessary.
+      if shared.Settings.WASM_BACKEND:
+        shared.Settings.EXPORT_ALL = 1


I think we need to do this both for SIDE_MODULE=1 and MAIN_MODULE=1, down below on ling 1170 would make sense to me.

This still needs doing I think.

sbc100 · 2019-12-21T03:44:30Z

emcc.py

    elif shared.Settings.SIDE_MODULE:
      assert not shared.Settings.MAIN_MODULE
      options.memory_init_file = False # memory init file is not supported with asm.js side modules, must be executable synchronously (for dlopen)
+      if not shared.Settings.WASM_BACKEND:
+        shared.Settings.EXPORT_ALL = 1


I think you can just do this once down on line 1166, no? Right below where we set LINKABLE = 1?

Still needs doing.

Thanks, done.

kripken · 2020-01-06T23:56:33Z

Oh, I was wrong in part of the analysis before. I saw that fastcomp adds all the JS library methods to asmLibraryArg, and that upstream does not, and I thought that explained the difference. So a possible solution was to just do what fastcomp does, as mentioned earlier. However, that doesn't really help - it's still increasing code size, but worse, it also means the dynamic loader must look for methods on asmLibraryArg too (when resolveSymbol is called for things like function pointers). So I think it's better to just use EXPORT_ALL for this.

But, as agreed earlier, let's only do it in MAIN_MODULE=1, so that in mode 2 users can control what is exported manually.

Also stop doing EXPORT_ALL in the test runner: do it in emcc.py when needed

sbc100 · 2020-01-08T00:21:04Z

src/support.js

+#if ASSERTIONS
+              // this resolved symbol must exist, as we are about to try to
+              // add it to the table; error clearly here instead of there.
+              assert(f, 'could not resolve: ' + name);


resolveSymbol already includes an assert which more detail than this.

Thanks, I forgot to build with assertions when I tested this I think...

sbc100 · 2020-01-08T00:21:49Z

emcc.py

    elif shared.Settings.SIDE_MODULE:
      assert not shared.Settings.MAIN_MODULE
      options.memory_init_file = False # memory init file is not supported with asm.js side modules, must be executable synchronously (for dlopen)
+      if not shared.Settings.WASM_BACKEND:
+        shared.Settings.EXPORT_ALL = 1


Still needs doing.

sbc100 · 2020-01-08T20:30:24Z

emcc.py

        shared.Settings.LINKABLE = 1
+        shared.Settings.EXPORT_ALL = 1


If WASM_BACKEND?

I think there is no harm to do it in fastcomp too, and it makes the two paths more similar. Also, I'm not sure that there isn't a fastcomp bug that isn't fixed by this, but I didn't investigate. But maybe I'm forgetting something?

) In mode 1 we should provide the entire JS library to other modules, if they want to import something from there. This was always needed, but the test suite did it manually. With this PR we can remove it from the test suite as we do it automatically, which is safer for users. Note that there is no change to mode 2, in which we do DCE as normal and the user must specify what to export to other modules. In that case the user must export JS library methods as required.

kripken added 3 commits December 19, 2019 10:20

EXPORT_ALL for MAIN_MODULES in the wasm backend. fixes #9793

24c7157

Merge remote-tracking branch 'origin/incoming' into exall

7925874

Fixes. also fixes metadce here, turns out

7132b9f

kripken requested a review from sbc100 December 19, 2019 23:07

sbc100 mentioned this pull request Dec 20, 2019

Warn if EXPORTED_FUNCTIONS is used with MAIN_MODULE=1/SIDE_MODULE=1 #10075

Merged

sbc100 reviewed Dec 20, 2019

View reviewed changes

kripken added 7 commits December 20, 2019 15:04

Merge remote-tracking branch 'origin/incoming' into exall

c7ee0b2

wip yolo

f4ed6e3

fastcomp side modules need EXPORT_ALL

94c4bc4

update tests

618de01

update tests

10500e5

comment fix

97b0f5a

update tests

7e398ff

sbc100 reviewed Dec 21, 2019

View reviewed changes

kripken added 6 commits January 6, 2020 15:04

Merge remote-tracking branch 'origin/incoming' into exall

2e18add

try

637e25e

more

460f247

wip

6b5d0ea

fix

8ea0c49

better

cbd5965

update tests

a09b98c

sbc100 reviewed Jan 8, 2020

View reviewed changes

kripken added 2 commits January 8, 2020 12:16

feedback

a5bc081

Merge remote-tracking branch 'origin/incoming' into exall

2f91aeb

fix

7f0c7a5

sbc100 reviewed Jan 8, 2020

View reviewed changes

sbc100 approved these changes Jan 8, 2020

View reviewed changes

kripken merged commit 4e0ed34 into incoming Jan 8, 2020

kripken deleted the exall branch January 8, 2020 22:05

sbc100 mentioned this pull request Jun 16, 2021

Improve dynamic linking browser tests. NFC #14448

Merged

Enable EXPORT_ALL in main modules with the wasm backend #10079

Enable EXPORT_ALL in main modules with the wasm backend #10079

Uh oh!

Conversation

kripken commented Dec 19, 2019

Uh oh!

sbc100 commented Dec 19, 2019

Uh oh!

kripken commented Dec 19, 2019

Uh oh!

sbc100 commented Dec 20, 2019

Uh oh!

kripken commented Dec 20, 2019

Uh oh!

sbc100 commented Dec 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Jan 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!