Skip to content

Add an invalid #define for SDL_ThreadID() to SDL_oldnames.h#15801

Merged
slouken merged 3 commits into
libsdl-org:mainfrom
DanielGibson:threadid-warning-header
Jun 12, 2026
Merged

Add an invalid #define for SDL_ThreadID() to SDL_oldnames.h#15801
slouken merged 3 commits into
libsdl-org:mainfrom
DanielGibson:threadid-warning-header

Conversation

@DanielGibson

@DanielGibson DanielGibson commented Jun 10, 2026

Copy link
Copy Markdown
Contributor
  • I confirm that I am the author of this code and release it to the SDL project under the Zlib license. This contribution does not contain code from other sources, including code generated by a Large Language Model ("AI").

Add #define SDL_ThreadID() SDL_ThreadID_is_a_type_now_function_is_SDL_GetCurrentThreadID to SDL_oldnames.h to make sure people don't accidentally use SDL_ThreadID() when SDL_GetCurrentThreadID() was intended.
As SDL_ThreadID is a type in SDL3, it can't be handled with a simple renaming macro.

I added a long explanation about this in SDL_oldnames.h that people will find when running into this compiler error. It includes specific instructions what to do in different situations.

Description

In SDL2, SDL_ThreadID() was a function that's now SDL_GetCurrentThreadID(), but in SDL3 SDL_ThreadID is a type, so in C++ x = SDL_ThreadID() is valid code (default constructor which in case of integers means 0), so that's a massive footgun.

While there is no good reason to use SDL_ThreadID() in SDL3, the #define also causes compiler errors for any code where SDL_ThreadID is followed by (, like function pointer definitions for functions returning SDL_ThreadID - in SDL itself it caused problems in SDL_dynapi.c, that I fixed with an #undef in that file.

These cases should be pretty rare and having to work around them is weighted out (?) by catching the mistake of not renaming SDL_ThreadID() to SDL_GetCurrentThreadID().
By the way, rename_symbols.py so far does not do this rename[*], so it's likely that this is a common issue.
I personally ran into it in dhewm3.

See the big comment in SDL_oldnames.h for more details

[*] and doing so in a way that doesn't cause breakage when running rename_symbols.py twice is non-trivial

Existing Issue(s)

fixes #15749

…dl-org#15749

in SDL2 SDL_ThreadID() was a function that's now
SDL_GetCurrentThreadID(), but in SDL3 SDL_ThreadID is a type, so
in C++ `x = SDL_ThreadID()` is valid code (default constructor which
in case of integers means 0), so that's a massive footgun.

See the big comment in SDL_oldnames.h for more details.

Added `#undef SDL_ThreadID` in SDL_dynapi.c because it has one of the
(quite rare) cases  where "SDL_ThreadID" followed by a "(" is actually
correct and necessary (function pointer returning SDL_ThreadID).
@DanielGibson

Copy link
Copy Markdown
Contributor Author

The failing GDK/MSVC test says RuntimeError: Visual Studio version is incompatible, I don't think that's related to my changes.

BTW: I'm currently adding information about this to README-migration.md

@madebr

madebr commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

The failing GDK/MSVC test says RuntimeError: Visual Studio version is incompatible, I don't think that's related to my changes.

Yes, totally unrelated.

The GDK version we're using on ci does not support Visual Studio 2026.
I changed the job to an earlier version as part of #15797, but I'll push a separate commit so it can be cherry-picked to the 3.4.x branch.

Comment thread include/SDL3/SDL_oldnames.h Outdated
@slouken slouken merged commit b04d026 into libsdl-org:main Jun 12, 2026
2 checks passed
@slouken

slouken commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Merged, thanks! I'm putting this in 3.6.0 so we can catch any more unexpected fallout from this.

@slouken slouken added this to the 3.6.0 milestone Jun 12, 2026
@sezero

sezero commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

@slouken

slouken commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Yup, I went ahead and reverted this.

@DanielGibson

Copy link
Copy Markdown
Contributor Author

Why did the CI builds triggered by the PR not catch this?

@DanielGibson

Copy link
Copy Markdown
Contributor Author

Ah, the fallout is just in sdl2-compat..

Have you considered at least giving me a chance to fix such things before reverting the merged PR? :-p

@DanielGibson

Copy link
Copy Markdown
Contributor Author

The solution is probably the same as what I did in SDL_dynapi.c in this PR:
At the beginning of sdl2_compat.c, after the includes, add

#ifdef SDL_ThreadID
/* prevent the SDL_ThreadID() define from conflicting
   with the SDL_ThreadID() implementation here */
#undef SDL_ThreadID
#endif

@sezero

sezero commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Or, maybe guard your define with !defined(__BUILDING_SDL2_COMPAT__) ?

@DanielGibson

Copy link
Copy Markdown
Contributor Author

Why would I leak SDL2-compat implementation details into SDL3?

I clearly documented that this can lead to false positives in rare edge cases and showed up ways to work around those issues.

I really don't get why the reaction to the first edge case having a problem (and implementing SDL2 with SDL3 definitely is an edge case) is to revert this instead of working around it in the edge case.

@sezero

sezero commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Why would I leak SDL2-compat implementation details into SDL3?

The only purpose of __BUILDING_SDL2_COMPAT__ is to be used in SDL3 headers for situations like this.

@DanielGibson

Copy link
Copy Markdown
Contributor Author

Perhaps more generally useful (in case other projects want to do hacks with their own SDL_ThreadID defines) would be

#ifndef SDL_ThreadID
#define SDL_ThreadID()  SDL_ThreadID_is_a_type_now_function_is_SDL_GetCurrentThreadID
#endif

@madebr

madebr commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Can't we disable SDL_oldnames.h in sdl2-compat?

@DanielGibson

DanielGibson commented Jun 13, 2026

Copy link
Copy Markdown
Contributor Author

Can't we disable SDL_oldnames.h in sdl2-compat?

good point, a #define SDL_oldnames_h_ could probably be used there?

anyway, next try (that should also solve the sdl2-compat problem): #15822

@icculus

icculus commented Jun 13, 2026

Copy link
Copy Markdown
Collaborator

I really don't get why the reaction to the first edge case having a problem (and implementing SDL2 with SDL3 definitely is an edge case) is to revert this instead of working around it in the edge case.

I haven't looked at this specific case, but we tend to revert breaks right away; it's not personal, fixes can be reapplied when ready, but until then the build is broken for everyone that might want to push unrelated fixes. It's not meant to be a rejection of the original concept or anything.

@DanielGibson

DanielGibson commented Jun 13, 2026

Copy link
Copy Markdown
Contributor Author

I haven't looked at this specific case, but we tend to revert breaks right away;

I get this when the break happens in this repo, but elsewhere?

And as I said, this change is expected to break edge cases, for the greater good of showing up (presumably much more common) broken code that uses SDL_ThreadID() instead of SDL_GetCurrentThreadID() with SDL3.

Now my additional change fixes the specific edge case of sdl2-compat, but there are still other edge cases that can't be worked around here (but can relatively easy be worked around in user code)..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SDL_ThreadID still a footgun when migrating from SDL2 to SDL3, NOT handled by rename_symbols.py

5 participants