Skip to content

bpo-33608: make arm macros match x86 macros #12665

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.

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

Closed
wants to merge 1 commit into from

Conversation

paulmon
Copy link
Contributor

@paulmon paulmon commented Apr 2, 2019

The ARM changes to _Py_atomic_store_32bit don't match the x86 changes and cause a build break.
Changing _Py_atomic_store_32bit for ARM to match x86 fixes the build issue.
@ericsnowcurrently @zooba

https://bugs.python.org/issue33608

@zooba
Copy link
Member

zooba commented Apr 12, 2019

@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
Copy link
Contributor Author

paulmon commented Apr 12, 2019

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
Copy link
Member

zooba commented Apr 12, 2019

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
Copy link
Contributor Author

paulmon commented Apr 12, 2019

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
Copy link
Member

zooba commented Apr 12, 2019

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.

@paulmon
Copy link
Contributor Author

paulmon commented Apr 18, 2019

See #12877

@paulmon paulmon closed this Apr 18, 2019
@paulmon paulmon deleted the arm_33608_fix branch May 23, 2019 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants