Skip to content
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

Ensure STDCALL/STDCALLPTR is only invoked on x86 #72

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

GravisZro
Copy link
Contributor

@GravisZro GravisZro commented Apr 18, 2024

Prevent the code from trying to use stdcall for anything except (32-bit) x86 builds because it's the only platform it's applicable for.

This clears up a lot of annoying warning messages.

Alteration to CMakeList.txt is only whitespace clean up.

@GravisZro GravisZro force-pushed the fix/stdcall branch 2 times, most recently from 8c78689 to 21d486a Compare April 18, 2024 19:07
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
lib/module.h Outdated Show resolved Hide resolved
scripts/AIGame.cpp Outdated Show resolved Hide resolved
@winterheart
Copy link
Collaborator

Why just not remove STDCALLs entirely? Is there low-level magic that may prevent it runs in 32bit mode?

@GravisZro
Copy link
Contributor Author

@winterheart x86 calls had several different forms and without specifying how an exposed library function should be structured your compiler may end up structuring or calling that library function incorrectly.

@GravisZro GravisZro force-pushed the fix/stdcall branch 2 times, most recently from fbd3f1c to bfc3c1d Compare April 20, 2024 19:07
@GravisZro GravisZro requested review from Lgt2x and winterheart April 20, 2024 19:14
@GravisZro
Copy link
Contributor Author

@winterheart
You can learn more about why stdcall is needed here: https://en.wikipedia.org/wiki/X86_calling_conventions

@Lgt2x
Copy link
Member

Lgt2x commented Apr 21, 2024

I'll let others review this one, I'm not very comfortable with x86/win32 subjects :)

@Lgt2x Lgt2x removed their request for review April 21, 2024 11:14
@winterheart
Copy link
Collaborator

winterheart commented Apr 21, 2024

I'd suggest move 32-bitness check into configuration level to CMakeLists.txt with (place it after project(...)):

if(CMAKE_SIZEOF_VOID_P EQUAL 4)
    add_definitions("-DD3_ENABLE_STDCALLS")
endif()

And to code goes (lib/module.h or into libraries itself):

#ifdef D3_ENABLE_STDCALLS
#ifdef _MSC_VER // Visual C++ Build
#define STDCALL __stdcall
#define STDCALLPTR *STDCALL
#else // Non-Visual C++ Build
#define STDCALL __attribute__((stdcall))
#define STDCALLPTR STDCALL *
#endif // Visual C++ Build
#else // D3_ENABLE_STDCALLS
#define STDCALL
#define STDCALLPTR STDCALL *
#endif // D3_ENABLE_STDCALLS

And this code will be activated only on 32-bit systems and depends on compiler.

Still, I don't know why this still needed on 32-bit Linux systems, but for compatibility I would left it as is.

@winterheart winterheart added the 64bit Issue or pull relates to 64bit label Apr 21, 2024
@GravisZro
Copy link
Contributor Author

GravisZro commented Apr 22, 2024

Basing the architecture on CMAKE_SIZEOF_VOID_P will trigger ANY 32-bit build, including x86, ARMv7, PPC, et ceterea.

I hoped CMake would have a good way to the target architecture but it's OS specific.

As for leaving the STDCALL code "as is", doing so will result in build failures for non-x86/x86_64 targets.

I'll update the code.

@GravisZro GravisZro force-pushed the fix/stdcall branch 4 times, most recently from 24d06b0 to 6a1d3bb Compare April 22, 2024 02:10
@GravisZro
Copy link
Contributor Author

@winterheart, I have revised the code to exclude non-C++17 compilers and removed duplicate detection macros. I hope this is now sufficiently terse.

Prevent the code from trying to use stdcall for anything except (32-bit) x86 builds
because it's the only platform it's applicable for.
@winterheart winterheart merged commit 28df52f into DescentDevelopers:main Apr 22, 2024
5 checks passed
@GravisZro GravisZro deleted the fix/stdcall branch April 23, 2024 16:00
JeodC pushed a commit that referenced this pull request Apr 28, 2024
Ensure STDCALL/STDCALLPTR is only invoked on x86
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
64bit Issue or pull relates to 64bit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants