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.

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

gh-110820: Make sure processor specific defines are correct for Universal 2 build on macOS #112828

Merged
merged 2 commits into from
Dec 8, 2023
Merged

gh-110820: Make sure processor specific defines are correct for Universal 2 build on macOS #112828

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d75e948
gh-110820: Make sure processor specific defines are correct for Unive…
ronaldoussoren Dec 7, 2023
494a6e2
remove debug stuf
ronaldoussoren Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion Include/pymacconfig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
#define PY_MACCONFIG_H
#ifdef __APPLE__

#undef ALIGNOF_MAX_ALIGN_T
#undef SIZEOF_LONG
#undef SIZEOF_LONG_DOUBLE
#undef SIZEOF_PTHREAD_T
#undef SIZEOF_SIZE_T
#undef SIZEOF_TIME_T
Expand All @@ -20,6 +22,7 @@
#undef DOUBLE_IS_BIG_ENDIAN_IEEE754
#undef DOUBLE_IS_LITTLE_ENDIAN_IEEE754
#undef HAVE_GCC_ASM_FOR_X87
#undef HAVE_GCC_ASM_FOR_X64

#undef VA_LIST_IS_ARRAY
#if defined(__LP64__) && defined(__x86_64__)
Expand Down Expand Up @@ -74,8 +77,14 @@
# define DOUBLE_IS_LITTLE_ENDIAN_IEEE754
#endif

#ifdef __i386__
#if defined(__i386__) || defined(__x86_64__)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added __x86_64__ to the check because the compiler included with Xcode 15.1 (and possibly earlier) no longer defines __i386__ when building for Intel.

# define HAVE_GCC_ASM_FOR_X87
# define ALIGNOF_MAX_ALIGN_T 16
# define HAVE_GCC_ASM_FOR_X64 1
# define SIZEOF_LONG_DOUBLE 16
#else
# define ALIGNOF_MAX_ALIGN_T 8
# define SIZEOF_LONG_DOUBLE 8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A compatibility question: up until now, any universal2 executable built on an Intel Mac define 16 for both Intel and Apple Silicon Macs, while builds on Apple Silicon Macs (either single arch or universal2) use 8. With this PR going forward, that would change. Are there any compatibility implications for, say, existing third-party extension modules?

Copy link
Contributor Author

@ronaldoussoren ronaldoussoren Dec 7, 2023 •
edited by ned-deily
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another issue is HAVE_GCC_ASM_FOR_X87, which was silently disabled on macOS for both universal2 and intel-only builds (at least with Xcode 15.1, I haven't checked older Apple compiler).

If I read the code correctly ALIGNOF_MAX_ALIGN_T is only used in typeobject.c in the definition of _align_up and that's to get the correct alignment for type data that's specified with a negative item size which is a feature new in 3.12. This likely only affects performance, although unless the subtype specific data contains vector types (AFAIK those have strict alignment requirements).

I'm not sure how to effectively search for external users of the macro.

My gut feeling is that this should be safe. Maybe ask Thomas about his opinion as RM?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW. I get no hits on SIZEOF_LONG_DOUBLE at all in 3.13, other than exporting it through pyconfig and sysconfig.

Copy link
Member

@ned-deily ned-deily Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My gut feeling is that this should be safe. Maybe ask Thomas about his opinion as RM?

@Yhg1s, @ambv Any opinions? We're early enough in the 3.13 dev cycle that it shouldn't be any issue changing this now regardless. But what about for 3.12 or even 3.11? Is there any reasonable risk that this change could affect existing extension modules (for example, in PyPI wheels) that were built with previous releases of 3.12.x (et al) when used with universal2 builds with this PR merged?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, one reason for backporting is that, without it, tests fail when universal2 builds are built on Apple Silicon Macs and then run on Intel Macs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering the defines are plain wrong, this feels like something that should be backported, yes. The compatibility impact scales with the bug impact: if nobody uses it, nobody should notice; if lots of people use it, they will want the fix. I think it's worth backporting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll back port to 3.12 and propose to skip the back port to 3.11 because the impact there is likely lower:

  • ALIGNOF_MAX_ALIGN_T isn't present in 3.11( added for a feature in 3.12)
  • SIZEOF_LONG_DOUBLE is present, but isn't used by CPython itself
  • HAVE_GCC_ASM_FOR_X64 is present, but isn't used by CPython itself
  • HAVE_GCC_ASM_FOR_X87 is present and used, but X87_DOUBLE_ROUNDING is not set

For the latter: I checked with the configure check for x87 double rounding that double rounding isn't detected when compiling for x86_64 using clang -arch x86_64.

cpython/configure.ac

Lines 5469 to 5492 in 1eac0aa

AC_CACHE_CHECK([for x87-style double rounding], [ac_cv_x87_double_rounding], [
# $BASECFLAGS may affect the result
ac_save_cc="$CC"
CC="$CC $BASECFLAGS"
AC_RUN_IFELSE([AC_LANG_SOURCE([[
#include <stdlib.h>
#include <math.h>
int main(void) {
volatile double x, y, z;
/* 1./(1-2**-53) -> 1+2**-52 (correct), 1.0 (double rounding) */
x = 0.99999999999999989; /* 1-2**-53 */
y = 1./x;
if (y != 1.)
exit(0);
/* 1e16+2.99999 -> 1e16+2. (correct), 1e16+4. (double rounding) */
x = 1e16;
y = 2.99999;
z = x + y;
if (z != 1e16+4.)
exit(0);
/* both tests show evidence of double rounding */
exit(1);
}
]])],

#endif

#endif // __APPLE__
Expand Down
1 change: 1 addition & 0 deletions Include/pyport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,7 @@ extern "C" {
#if !defined(ALIGNOF_MAX_ALIGN_T) || ALIGNOF_MAX_ALIGN_T == 0
# undef ALIGNOF_MAX_ALIGN_T
# define ALIGNOF_MAX_ALIGN_T _Alignof(long double)
# error "Why?"
ronaldoussoren marked this conversation as resolved.
Show resolved Hide resolved
#endif

#ifndef PY_CXX_CONST
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/macOS/2023-12-07-14-19-46.gh-issue-110820.DIxb_F.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Make sure the preprocessor definitions for ``ALIGNOF_MAX_ALIGN_T``,
``SIZEOF_LONG_DOUBLE`` and ``HAVE_GCC_ASM_FOR_X64`` are correct for
Universal 2 builds on macOS.
Loading