Skip to content

Commit

Permalink
FIX Prevent segfaults in CLI runner (pyodide#4836)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
hoodmane committed Jun 7, 2024
1 parent d9f9679 commit af8cf23
Show file tree
Hide file tree
Showing 9 changed files with 74 additions and 9 deletions.
4 changes: 4 additions & 0 deletions docs/project/changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ myst:
"Emscripten" and not as "emscripten".
{pr}`4812`

- {{ Fix }} Resolution of JavaScript symbols in dynamic libraries doesn't fail
anymore.
{pr}`4836`

### Packages

- New Packages: `pytest-asyncio` {pr}`4819`
Expand Down
11 changes: 11 additions & 0 deletions packages/cpp-exceptions-test2/meta.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package:
name: cpp-exceptions-test2
version: "1.0"
tag:
- core
- pyodide.test
source:
path: src
build:
cxxflags: -fexceptions
ldflags: -fexceptions
13 changes: 13 additions & 0 deletions packages/cpp-exceptions-test2/src/cpp_exceptions_test2.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#include "Python.h"
#include <stdexcept>

extern "C" __attribute__((visibility("default"))) PyObject*
PyInit_cpp_exceptions_test2()
{
try {
throw std::runtime_error("something bad?");
} catch (const std::exception& e) {
PyErr_SetString(PyExc_ImportError, "oops");
}
return nullptr;
}
Empty file.
16 changes: 16 additions & 0 deletions packages/cpp-exceptions-test2/src/setup.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
from setuptools import Extension, setup

setup(
name="cpp-exceptions-test2",
version="1.0",
ext_modules=[
Extension(
name="cpp_exceptions_test2", # as it would be imported
# may include packages/namespaces separated by `.`
language="c++",
sources=[
"cpp_exceptions_test2.cpp"
], # all sources are compiled into a single binary file
),
],
)
8 changes: 4 additions & 4 deletions src/core/error_handling.c
Original file line number Diff line number Diff line change
Expand Up @@ -119,13 +119,13 @@ EM_JS(JsVal, restore_stderr, (void), {
/**
* Wrap the exception in a JavaScript PythonError object.
*
* The return value of this function is always a valid hiwire ID to an error
* object. It never returns NULL.
* The return value of this function is always a JavaScript error object. It
* never returns null.
*
* We are cautious about leaking the Python stack frame, so we don't increment
* the reference count on the exception object, we just store a pointer to it.
* Later we can check if this pointer is equal to sys.last_exc and if so
* restore the exception (see restore_sys_last_exception).
* Later we can check if this pointer is equal to sys.last_exc and if so restore
* the exception (see restore_sys_last_exception).
*
* WARNING: dereferencing the error pointer stored on the PythonError is a
* use-after-free bug.
Expand Down
4 changes: 4 additions & 0 deletions src/js/load-package.ts
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,10 @@ export async function loadPackage(
Array.from(toLoad.values()).map(({ installPromise }) => installPromise),
);

// 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
// TODO: Fix Emscripten so this isn't needed
Module.reportUndefinedSymbols();
if (loadedPackageData.size > 0) {
const successNames = Array.from(loadedPackageData, (pkg) => pkg.name)
Expand Down
11 changes: 6 additions & 5 deletions src/templates/python
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,11 @@ async function main() {
console.error(e);
}
}
// 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
// TODO: Fix Emscripten so this isn't needed
py._module.reportUndefinedSymbols();

py.runPython(
`
Expand Down Expand Up @@ -176,14 +181,10 @@ async function main() {
try {
errcode = py._module._run_main();
} catch (e) {
// If someone called exit, just exit with the right return code.
if(e.constructor.name === "ExitStatus"){
process.exit(e.status);
}
// Otherwise if there is some sort of error, include the Python
// tracebook in addition to the JavaScript traceback
py._module._dump_traceback();
throw e;
py._api.fatal_error(e);
}
if (errcode) {
process.exit(errcode);
Expand Down
16 changes: 16 additions & 0 deletions src/tests/test_cmdline_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -502,3 +502,19 @@ def test_sys_exit(selenium, venv):
assert result.returncode == 12
assert result.stdout == ""
assert result.stderr == ""


def test_cpp_exceptions(selenium, venv):
result = install_pkg(venv, "cpp-exceptions-test2")
print(result.stdout)
print(result.stderr)
assert result.returncode == 0
result = subprocess.run(
[venv / "bin/python", "-c", "import cpp_exceptions_test2"],
capture_output=True,
encoding="utf-8",
)
print(result.stdout)
print(result.stderr)
assert result.returncode == 1
assert "ImportError: oops" in result.stderr

0 comments on commit af8cf23

Please sign in to comment.