Skip to content

BUG: numpy segfaults on import with current CPython main #23766

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.

Sign up for GitHub

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

Jump to bottom
Closed
tacaswell opened this issue May 15, 2023 · 10 comments
Closed

BUG: numpy segfaults on import with current CPython main #23766

tacaswell opened this issue May 15, 2023 · 10 comments
Labels
00 - Bug

Comments

@tacaswell
Copy link
Contributor

Describe the issue:

numpy segfaults on import with current CPython main.

Reproduce the code example:

import numpy as np

Error message:

$ python
Python 3.12.0a7+ (heads/main:46f1c78eeb, May 13 2023, 16:09:15) [GCC 13.1.1 20230429] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import numpy as np

Segmentation fault (core dumped)

Runtime information:

Due to segfault on import can not get this 🤣

I can reproduce this with the ~current main branch of numpy + current cython master branch or with the lastest stable cython (0.29.31) + numpy (1.24.x) from pypi.

discovered via https://github.com/tacaswell/build_the_world

Context for the issue:

I bisected this back to

$ git bisect bad
de64e7561680fdc5358001e9488091e75d4174a3 is the first bad commit
commit de64e7561680fdc5358001e9488091e75d4174a3
Author: Eric Snow <ericsnowcurrently@gmail.com>
Date:   Tue May 2 21:30:03 2023 -0600

    gh-94673: More Per-Interpreter Fields for Builtin Static Types (gh-103912)

    his involves moving tp_dict, tp_bases, and tp_mro to PyInterpreterState, in the same way we did for tp_subclasses.  Those three fields are effectively const for builtin static types (unlike tp_subclasses).  In theory we only need to make their values immortal, along with their contents.  However, that isn't such a simple proposition.  (See gh-103823.)  In the meantime the simplest solution is to move the fields into the interpreter.

    One alternative is to statically allocate the values, but that's its own can of worms.

 Include/internal/pycore_typeobject.h |   7 ++
 Modules/_abc.c                       |   1 +
 Modules/gcmodule.c                   |  31 +----
 Objects/structseq.c                  |   9 +-
 Objects/typeobject.c                 | 223 +++++++++++++++++++++++++++--------
 5 files changed, 185 insertions(+), 86 deletions(-)

which came in via python/cpython#103912
@rgommers
Copy link
Member

Cc @lysnikolaou and @mattip - this is interesting timing, after our discussion a few days ago on NumPy needing updates for CPython changes related to immortal objects. @lysnikolaou maybe this is a good place to start digging in?

@charris
Copy link
Member

charris commented May 15, 2023

Hmm, if it is new in Python 3.12 we don't need to fix 1.24.x. Which is good :)

@mattip
Copy link
Member

mattip commented May 15, 2023

I am seeing a failed assert in released cython 0.29.3x here but that has been fixed on cython master to avoid that path for python>=3.12a7.

On cython master, compilation fails. In __Pyx_PyThreadState_Current the tstate fields used no longer exist in the struct on CPython 3.12.0a7.

I am curious how you got a build to work with cython master.

@mattip mattip mentioned this issue May 15, 2023
Closed
@tacaswell
Copy link
Contributor Author

I have -DCYTHON_FAST_THREAD_STATE=0 in my CFLAGS

    CFLAGS = (" -Wall -O2 -pipe -fomit-frame-pointer  "
              "-fno-strict-aliasing -Wmaybe-uninitialized  "
              "-Wdeprecated-declarations -Wimplicit-function-declaration  "
              "-march=native -DCYTHON_FAST_THREAD_STATE=0")

is the full set of cflags I am building with. I'm also going thourgh distutils (changing to meson is on my todo list).

@mattip
Copy link
Member

mattip commented May 15, 2023

Thanks

@scoder
Copy link
Contributor

scoder commented May 16, 2023

I am seeing a failed assert in released cython 0.29.3x here but that has been fixed on cython master to avoid that path for python>=3.12a7.

I just copied that change to Cython 0.29.x in cython/cython@8af954f. It will be included in 0.29.35.

@lysnikolaou lysnikolaou mentioned this issue May 18, 2023
Closed
@lysnikolaou
Copy link
Member

I looked into this after building with CPython and Cython main. This looks like a bug in CPython, but I'm not sure about it. I've opened an issue on python/cpython#104614 with more info about the code path that fails, in order to discuss what the correct way forward is.

@rgommers rgommers mentioned this issue May 29, 2023
Closed
@lysnikolaou lysnikolaou mentioned this issue May 31, 2023
Merged
@lysnikolaou
Copy link
Member

lysnikolaou commented May 31, 2023 •
edited
Loading

Brief status update on this: The issue will be fixed upstream in CPython and will be part of beta2, expected early next week.

@tacaswell
Copy link
Contributor Author

I can confirm I was able to build basically everything in the scipy stack from that PR branch (modulo some cython issues that are unrelated).

@rgommers rgommers mentioned this issue Jun 5, 2023
Closed
@tacaswell
Copy link
Contributor Author

This is resolved upstream, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
00 - Bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@charris @rgommers @tacaswell @scoder @mattip @lysnikolaou