Skip to content

Switch to separate setting for loading shared libraries globally#3054

Closed
hoodmane wants to merge 7 commits intopyodide:mainfrom
hoodmane:sharedlib-no-global
Closed

Switch to separate setting for loading shared libraries globally#3054
hoodmane wants to merge 7 commits intopyodide:mainfrom
hoodmane:sharedlib-no-global

Conversation

@hoodmane
Copy link
Copy Markdown
Member

@hoodmane hoodmane commented Aug 31, 2022

We have 16 shared libraries (7 for testing purposes, 9 "real" ones), but most of them do not need to be loaded globally, only 3 of them need it and two of the three are tests. This PR adds a new global setting to handle this. Hopefully we can gradually reduce how many libraries need to be loaded globally since global loading is a maintenance hazard.

Checklists

  • Add a CHANGELOG entry
  • Add new / update outdated documentation

@hoodmane hoodmane marked this pull request as draft August 31, 2022 20:39
@rth
Copy link
Copy Markdown
Member

rth commented Sep 1, 2022

How do you determine if a .so needs to be loaded globally?

Also global is a very generic name. Maybe global_dll or global_so would be more specific.

@hoodmane
Copy link
Copy Markdown
Member Author

hoodmane commented Sep 1, 2022

Yeah global_dll or global_so both sound better.

@hoodmane
Copy link
Copy Markdown
Member Author

hoodmane commented Sep 1, 2022

How do you determine if a .so needs to be loaded globally?

Well it happens when b.so requires symbols from a.so but isn't linked against a.so. We can find when this happens by trial and error. It's also worth noting that making a library global is synchronous and can be done lazily (after the library is loaded, before any of the missing symbols are used). So in principal the incorrectly linked b.so could declare that it needs a.so to be global in it's meta.yaml file instead of a.so having to know that b.so exists and declare itself as global.

Ideally we would also fix everything to link properly and not need anything to be global. This would be particularly helpful for the command line runner.

@ryanking13
Copy link
Copy Markdown
Member

Thanks for the work!

Ideally we would also fix everything to link properly and not need anything to be global. This would be particularly helpful for the command line runner.

Yes that would be ideal. It looks like the most problematic library is CLAPACK for now.

@hoodmane
Copy link
Copy Markdown
Member Author

Yeah I'm a bit confused about this PR, since I don't reproduce a lot of the CI failures locally. If you feel inclined @ryanking13 you could take it over.

@hoodmane
Copy link
Copy Markdown
Member Author

I think this is more or less a prerequisite for #3098 to work well.

@ryanking13
Copy link
Copy Markdown
Member

ryanking13 commented Sep 15, 2022

Yeah I'm a bit confused about this PR, since I don't reproduce a lot of the CI failures locally. If you feel inclined @ryanking13 you could take it over.

I'll see if I can fix this. I also tried to get rid of global shared lib loading a few months ago but failed back then.
It is interesting that sqlite3 needs to be specified as a global_dso: true. We use a static version of libsqlite3 so something is clearly wrong here...

@ryanking13
Copy link
Copy Markdown
Member

I have looked into this for a few days, and I am not sure loading a dependent shared library locally (and exposing its symbols to a specific module) is possible without a large modification in emscripten codebase.

Emscripten resolves symbol from:

  1. Global symbols
  2. Symbols exported from module itself

which means if a module (.so) needs to look for an external symbol, it has to be loaded globally.

@hoodmane
Copy link
Copy Markdown
Member Author

if a module (.so) needs to look for an external symbol, it has to be loaded globally.

I don't believe this is correct. See for instance the comment just above that:

We need this fallback because: ... Symbols from side modules are not always added to the global namespace.

I think it is possible for one module to use a symbol from another even if the other module isn't in global scope. But the exact behavior is pretty confusing.

@hoodmane hoodmane closed this Mar 1, 2023
@hoodmane hoodmane deleted the sharedlib-no-global branch March 1, 2023 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants