bpo-33608: make arm macros match x86 macros by paulmon · Pull Request #12665 · python/cpython

@paulmon

@zooba

@paulmon I'm a little concerned that we're losing the type validation here and hard-casting to a pointer (and potentially losing padding correction, but that seems unlikely with a word-sized struct).

Can you elaborate on what the crash is? Perhaps it's a genuine pointer mismatch?

@paulmon

It's not a crash. It's a compiler error. Changes were made to x86/x64 which build. The changes made to ARM/ARM64 were incomplete and don't compile. You can repro this bug by running pcbuild/build.bat -p arm on the master branch.

Without this change the code evaluates to __InterlockedExchange_acq((volatile long*)&((&((&_PyRuntime.ceval.gil.last_holder)->_value))->_value) and produces this error:
signalmodule.c(251): error C2223: left of '->_value' must point to struct/union

Prior to merging #12240 this compiled for ARM32. After merging the PR it no longer compiles

I believe the cast is required to do a thread-safe InterlockedExchange to copy the pointer because InterlockedExchange doesn't have a version for pointer types.

@zooba

That would seem to suggest that the macro is being used recursively somewhere. The bug Eric's change was fixing was macros being passed expressions that it was misevaluating. This seems to be the same.

I don't have the ARM tools installed on my laptop in a café right now (I assume that's why it gets halfway through building and then the linker complains that the module and target machine types don't match... it'd be nice to fail earlier ;) ). Later today I'll take a look when I can get them installed, but I want these to be correct rather than just hiding issues with casts.

@paulmon

If you look at line 494 here: https://github.com/python/cpython/pull/12240/files
There is a .value that was converted to ->value when calling _Py_atomic_store_32bit.

Then look at lines 410, 413, and 416. There is no .value but ->value was added, but only inside #elif defined(_M_ARM) || defined(_M_ARM64)

So the double ->value comes from referencing ->value in both _Py_atomic_store_32bit and _Py_atomic_store_explicit when it should only be in one of those places.

@zooba

Ah, I see.

@ericsnowcurrently - do we want to normalize on passing _Py_atomic_int pointers into the actual functions, or the pointer values? I think if we pass in the strong type then we can keep the compile-time validation and also the "do nothing" macros, so that's probably better.

@zooba zooba mentioned this pull request

Apr 18, 2019

@paulmon