floooh / sokol

minimal cross-platform standalone C headers
https://floooh.github.io/sokol-html5
zlib License
6.92k stars 487 forks source link

[sokol_app] A call to GetIntegerv in _sapp_wgl_create_context leaves the stack corrupted #1005

Closed rcases closed 7 months ago

rcases commented 7 months ago

It can be solved by changing the call convention of GetIntegerv to WINAPI:

void WINAPI (*GetIntegerv)(uint32_t pname, int32_t* data); and _sapp.wgl.GetIntegerv = (void WINAPI (*)(uint32_t, int32_t*)) GetProcAddress(_sapp.wgl.opengl32, "glGetIntegerv");

floooh commented 7 months ago

Looks like an oversight on my side, but interesting that it didn't show up during testing. What compiler are you using?

floooh commented 7 months ago

Ok, should be fixed now. I moved the WINAPI macro inside (*) as in the other GL loader definitions.

Curious though that I wasn't able to reproduce the stack corruption, neither in debug nor release mode (tested with MSVC).

PS: thanks for the bug report and investigation :)

rcases commented 7 months ago

Looks like an oversight on my side, but interesting that it didn't show up during testing

I use Embarcadero's clang compiler. But more than the type of compiler, it is the type of call that you have configured in your test projects, probably stdcall since it is the same as WINAPI. In my case I had cdecl defined by default.

floooh commented 7 months ago

Are you compiling for 32-bits x86 maybe? E.g. I'm seeing this in the MSVC docs:

All __stdcall functions must have prototypes. This calling convention is only available in compilers that target x86, and is ignored by compilers that target other architectures.

...might explain why it didn't crash for me because I'm not testing 32-bit builds anymore (at least 32-bits x86, technically WASM is 32-bits).

rcases commented 7 months ago

Are you compiling for 32-bits x86 maybe? E.g. I'm seeing this in the MSVC docs:

All __stdcall functions must have prototypes. This calling convention is only available in compilers that target x86, and is ignored by compilers that target other architectures.

...might explain why it didn't crash for me because I'm not testing 32-bit builds anymore (at least 32-bits x86, technically WASM is 32-bits).

Yes, I am compiling at 32-bits x86.

floooh commented 7 months ago

Ah ok, that explains it at least. Is there a specific reason for using 32-bit builds though? E.g. in my last job we supported old Windows versions for much longer than even Microsoft did, and we dropped 32-bit support in 2017 and Windows XP support around 2020 or so :)

rcases commented 7 months ago

It is due to poor design on my part of my old libraries and the abusive use of ints. I have never found the time to migrate them since I have not had any need to use elements that require 64 bits (except when accessing files, which I have resolved with a specific type).

In other cases for fast model loading, the format is a copy of the structures in memory with 32-bit pointers between them and a relocation table at the end.

floooh commented 7 months ago

Ah ok makes sense. If you stumble over additional problem please open new tickets. I don't want to "officially" support 32-bit x86, but if it's only about minor things like this then there's no harm in fixing the issues :)