libepoxy icon indicating copy to clipboard operation
libepoxy copied to clipboard

Fix Win32 multithread dispatching bugs.

Open gjz010 opened this issue 2 years ago • 7 comments

Using __thread (and __declspec(thread) on MSVC) to replace dirty DllMain hack and thus allowing static build (-Ddefault_library=static). (Possibly solving #200 issue.) Also fixed race condition in dispatch table logic. Now on Win32 it will always use dispatch table. (#199 minimal reproducing example passed.)

gjz010 avatar Nov 16 '21 14:11 gjz010

FYI: since the patch forced using dispatch table on Win32 the compiler will complain about unused epoxy_*_global_rewrite_ptr functions, like:

include/epoxy/wgl_generated.h:956:26: warning: 'epoxy_wglWaitForSbcOML_global_rewrite_ptr' defined but not used [-Wunused-function]
  956 | #define wglWaitForSbcOML epoxy_wglWaitForSbcOML
      |                          ^~~~~~~~~~~~~~~~~~~~~~
../src/dispatch_common.h:105:5: note: in definition of macro 'GEN_GLOBAL_REWRITE_PTR_RET'
  105 |     name##_global_rewrite_ptr args                               \
      |     ^~~~
src/wgl_generated_dispatch.c:4686:1: note: in expansion of macro 'GEN_THUNKS_RET'
 4686 | GEN_THUNKS_RET(BOOL, wglWaitForSbcOML, (HDC hdc, INT64 target_sbc, INT64 * ust, INT64 * msc, INT64 * sbc), (hdc, target_sbc, ust, msc, sbc))
      | ^~~~~~~~~~~~~~
src/wgl_generated_dispatch.c:4686:22: note: in expansion of macro 'wglWaitForSbcOML'
 4686 | GEN_THUNKS_RET(BOOL, wglWaitForSbcOML, (HDC hdc, INT64 target_sbc, INT64 * ust, INT64 * msc, INT64 * sbc), (hdc, target_sbc, ust, msc, sbc))
      |                      ^~~~~~~~~~~~~~~~

(See CI log in https://github.com/anholt/libepoxy/runs/4226313433?check_suite_focus=true). I would appreciate it if you could tell how this should be handled.

gjz010 avatar Nov 16 '21 15:11 gjz010

@ebassi Could you support him further with his question, so this can be merged eventually?

melroy89 avatar Mar 02 '22 22:03 melroy89

This pull request resolved some issues I was seeing around the dispatch returning null for function pointers when accessed from different threads

dpogue avatar Apr 10 '22 22:04 dpogue

Is there any progress on this matter ?

I am too seeing crashes with Windows builds (no issue on Linux) when trying to create and switch to shared GL contexts, with a Second Life third party viewer I am trying to migrate to libepoxy...

EDIT: I applied the "threaded.patch" from https://github.com/dpogue/Plasma/commit/16149d7b52342d4be5dd4936a364bb4e371fd3ae and it indeed seems to solve the crashes I was seeing with libepoxy under Windows (more testing will be needed, but so far, so good).

sldevel avatar Sep 25 '22 10:09 sldevel

@ebassi Could you support him further with his question, so this can be merged eventually?

One way to avoid compiler warnings would be to mark everything with __attribute__((unused)), but that's not really a good plan. I don't really understand why the compiler would complain about unused symbols in a library, though.

ebassi avatar Sep 30 '22 13:09 ebassi

Since I'm not a Windows expert or developer, I'd like to get a second pair of eyes; switching to thread-local storage using compiler annotations instead of run time API is a bit iffy; I don't want to regress the shared library use case on Windows just to fix the static build case. I honestly think that static builds of C library in 2022 are a mistake more often than not, anyway. I care about static builds insofar as they don't require weird contortions.

ebassi avatar Sep 30 '22 13:09 ebassi

As a follow up to my previous comment, I must point out that while the use of multiple shared GL contexts do not crash any more libepoxy with that patch, I am faced with a weird issue, where the viewer process never exits on WinMain() return ! I worked around it by calling "TerminateProcess(GetCurrentProcess(), exit_code);" just before "return exit_code;" at the very end of WinMain(), but it is of course an ugly, dirty hack. This issue also arises, when I configure the viewer not to create shared GL contexts beside the main one (but of course stilll calls createSharedContext() for the main context). Note that I am using the shared variant of libepoxy.

sldevel avatar Oct 02 '22 15:10 sldevel