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

[ronaldoussoren]

A number of processor specific defines are different for x86-64 and arm64, and need to be adjusted in pymacconfig.h.

• Issue: Assertion failure in test_capi test_alignof_max_align_t possible with macOS universal2 (multi-arch) builds #110820

[ronaldoussoren]

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.

[ronaldoussoren]

I've tested this on an M1 system by building a "universal2" build and then running test_capi for both architectures:

$ arch -arm64 ./python.exe -m test test_capi -v 
$ arch -x86_64 ./python.exe -m test test_capi -v 

Without this PR the second command crashes as described in the issue.

[ronaldoussoren]

Longer term it might be more useful to simply pymacconfig.h using pre-defined macro's in clang and GCC, e.g.

#define ALIGNOF_MAX_ALIGN_T  __BIGGEST_ALIGNMENT__
#define SIZEOF_LONG_DOUBLE     __SIZEOF_LONG_DOUBLE__ 

That's for a different PR though, one that won't be back ported to older releases.

[ned-deily]

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?

[ronaldoussoren]

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?

[ronaldoussoren]

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

[ned-deily]

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?

[ned-deily]

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.

[Yhg1s]

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.

[ronaldoussoren]

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);
	}
	]])],

[ned-deily]

LGTM. I did a quick test with a universal2 installer built on an Apple Silicon Mac and test_capi now passes on both archs.

[miss-islington-app]

Thanks @ronaldoussoren for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

[miss-islington-app]

Sorry, @ronaldoussoren, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 15a80b15af9a0b0ebe6bd538a1919712ce7d4ef9 3.12

[bedevere-app]

GH-112864 is a backport of this pull request to the 3.12 branch.