gh-109054: configure checks if libatomic is needed #109101
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vstinner
force-pushed
the
configure_libatomic
branch
from
4a14807
to
dc9754a
Compare
|
Command to test this PR: Output on Linux x86-64: Well,
#if _Py_USE_GCC_BUILTIN_ATOMICS
# define Py_ATOMIC_GCC_H
# include "cpython/pyatomic_gcc.h"
# undef Py_ATOMIC_GCC_H
#elif __STDC_VERSION__ >= 201112L && !defined(__STDC_NO_ATOMICS__)
# define Py_ATOMIC_STD_H
# include "cpython/pyatomic_std.h"
# undef Py_ATOMIC_STD_H
#elif defined(_MSC_VER)
# define Py_ATOMIC_MSC_H
# include "cpython/pyatomic_msc.h"
# undef Py_ATOMIC_MSC_H
#else
# error "no available pyatomic implementation for this platform/compiler"
#endif |
|
!buildbot Raspbian |
|
!buildbot ARM Raspbian |
|
My change doesn't work. Apparently I picked the wrong function for the configure test. I made the assumption that __atomic_fetch_or_8() is related _Py_atomic_or_uint8(). checking whether libatomic is needed by <pyatomic.h>... no [ERROR] _testcapi failed to import: /var/lib/buildbot/workers/pull_request.gps-raspbian.nondebug/build/build/lib.linux-aarch64-3.13/_testcapi.cpython-313-arm-linux-gnueabihf.so: undefined symbol: __atomic_fetch_or_8 |
|
Thanks for running the buildbot test, that was exactly what I planned to do 😁 |
|
Oh wait, it's 8 bytes! Not 8 bits. I found this on the Internet: So there are functions for 1, 2, 4 and 8 bytes. |
Makefile.pre.in
Outdated
| @@ -267,6 +267,7 @@ STATIC_LIBPYTHON= @STATIC_LIBPYTHON@ | |||
|
|
|||
| LIBS= @LIBS@ | |||
| LIBM= @LIBM@ | |||
| LIBATOMIC= @LIBATOMIC@ | |||
There was a problem hiding this comment.
I don't think this is needed; since we only need this for _testcapi, you'll be fine with MODULE__TESTCAPI_LDFLAGS (it is set automatically via PY_STDLIB_MOD in configure.ac.
There was a problem hiding this comment.
Shortly, LIBATOMIC will be needed by more code. I'm pretty sure that @colesbury will use <cpython/pyatomic.h> in a few more C files :-D
Moreover, I'm trying to generate MODULE__TESTCAPI_LDFLAGS=$(LIBATOMIC) in the Makefile. So after configure, it would remind possible to "fix" LIBATOMIC once for all object files which use the variable.
But I failed to fail a syntax to generate this line, configure strongly wants to replace LIBATOMIC variable with its value :-)
There was a problem hiding this comment.
I think this should be done in configure.ac. If it is needed by other extension modules, we add it to their PY_STDLIB_MOD. If it is needed by everything (a global linker option), we can use one of the LDFLAGS variables that are already in place :)
There was a problem hiding this comment.
I disagree, see my comments on the PR below.
vstinner
force-pushed
the
configure_libatomic
branch
2 times, most recently
from
1a97732
to
e241894
Compare
|
I updated my PR:
|
|
!buildbot ARM Raspbian |
|
Since we only need this for
|
|
Ok, with my update, my PR works as expected to fix building the configure: make uses |
|
I modified my PR to generate I made the same change for LIBM. Before, you had to modify all variables using LIBATOMIC and LIBM to update them as well. Concrete example on my Linux x86-64, to force linking to The |
See my previous comment, IMO it's useful to allow overriding LIBATOMIC. In my experience, build systems are not smart enough and they should give back some flexibility to users. As @colesbury shown, Also, you can expect that shortly, LIBATOMIC will be needed in more Python extensions, and Python itself. This PR prepares that. |
I agree, but I think that flexibility should be in configure, and not Makefile. We provide a lot of such customisation options in $ ./configure --help | grep '^\s\+[A-Z]\{2,\}'
GIL (default is no)
PKG_CONFIG path to pkg-config utility
PKG_CONFIG_PATH
PKG_CONFIG_LIBDIR
MACHDEP name for machine-dependent library files
CC C compiler command
CFLAGS C compiler flags
LDFLAGS linker flags, e.g. -L<lib dir> if you have libraries in a
LIBS libraries to pass to the linker, e.g. -l<library>
CPPFLAGS (Objective) C/C++ preprocessor flags, e.g. -I<include dir> if
CPP C preprocessor
HOSTRUNNER Program to run CPython for the host platform
PROFILE_TASK
BOLT_INSTRUMENT_FLAGS
BOLT_APPLY_FLAGS
LIBUUID_CFLAGS
LIBUUID_LIBS
LIBFFI_CFLAGS
LIBFFI_LIBS linker flags for LIBFFI, overriding pkg-config
LIBSQLITE3_CFLAGS
LIBSQLITE3_LIBS
TCLTK_CFLAGS
TCLTK_LIBS linker flags for TCLTK, overriding pkg-config
GDBM_CFLAGS C compiler flags for gdbm
GDBM_LIBS additional linker flags for gdbm
ZLIB_CFLAGS C compiler flags for ZLIB, overriding pkg-config
ZLIB_LIBS linker flags for ZLIB, overriding pkg-config
BZIP2_CFLAGS
BZIP2_LIBS linker flags for BZIP2, overriding pkg-config
LIBLZMA_CFLAGS
LIBLZMA_LIBS
LIBREADLINE_CFLAGS
LIBREADLINE_LIBS
LIBEDIT_CFLAGS
LIBEDIT_LIBS
CURSES_CFLAGS
CURSES_LIBS linker flags for CURSES, overriding pkg-config
PANEL_CFLAGS
PANEL_LIBS linker flags for PANEL, overriding pkg-config
LIBB2_CFLAGS
LIBB2_LIBS linker flags for LIBB2, overriding pkg-config |
As you wish, I reverted my changes to leave Makefile.pre.in unchanged. |
It seems like LIBATOMIC cannot be overriden with my latest change, I don't see it in configure help. I suppose that we can enhance configure later if needed. |
configure.ac
Outdated
| # If the check is done after AC_OUTPUT, modifying LIBS has no effect anymore. | ||
| # <pyport.h> cannot be included alone, it's designed to be included by | ||
| # <Python.h>: it expects other includes and macros to be defined. | ||
| LIBATOMIC="" |
There was a problem hiding this comment.
I don't think is needed anymore :)
There was a problem hiding this comment.
What do you mean? Creating variables is not needed in configure.ac? I prefer to be explicit and declare it to an empty string.
There was a problem hiding this comment.
Oh, LIBATOMIC=${LIBATOMIC-"-latomic"} does nothing if LIBATOMIC is already defined (to an empty string). So I removed it.
vstinner
force-pushed
the
configure_libatomic
branch
from
6f07bd7
to
efd3ec2
Compare
|
@erlend-aasland: I updated my PR, would you mind to review it again? |
|
!buildbot ARM Raspbian |
|
AIX 32-bit is also affected by the issue and my latest PR fix it: #109054 (comment) |
|
!buildbot ARM Raspbian |
|
@gpshead: Apparently, I exhausted my CI budget for today: the buildbot doesn't want to run again :-( Are you able to test my latest PR manually on the buildbot? Or can you maybe provide me a SSH access to it? |
| ]) | ||
|
|
||
| AS_VAR_IF([ac_cv_libatomic_needed], [yes], | ||
| [LIBATOMIC=${LIBATOMIC-"-latomic"}]) |
There was a problem hiding this comment.
I just realised we could probably just use ${VAR="default"} instead of the more verbose VAR=${VAR-"default"}. However, let's stick to the existing practise :) And hopefully, configure.ac is git rm'd some day.
erlend-aasland
approved these changes
Sep 8, 2023
|
Oh, https://buildbot.python.org/all/#/builders/1062/builds/384 ran 3 hours ago: ARM Raspbian PR build is a success! |
|
Thanks a lot for the reviews @erlend-aasland! I merged my PR. I confirm that the end, it's still possible to override LIBATOMIC in configure: |
|
|
|
Minor unrelated issue:
Not good. "Compile host Python" failed with: |
I wrote PR #109211 to fix building Python with cross-compilation (wasm32 buildbots). |
|
I wrote PR #109224 to document |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix building the _testcapi extension on Linux armv6a which requires linking to libatomic when <cpython/pyatomic.h> is used: the
_Py_atomic_or_uint8()function requires libatomic__atomic_fetch_or_8()on this platform.The configure script now checks if linking to libatomic is needed and generates a new LIBATOMIC variable used to build the _testcapi extension.
Building the
_testcapiextension now uses the LIBATOMIC variable in its LDFLAGS, since Modules/_testcapi/pyatomic.c uses <cpython/pyatomic.h>.