FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

FIX Prevent segfaults in CLI runner by hoodmane · Pull Request #4836 · pyodide/pyodide · GitHub

FIX Prevent segfaults in CLI runner - #4836

Merged
hoodmane merged 11 commits into
pyodide:mainfrom
hoodmane:cli-runner-fix-got
Jun 7, 2024
Merged

FIX Prevent segfaults in CLI runner#4836
hoodmane merged 11 commits into
pyodide:mainfrom
hoodmane:cli-runner-fix-got

Conversation

hoodmane commented Jun 4, 2024
edited
Loading

Copy link
Copy Markdown
Member

After many hours of debugging, I minimized the problem down to this: emscripten-core/emscripten#22052

Thanks to @henryiii for reporting. See thread here for my comments while I was debugging:
scikit-hep/boost-histogram#938

Resolves #2964.

After many hours of debugging, I minimized the problem down to this:
emscripten-core/emscripten#22052

Thanks to @henryiii for reporting. See thread here for my comments while I was
debugging:
scikit-hep/boost-histogram#938
hoodmane added this to the 0.26.1 milestone Jun 4, 2024

hoodmane commented Jun 4, 2024

Copy link
Copy Markdown
Member Author

Actually, we could just backport emscripten-core/emscripten#22053

ryanking13 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for analysing and fixing the issue @hoodmane!

Comment thread src/js/load-package.ts
// Warning: this sounds like it might not do anything important, but it
// fills in the GOT. There can be segfaults if we leave it out.
// See https://github.com/emscripten-core/emscripten/issues/22052
Module.reportUndefinedSymbols();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Huh, If I remember correctly, that function throws if it encounters a symbol that doesn't actually exist, but I may be misremembering something.

hoodmane Jun 5, 2024
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It fills in the Global Object Table:

    var reportUndefinedSymbols = () => {
      for (var [symName, entry] of Object.entries(GOT)) {
        if (entry.value == 0) {
          var value = resolveGlobalSymbol(symName, true).sym;
          if (!value && !entry.required) {
            // Ignore undefined symbols that are imported as weak.
            continue;
          }
          if (typeof value == "function") {
            /** @suppress {checkTypes} */
            entry.value = addFunction(value, value.sig);
          } else if (typeof value == "number") {
            entry.value = value;
          } else {
            throw new Error(
              `bad export type for '${symName}': ${typeof value}`,
            );
          }
        }
      }
    };

ryanking13 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks, let's see if your upstream PR will get merged.

hoodmane commented Jun 6, 2024

Copy link
Copy Markdown
Member Author

I think upstream PR is going to take a while and probably change, but I wouldn't be against using the patch in that PR here instead of this change.

Copy link
Copy Markdown
Member

I think upstream PR is going to take a while and probably change, but I wouldn't be against using the patch in that PR here instead of this change.

Sure, I have no objection with it.

hoodmane commented Jun 7, 2024

Copy link
Copy Markdown
Member Author

Except that it seems to break pyxel and pygame?? I'll have to investigate further.

hoodmane commented Jun 7, 2024
edited
Loading

Copy link
Copy Markdown
Member Author

The problem has to do with dynamic linking of JavaScript symbols.

hoodmane merged commit fa7dcc3 into pyodide:main Jun 7, 2024
hoodmane deleted the cli-runner-fix-got branch June 7, 2024 22:16
hoodmane added a commit to hoodmane/pyodide that referenced this pull request Jun 7, 2024
After many hours of debugging, I minimized the problem down to this: emscripten-core/emscripten#22052

Thanks to @henryiii for reporting. See thread here for my comments while I was debugging:
scikit-hep/boost-histogram#938

Resolves pyodide#2964.
JeanChristopheMorinPerso added a commit to JeanChristopheMorinPerso/otio-wasm that referenced this pull request Nov 24, 2024
JeanChristopheMorinPerso added a commit to JeanChristopheMorinPerso/otio-wasm that referenced this pull request Nov 24, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

TypeError: getWasmTableEntry(...) is not a function

2 participants


Back | FazBrowse Home | New Git URL