_Py_ThreadId() fails when compiled using GCC on Windows

[tonyroberts]

Bug report

Bug description:

It should be possible to build a C extension module using GCC on Windows, but when defining Py_GIL_DISABLED any call to _Py_ThreadId fails.

The if/defs in _Py_ThreadId look for _MSC_VER to be defined to determine whether it's a Windows build or not, but that specifies the VS version and not whether the build is for Windows or not.

Instead it would be better to use _WIN32 (see https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170)

Something along these lines...

#if defined(_WIN32)
#include <intrin.h>
#endif

static inline uintptr_t
_Py_ThreadId(void)
{
    uintptr_t tid;
#if defined(_WIN32) && defined(_M_X64)
    tid = __readgsqword(48);
#elif defined(_WIN32) && defined(_M_IX86)
    tid = __readfsdword(24);
#elif defined(_WIN32) && defined(_M_ARM64)
    tid = __getReg(18);
...

CPython versions tested on:

3.13

Operating systems tested on:

Windows

Linked PRs

• gh-124609: Fix _Py_ThreadId for Windows builds using MinGW #124663
• [3.13] gh-124609: Fix _Py_ThreadId for Windows builds using MinGW (GH-124663) #124698

[colesbury]

I'd be happy to review a PR to support GCC on Windows

[tonyroberts]

Thanks @colesbury, I'll put a PR together tomorrow. Should be a very minor change I think

[zooba]

Just looking at the proposed code now, I think you'll need to check both _MSC_VER (which ought to determine whether intrin.h and __readgsqword is available) and _WIN32 for whether we should be using those registers and offsets.

If GCC on Windows provides the same intrinsics but doesn't define the compatible _MSC_VER (which I thought it did?) then we probably need an additional check there. MS_WINDOWS is better than _WIN32 generally, but we really want to be checking for these features with the correct conditions and not whichever happen to usually line up (or else we cause bugs like this one, where it didn't line up properly 😉 )

[tonyroberts]

Hi @zooba,

I just did a quick check and gcc on windows doesn't define _MSC_VER (at least not using winlibs' gcc 14.1.0). Happy to use MS_WINDOWS. _WIN32 is used elsewhere in the code already but if new code is using that in preference then let's go with that.

The problem with the code right now (

cpython/Include/object.h

Line 174 in a4d1fdf

_Py_ThreadId(void)

) is that if _MSC_VER isn't defined then it uses the "#elif defined(x86_64)" block, which will compile but crash.

With the change to MS_WINDOWS, if __readgsqword is not available then the code will fail to compile, which seems preferable to it compiling the wrong code and causing an error at runtime. Perhaps a specific check for a minimum _MSC_VER could be added (if defined) to give an explicit compile error? Do you know the min value for _MSC_VER that is required? Looks like it's been there since at least VS 2015, but the docs online don't go back further than that (https://learn.microsoft.com/en-us/cpp/intrinsics/readgsbyte-readgsdword-readgsqword-readgsword?view=msvc-140).

Thanks!

ps. I see the #include <intrin.h> isn't needed as it's already added elsewhere

[zooba]

if _MSC_VER isn't defined then it uses the "#elif defined(x86_64)" block, which will compile but crash.

Right, so you need a check for when the target OS is Windows (MS_WINDOWS) but the compiler is not MSVC (_MSC_VER), and then provide alternative (or copies of) code that will work in that situation.

Maybe GCC on Windows has the same intrinsics, but I'm not aware of anything that requires or enforces that. If it is somehow enforced, then sure, but if not then we should detect GCC on Windows explicitly, even if it uses the same code.

Trying to detect too many things with too few conditions is why this bug came up in the first place. Now that we know, let's use the right conditions for all the cases. I don't know those off the top of my head, so when you figure them out, submit a PR and we ought to be able to tell if it looks reasonable or not.