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

Fix Win32 multithread dispatching bugs. #265

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gjz010
Copy link

@gjz010 gjz010 commented Nov 16, 2021

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.)

Copy link
Collaborator

@ebassi ebassi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this, it's very much appreciated.

src/dispatch_wgl.c Outdated Show resolved Hide resolved
src/gen_dispatch.py Outdated Show resolved Hide resolved
src/gen_dispatch.py Outdated Show resolved Hide resolved
@gjz010
Copy link
Author

gjz010 commented Nov 16, 2021

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.

@melroy89
Copy link

melroy89 commented Mar 2, 2022

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

@dpogue
Copy link

dpogue commented Apr 10, 2022

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

dpogue added a commit to dpogue/Plasma that referenced this pull request Apr 10, 2022
- Resolve multithreading issues
- Enable building as a static library

Ref: anholt/libepoxy#265
dpogue added a commit to dpogue/Plasma that referenced this pull request Apr 11, 2022
- Resolve multithreading issues
- Enable building as a static library

Unfortunately, using it as a static library causes linking errors, so
we're keeping the line that forces it to be a dynamic library on
Windows.

Ref: anholt/libepoxy#265
dpogue added a commit to dpogue/Plasma that referenced this pull request Apr 17, 2022
- Resolve multithreading issues
- Enable building as a static library

Unfortunately, using it as a static library causes linking errors, so
we're keeping the line that forces it to be a dynamic library on
Windows.

Ref: anholt/libepoxy#265
@vldevel
Copy link

vldevel commented Sep 25, 2022

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 dpogue/Plasma@16149d7 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).

@ebassi
Copy link
Collaborator

ebassi commented Sep 30, 2022

@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
Copy link
Collaborator

ebassi commented Sep 30, 2022

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.

@vldevel
Copy link

vldevel commented Oct 2, 2022

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.

franko added a commit to franko/lhelper-recipes that referenced this pull request Dec 15, 2023
Not yet completely tested but there is a crash on Windows and it should
be solved by this patch.

Based on:

anholt/libepoxy#200
anholt/libepoxy#265
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.

5 participants