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

Mimalloc header is not installed #116984

Closed
oraluben opened this issue Mar 19, 2024 · 10 comments
Closed

Mimalloc header is not installed #116984

oraluben opened this issue Mar 19, 2024 · 10 comments
Labels
type-bug An unexpected behavior, bug, or error

Comments

@oraluben
Copy link
Contributor

oraluben commented Mar 19, 2024

Bug report

Bug description:

Mimalloc is introduced in #109914.
This will cause a header not found when building any extension that includes a pycore_*.h header as:

[build] In file included from /Users/yyc/repo/py/install/include/python3.13d/internal/pycore_long.h:13:
[build] In file included from /Users/yyc/repo/py/install/include/python3.13d/internal/pycore_runtime.h:17:
[build] In file included from /Users/yyc/repo/py/install/include/python3.13d/internal/pycore_interp.h:30:
[build] /Users/yyc/repo/py/install/include/python3.13d/internal/pycore_mimalloc.h:39:10: fatal error: 'mimalloc.h' file not found
[build] #include "mimalloc.h"
[build]          ^~~~~~~~~~~~
[build] 1 error generated.

See following PR for detail.

CPython versions tested on:

3.13, CPython main branch

Operating systems tested on:

Linux, macOS

Linked PRs

@oraluben oraluben added the type-bug An unexpected behavior, bug, or error label Mar 19, 2024
@pitrou
Copy link
Member

pitrou commented Mar 19, 2024

Slightly unrelated, but I see that mimalloc types are embedded directly in structures such as PyInterpreterState or PyThreadState.

Unless mimalloc has a stable ABI, does it risk breaking if an extension module or an application embedding Python uses its own different version of mimalloc?

It's also weird that a third-party library is included from CPython headers. Perhaps it would be better to hide this behind opaque pointers.

@encukou @colesbury

@oraluben
Copy link
Contributor Author

oraluben commented Mar 19, 2024

I walked through the discussions in #109914 and seems this issue has been already discussed (at least mentioned, but forgotten later? Or not correctly installed after moved to Internal/? #109914 (review), #109914 (comment), #109914 (comment)). It's somehow intended that this header is not installed, but since cpython decides to install pycore_*.h headers, the mimalloc headers need to be installed as well (otherwise the core headers are all broken).

I also see some related about this:

Unless mimalloc has a stable ABI, does it risk breaking if an extension module or an application embedding Python uses its own different version of mimalloc?

If I'm not mistaken, no symbol is exported (#109914 (comment)), and the reason mimalloc headers are included is that there's extra patches on top of upstream. But the longterm plan is to upstream those (I cannot find the link to this but I believe saw Sam mentioned it somewhere).

Hope it helped @pitrou

@pitrou
Copy link
Member

pitrou commented Mar 19, 2024

If I'm not mistaken, no symbol is exported

Sure, but:

  1. mimalloc.h is included, and it could come from whatever install of mimalloc is used by third-party extension code, not necessarily the mimalloc used for building CPython
  2. the binary layout of PyInterpreterState and PyThreadState depends on the size of mimalloc types, which may vary from one mimalloc version to another.

@colesbury
Copy link
Contributor

colesbury commented Mar 19, 2024

@pitrou:

  1. mimalloc.h is only included in internal headers (pycore_xxx.h). Not in any of the public APIs (Include or Include/cpython).
  2. PyInterpreterState is opaque. The mimalloc structs are included in _PyThreadStateImpl, not PyThreadState. _PyThreadStateImpl is not in any of the public headers.

@pitrou
Copy link
Member

pitrou commented Mar 19, 2024

@colesbury This issue shows that "internal" headers are included by third-party code. Presumably, PyInterpreterState and PyThreadState are also consumed by third-party code.

@colesbury
Copy link
Contributor

Yes, people sometimes include our internal headers. People also sometimes copy-paste our internal headers. We don't make attempts to avoid language level name conflicts in these cases.

@pitrou
Copy link
Member

pitrou commented Mar 19, 2024

I certainly don't have a horse in this race, but I find it weird to on the one hand fix this issue by installing the mimalloc headers, and on the other hand to not care about the potential consequences of including said headers in third-party code.

@oraluben
Copy link
Contributor Author

I'd also prefer to not need to add Internal/mimalloc as extra include path, even after the headers are installed, if that's possible.

erlend-aasland pushed a commit that referenced this issue Apr 23, 2024
- Install mimalloc header only when enabled
- Rename WITH_MIMALLOC to INSTALL_MIMALLOC
@colesbury
Copy link
Contributor

Reopening this for the Internal/mimalloc include path issue.

@colesbury colesbury reopened this May 8, 2024
colesbury added a commit to colesbury/cpython that referenced this issue May 8, 2024
…t file

Some embedders and extensions include parts of the internal API. The
pycore_mimalloc.h file is transitively include by a number of other
internal headers. This avoids include errors for code that was
already including those headers.
colesbury added a commit to colesbury/cpython that referenced this issue May 8, 2024
…t file

Some embedders and extensions include parts of the internal API. The
pycore_mimalloc.h file is transitively include by a number of other
internal headers. This avoids include errors for code that was
already including those headers.
colesbury added a commit that referenced this issue May 9, 2024
…#118808)

Some embedders and extensions include parts of the internal API. The
pycore_mimalloc.h file is transitively include by a number of other
internal headers. This avoids include errors for code that was
already including those headers.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 9, 2024
…t file (pythonGH-118808)

Some embedders and extensions include parts of the internal API. The
pycore_mimalloc.h file is transitively include by a number of other
internal headers. This avoids include errors for code that was
already including those headers.
(cherry picked from commit 71cc065)

Co-authored-by: Sam Gross <[email protected]>
colesbury added a commit that referenced this issue May 9, 2024
…nt file (GH-118808) (#118866)

Some embedders and extensions include parts of the internal API. The
pycore_mimalloc.h file is transitively include by a number of other
internal headers. This avoids include errors for code that was
already including those headers.
(cherry picked from commit 71cc065)

Co-authored-by: Sam Gross <[email protected]>
SonicField pushed a commit to SonicField/cpython that referenced this issue May 10, 2024
…t file (python#118808)

Some embedders and extensions include parts of the internal API. The
pycore_mimalloc.h file is transitively include by a number of other
internal headers. This avoids include errors for code that was
already including those headers.
@oraluben
Copy link
Contributor Author

Closing for the PRs from @colesbury have been landed, thanks for make this more friendly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

4 participants