Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to emscripten 1.38.34 with fastcomp #480

Merged
merged 27 commits into from Nov 30, 2020

Conversation

rth
Copy link
Member

@rth rth commented Jun 27, 2019

This aims to upgrade the build system to use emscripten 1.36.36 (still with fastcomp), which should help with moving to the upstream llvm backend in #476 (using the same emscrtipten version).

One challenge is that previously we used emscripten, binaryen, patched clang of the same emsdk version, built from source to which additional patches were applied.

Currently, the latest available tags in emsdk are,

  • 1.38.36 [default (fastcomp) backend]
  • 1.38.36-upstream [upstream LLVM wasm backend]
  • sdk-tag-1.38.31-64bit
  • clang-e1.38.31-64bit
  • binaryen-tag-1.38.31-64bit
  • emscripten-tag-1.38.31-64bit

when we use the 1.38.36 tag, we do get that version of emscripten but then it's unclear whether previous versions of binaryen should be used (or if there is another way of setting NUM_PARAMS for FuncCastEmulation).

Here, I tried the 1.38.36 tag (emscripten) + binaryen-tag-1.38.31-64bit and I tried removing the clang-e1.38.31-64bit tag (since anyway we wouldn't be able to patch it with upstream llvm). This builds (locally) but fails to load for tests.

Will try re-adding clang tag, next.

cc @mdboom

@rth
Copy link
Member Author

rth commented Jul 8, 2019

@mdboom Any thoughts on this, or on how to proceed with the migration in #476 ?

@mdboom
Copy link
Collaborator

mdboom commented Jul 8, 2019

No updates -- I had meant to look at this last week, but got derailed on other things...

@mdboom
Copy link
Collaborator

mdboom commented Jul 11, 2019

I'm getting a little farther by moving this to emscripten 1.38.38... Will report more later.

@mdboom
Copy link
Collaborator

mdboom commented Jul 11, 2019

Getting an error about needing -fPIC, so recompiling with that...

@rth
Copy link
Member Author

rth commented Oct 10, 2019

Closing in favor of #531

@rth rth reopened this May 27, 2020
@rth rth changed the title WIP: Upgrade to emscripten 1.38.36 with fastcomp WIP: Upgrade to emscripten 1.38.35 with fastcomp May 27, 2020
@rth rth marked this pull request as draft May 27, 2020 11:23
@rth rth force-pushed the update-1.38.36-fastcomp branch from 08a39cb to 71365a6 Compare May 27, 2020 11:24
@rth
Copy link
Member Author

rth commented May 27, 2020

This is another attempt to update emscripten version by small increments.

Current situation is that building CPython works (at least locally for me), but loading pyodide fails in tests which needs more investigation. Once CPython is working, it might be necessary to manually install binaryen as we did before to re-apply the emsdk/patches/num_params.patch. I really hope it can work without emsdk/patches/callHandlers.patch as otherwise it means that we need to build fastcomp-clang from sources as we did before which takes a while.

@SimonBiggs
Copy link
Contributor

I believe the callHandlers.patch was merged into 1.38.31. See emscripten-core/emscripten-fastcomp#256

@rth rth mentioned this pull request May 28, 2020
@rth rth changed the title WIP: Upgrade to emscripten 1.38.35 with fastcomp Upgrade to emscripten 1.38.33 with fastcomp May 28, 2020
@rth
Copy link
Member Author

rth commented May 28, 2020

This should be a bit easier now that update to 1.38.31 was made in #674

@rth rth changed the title Upgrade to emscripten 1.38.33 with fastcomp Upgrade to emscripten 1.38.34 with fastcomp May 28, 2020
@rth rth marked this pull request as ready for review May 28, 2020 20:32
@rth rth mentioned this pull request Jul 8, 2020
@dalcde
Copy link
Contributor

dalcde commented Nov 29, 2020

The error doesn't seem to be a numpy error. It happens with all C libraries. For example, the following also fail:

import regex
from lxml import etree

@rth
Copy link
Member Author

rth commented Nov 29, 2020

The error doesn't seem to be a numpy error. It happens with all C libraries.

Ahh, thank you that's very helpful! In particular, regex should be much easier to debug as it has only one C extension (lxml less so).

@dalcde
Copy link
Contributor

dalcde commented Nov 29, 2020 via email

@rth
Copy link
Member Author

rth commented Nov 29, 2020

FYI, lxml import occasionally fails on master as well, so that would not be a reliable package to use for debugging here. I'll need to re-build this PR to investigate.

@dalcde
Copy link
Contributor

dalcde commented Nov 29, 2020

I inserted the following snippet into src/main.c:

  void *handle = dlopen("/lib/python3.8/site-packages/regex/_regex.so", RTLD_LAZY);
  if (handle == NULL) {
    printf("Cannot dlopen\n");
  } else {
    printf("Can dlopen\n");
  }

  void *sym = dlsym(handle, "PyInit__regex");
  if (sym == NULL) {
    char* err = dlerror();
    if (err != NULL) {
      printf("%s\n", err);
      printf("Cannot dlsym\n");
    } else {
      printf("Loaded symbol is NULL\n");
    }
  } else {
    printf("Can dlsym\n");
  }

and forced the js bits to load regex.js early on. What I get is the following:

Can dlopen
Loaded symbol is NULL
Python initialization complete

In other words, both dlopen and dlsym are successful, but the symbol returned is the null pointer. I'm not very well-versed in wasm itself, but it seems to me that in _regex.so, PyInit__regex is not the null pointer, so the mystery continues. (EDIT: changing RTLD_LAZY to RTLD_NOW does not help)

EDIT 2: On the javascript side, the symbol _PyInit__regex is in the list of exports.
EDIT 3: The problem is on the dlsym side. Copying regex.{js,data} compiled with 1.38.34 to pyodide compiled with 1.38.31 works, but copying regex.{js,data} compiled with 1.38.31 to pyodide compiled with 1.38.34 does not. Attempting to load other symbols in the snippet above also return NULL.
EDIT4: dlsym works in a simple test case using our patched emscripten.

Now C libraries can be correctly imported

The relevant code was removed in upstream at
emscripten-core/emscripten@16d0187

Thus, I have no intentions to upstream this fix.
@dalcde
Copy link
Contributor

dalcde commented Nov 30, 2020

The problem is identified and fixed in rth#1 . Feel free to merge, cherry pick, and/or squash it into your PR.

@rth
Copy link
Member Author

rth commented Nov 30, 2020

Great work @dalcde, thank you!

This reverts commit 4dc3fcb.

scipy fails to build otherwise.
@rth rth mentioned this pull request Nov 30, 2020
@rth rth marked this pull request as ready for review November 30, 2020 17:52
@rth rth merged commit f7adad7 into pyodide:master Nov 30, 2020
@rth rth deleted the update-1.38.36-fastcomp branch November 30, 2020 17:54
@rth
Copy link
Member Author

rth commented Nov 30, 2020

Thanks so much @dalcde ! The rest of the CI was fine. I'm truly happy about this being merged. That typo in emscripten was really a blocker. Now we can try to incrementally update to the latest emscripten version #476 (comment)

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.

None yet

4 participants