Description

freakboy3742

opened on Apr 18, 2024

Issue body actions

Bug report

Bug description:

The CPython sources include a vendored copy of https://github.com/microsoft/mimalloc.

This was added 5 months ago as part of #109914. the commit comment says it is version "v2.12". That doesn't appear to be a mimalloc release, but v2.1.2 does exist, and was the current stable release as of November 2023.

However, that code references TARGET_IOS_IPHONE and TARGET_IOS_SIMULATOR. These symbols were deprecated in iOS 15 (released ~2 years ago). These references have been updated in the dev branch of mimalloc (along with many other changes), but there isn't an updated stable mimalloc release at time of writing.

We have also made a number of other changes to the vendored version (about 18 commits, by my count). It's not clear if these mirror changes that have been made upstream, or if we have effectively forked mimalloc at this point.

The code still appears to work at present, but there's a risk it might not in future when the symbol deprecations are finalised. There may also be other updates and bugfixes in mimalloc that we're not taking advantage of.

This was discovered during an audit of TARGET_OS symbol usage following the report of #117886 and #117891.

CPython versions tested on:

CPython main branch

Operating systems tested on:

macOS

Activity

1. freakboy3742

   added

   type-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or error

   OS-mac

   OS-ios

   on Apr 18, 2024
2. freakboy3742 commented on Apr 18, 2024

   

   freakboy3742

   on Apr 18, 2024

   ContributorAuthor

   More actions

   Tagging @DinoV, @colesbury and @vstinner as they appear to be most active in making modifications to the vendored mimalloc code. I'm happy to submit a patch updating the immediate macOS/iOS preprocessor symbol problem; but I don't know if there are any larger plans to coordinate with upstream mimalloc changes.
3. colesbury commented on Apr 19, 2024

   

   colesbury

   on Apr 19, 2024

   Contributor

   More actions

   These references have been updated in the dev branch of mimalloc

   The relevant upstream branch is dev-slice (v2.x development branch): https://github.com/microsoft/mimalloc/tree/dev-slice.

   But the preprocessor symbols look like they're used in the same places upstream as in CPython:

   dev: https://github.com/microsoft/mimalloc/blob/8f7d1e9a41bb0182166aac6a8d4d8b00f60ed032/src/prim/unix/prim.c#L45
   dev-slice: https://github.com/microsoft/mimalloc/blob/f199b888b47f77261aac9b63b612e77ff3fbd880/src/prim/unix/prim.c#L45

   there's a risk it might not in future when the symbol deprecations are finalised

   What should we replace them with?

   We should make the necessary changes in CPython's copy as well as file an issue and/or PR upstream.

   We're tracking differences from upstream here: #113141. We'll work on getting PRs upstream for the differences sometime after beta 1.
4. freakboy3742 commented on Apr 19, 2024

   

   freakboy3742

   on Apr 19, 2024

   ContributorAuthor

   More actions

   But the preprocessor symbols look like they're used in the same places upstream as in CPython:

   Clearly I need stronger coffee. I don't know what I was looking at, but I swear I saw an updated version.

   What should we replace them with?

   If I'm reading the intention of the code right (i.e., include mach/vm_statistics.h on macOS, but not iOS et al), replacing:

   #if !TARGET_IOS_IPHONE && !TARGET_IOS_SIMULATOR
   

   with

   #if !defined(TARGET_OS_OSX) || TARGET_OS_OSX
   

   should work. That catches the native explicit "This is macOS" symbol present in current macOS SDKs, with a fallback to "well this must be macOS" if the SDK is old enough that it doesn't define the TARGET_OS_* symbols.
5. freakboy3742 commented on Apr 19, 2024

   

   freakboy3742

   on Apr 19, 2024

   ContributorAuthor

   More actions

   I've opened microsoft/mimalloc#879 to track the issue upstream.

   Reacted by Ned Deily
6. colesbury commented on May 16, 2024

   

   colesbury

   on May 16, 2024

   Contributor

   More actions

   @freakboy3742 - is this fixed?
7. freakboy3742 commented on May 18, 2024

   

   freakboy3742

   on May 18, 2024

   ContributorAuthor

   More actions

   @colesbury It looks like it's been fixed upstream; but the upstream changes haven't been merged into the vendored copy in CPython's sources.

Issue actions