Skip to content

_Py_atomic_add_int16 failure on Windows ARM64 #110105

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
Open
zooba opened this issue Sep 29, 2023 · 2 comments
Open

_Py_atomic_add_int16 failure on Windows ARM64 #110105

zooba opened this issue Sep 29, 2023 · 2 comments
Assignees
@zooba
Labels
3.13 bugs and security fixes OS-windows

Comments

@zooba
Copy link
Member

zooba commented Sep 29, 2023 •
edited by bedevere-app bot
Loading

I saw this error on my private ARM64 builder:

Assertion failed: _Py_atomic_add_int16(&x, -2) == (int16_t)-1, file D:\a\1\s\cpython\Modules\_testcapi\pyatomic.c, line 54

It's a codegen issue in slightly older versions of MSVC, where it does not properly sign extend the value read from x before comparing to a properly sign-extended (int16_t)-1 (comparisons happen in 32 bits).

	ldaxrh      w19,[x9]
	sxth        w19,w19    # this line was missing
	add         w8,w19,#1
	stlxrh      wip1,w8,[x9]
	cbnz        wip1,|$LN10@main|
	dmb         ish

MSVC 19.35 has the issue and MSVC 19.38 seems to have fixed it, but I'm not sure exactly which version. We don't currently use the 16-bit functions anywhere AFAIK, so the only possible change would be to exclude the latter part of the test on older compilers (which is annoying because it's macrofied for all types).

So I guess I'll leave this here as an FYI for when someone sees the crash on their buildbot and has to investigate! And we'll probably close it once we see GHA and AzP on the latest compilers. (@colesbury FYI, not that there's anything to do here)

Linked PRs
@zooba zooba added OS-windows 3.13 bugs and security fixes labels Sep 29, 2023
@zooba zooba self-assigned this Sep 29, 2023
@colesbury
Copy link
Contributor

That's fascinating bug. It looks like this is an issue for starting with MSVC 19.33 and is fixed with MSVC 19.37 (https://gcc.godbolt.org/z/oj36vGqTn). I think it works ok on older versions of MSVC: this buildbot is running 19.31 and doesn't seem to have a problem: https://buildbot.python.org/all/#/builders/730.

I think we can fall back to a compare-exchange loop for older MSVC versions. I can put up a PR, if you'd like. There's another pyatomic.h fix I'm planning to do anyways.

@zooba
Copy link
Member Author

zooba commented Sep 29, 2023

If you want to code it up, sure. I imagine at some point we'll end up on a platform/compiler that has limited interlocked operators, so having the code already there could be handy.

Windows ARM64 is still experimental for a reason though, and this kind of thing is it. I'm also okay with just saying that the latest compiler release is required for it.

colesbury added a commit to colesbury/cpython that referenced this issue Sep 29, 2023
@colesbury
pythongh-110105: Fix MSVC ARM64 atomic add of 16 and 8-bit values
e889f54 
MSVC versions starting with 19.33 (and fixed in 19.37) do not properly
sign extend the value read from the atomic pointer in
`_InterlockedExchangeAdd8` and `_InterlockedExchangeAdd16`. Work around
this issue by using a compare-exchange loop for all MSVC versions prior
to 19.37 on ARM64.
colesbury added a commit to colesbury/cpython that referenced this issue Sep 29, 2023
@colesbury
pythongh-110105: Fix MSVC ARM64 atomic add of 16 and 8-bit values
3f8c15d 
MSVC versions starting with 19.33 (and fixed in 19.37) do not properly
sign extend the value read from the atomic pointer in
`_InterlockedExchangeAdd8` and `_InterlockedExchangeAdd16`. Work around
this issue by using a compare-exchange loop for all MSVC versions prior
to 19.37 on ARM64.
@bedevere-app bedevere-app bot mentioned this issue Sep 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zooba zooba

Labels
3.13 bugs and security fixes OS-windows
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@colesbury @zooba