GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-143632: Detect availability of `PR_SET_VMA` at runtime (fixes build with musl libc) by sideeffect42 · Pull Request #148771 · python/cpython Conversation
|
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
248dbf9 to
4dcdecf
Compare
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
4dcdecf to
8ddb452
Compare
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| #include "pycore_pystate.h" | ||
|
|
||
| #if defined(HAVE_PR_SET_VMA_ANON_NAME) && defined(__linux__) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I think that you should fix the detecting logic of HAVE_PR_SET_VMA_ANON_NAME not code itself.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
See here:
Lines 5731 to 5740 in bfe6f9f
| # musl libc redefines struct prctl_mm_map and conflicts with linux/prctl.h | |
| AS_IF([test "$ac_cv_libc" != musl], [ | |
| AC_CHECK_DECLS([PR_SET_VMA_ANON_NAME], | |
| [AC_DEFINE([HAVE_PR_SET_VMA_ANON_NAME], [1], | |
| [Define if you have the 'PR_SET_VMA_ANON_NAME' constant.])], | |
| [], | |
| [@%:@include <linux/prctl.h> | |
| @%:@include <sys/prctl.h>]) | |
| ]) | |
| # check for openpty, login_tty, and forkpty |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Why would I fix the detection logic to address a compile-time error in the actual implementation?
When you think about what this configure step actually does, you may notice that what we are detecting there has no real connection to reality.
The set of macro definitions in the two header files is either whatever the libc has copied from a random kernel version (<sys/prctl.h>) or whatever the version of the system's linux-headers package contains (<linux/prctl.h>).
Both of which don't have to match the kernel which will then actually run the interpreter in the future.
Say, the interpreter is compiled as part of the build chain of some distro which is then used as the base image for a Linux container.
The kernel on which this container may be run does not have to have anything in common with the kernel the distro in the base image usually ships.
What does this configure check tell us?
Further, the mere presence of #define HAVE_PR_SET_VMA_ANON_NAME in <linux/prctl.h> says nothing about whether the kernel has CONFIG_ANON_VMA_NAME enabled.
So Python could call prctl() on a kernel which does not support this feature.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I am with @sideeffect42. This has nothing to do with compile time error. It has to do with the implementation. The value it emits in <sys/kernal_info.h> needs to chnage in a future PR. Thoughts ?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
besides, including the linux header on glibc isn't needed either.
#include <sys/prctl.h> int test() {return PR_SET_VMA_ANON_NAME; }
compiles fine on debian trixie. so there's no need for "musl libc detected" workarounds.
obviously, the code using PR_SET_VMA_ANON_NAME can only be compiled if the macro is defined, but OP is correct that the macro can be defined even if the functionality isn't compiled into the kernel.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
6c751d0 to
6805c48
Compare
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
This PR is stale because it has been open for 90 days with no activity. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
6805c48 to
3804915
Compare
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
The compile-time error with musl libc still persists in 3.15.0rc3. It would be great if this bug could be fixed before 3.15 is released. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…ibc)
The build would fail on musl-based systems with the following error:
gcc -c -fno-strict-overflow -Wsign-compare -DNDEBUG -g -O3 -Wall -std=c11 -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror=implicit-function-declaration -fvisibility=hidden -I./Include/internal -I./Include/internal/mimalloc -I. -I./Include -DPy_BUILD_CORE -o Objects/obmalloc.o Objects/obmalloc.c
In file included from ./Include/internal/pycore_mmap.h:16,
from Objects/obmalloc.c:5:
/usr/include/sys/prctl.h:88:8: error: redefinition of 'struct prctl_mm_map'
88 | struct prctl_mm_map {
| ^~~~~~~~~~~~
In file included from ./Include/internal/pycore_mmap.h:15:
/usr/include/linux/prctl.h:134:8: note: originally defined here
134 | struct prctl_mm_map {
| ^~~~~~~~~~~~
make: *** [Makefile:3388: Objects/obmalloc.o] Error 1
This is due to Python including both <linux/prctl.h> and <sys/prctl.h> (which on
glibc imports the definitions from <linux/prctl.h> and on musl includes all the
definitions itself).
Given that Python is not the Linux kernel, <sys/prctl.h> should be included.
Another issue of the previous implementation and the configure check was that
the `_PyAnnotateMemoryMap()` function would only be enabled if the linux-headers
on the build host are recent enough to contain the `PR_SET_VMA_ANON_NAME` macro.
However the resulting binaries might be run on older kernels lacking the
functionality or be built on older systems but run on newer kernels.
With this patch, the function is always enabled and the magic numbers are
included with Python, so that the `prctl()` is always executed and
feature detection is essentially done at runtime.
When run on a kernel too old to support `PR_SET_VMA_ANON_NAME`,
`mmap.mmap.set_name()` will silently do nothing.
When the kernel is new enough but `CONFIG_ANON_VMA_NAME` is not enabled,
`mmap.mmap.set_name()` will raise an `OSError`, e.g.:
>>> mm.set_name("hello")
Traceback (most recent call last):
File "<python-input-5>", line 1, in <module>
mm.set_name("hello")
~~~~~~~~~~~^^^^^^^^^
OSError: [Errno 22] Invalid argument
3804915 to
ad7b55a
Compare
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.