Page MenuHomePhabricator

Implement eina_thread for native windows
Needs ReviewPublic

Authored by walac on Jun 29 2020, 7:50 AM.



The implementation design respects the fact that Eina_Thread is an
uintptr_t. This means we need to allocate the thread struct in the heap
and return the pointer. This creates a complication for eina_thread_self,
as the returned Eina_Thread is not supposed to be explicitly freed (like
the posix implementation counterpart).

To solve this problem, we store the created thread structure in the
target thread TLS slot. For threads that were not created through eina
API, in eina_thread_self we allocate a new structure, push it to the TLS
slot and mark it to be freed on thread exit.

Notice we prefer _beginthreadex over CreateThread. _beginthreadex,
besides creating a new thread, ensures the C runtime will work on a
multithread environment.

A new variable is introduced in the meson builds, called
sys_native_windows, that is true when we are compiling for Windows
outside mingw or cygwin. We check it by looking at the compiler we are
using. If it is Microsoft Visual C++ or clang-cl, we assume it is a native
Windows compilation linking directly to the Microsoft runtime libraries.

Diff Detail

rEFL core/efl
No Linters Available
No Unit Test Coverage
Build Status
Buildable 17106
Build 11383: arc lint + arc unit
walac created this revision.Jun 29 2020, 7:50 AM
walac requested review of this revision.Jun 29 2020, 7:50 AM

test this program on linux, then test it on windows and see if it is working nicely

147 ↗(On Diff #30800)

i don't think it is correct : log should be init before any usage of the eina_log macro, and for example, inarray is using it


EINA_THREAD_USE_WIN32 is defined by the meson build system. So if someone wants to use the native implementation, it must define it

either you find a way to not use this macro (is _WIN32 sufficient ?) or maybe adding it in eina.pc

cedric added inline comments.Jun 29 2020, 9:24 AM

This is a symbol that will only exist on Windows and not on any other architecture, not sure it is the best way to go. You could move all those #if in a wrapping function, that would be a bit cleaner I think.


Same things here.

Thanks for the feedback. Indeed those points were missed in the internal review. Thanks.

vtorri added inline comments.Jun 29 2020, 9:37 AM

i would like to quote Keith Marshall, one of the MinGW devs (not mingw-w64) : "it is useless to test something that will anyway exist."
So is it necessary to test if process.h exists as it will anyway exist on Windows. and _beginthreadex() will also necessarly exist on Windows (both exist since win xp)

so I think this part is useless, and as you said (and as recommended by msdn ( :

so simplify all this by using only _beginthreadex

vtorri added inline comments.Jun 29 2020, 9:39 AM

i do not agree : this code will work with mingw-w64 too

walac added inline comments.Jun 29 2020, 10:04 AM
147 ↗(On Diff #30800)

We have a cyclic dependency here then because the log implementation calls eina_thread_self, and it used to do so before eina_thread_init was called, and this impacted the windows implementation.


EINA_THREAD_USE_WIN32 is defined in eina_config.h, which is included by eina_thread.h. No further action is needed by the client, is it?


That was my first approach, but pthread_cleanup_push and pthread_cleanup_pop are defined as macros, which must be called in the same scope (at least in mingw), so it is not possible to isolate them in a function. So the solution was to create a custom function for Windows.


So, should the implementation in mingw move from Posix to native?

try also expedite ( with the software (gdi or ddraw) backend to benchmark it, compared to the current efl git

walac added inline comments.Jun 29 2020, 10:07 AM

When developing, I was testing on mingw, and I didn't know if mingw has process.h and if this process.h is the same as msvcrt. The same applies for _beginthreadex.

cedric added inline comments.Jun 29 2020, 10:24 AM

Oh, that's annoying, but well, I guess that's the way to go then :-(

vtorri added inline comments.Jun 29 2020, 10:44 AM
147 ↗(On Diff #30800)

so that must be solved in a way or another. like thread before log ?


and isn't _WIN32 sufficient ? I think it is.


i told @jptiz that that was something that i wanted, if it works of course :-p


mingw is (basically) a setof headers compatible with the Windows SDK, needed to develop on Windows, and the import libraries. The rest (the DLL) are provided by Windows itself

you could also have tried git grep process.h or git grep _beginthreadex and see it is used...

so, don't worry, everything you will use is provided by mingw-w64

so remove the checks below and the check on mscv or clanc-cl, they are useless

for expedite, with and without -y parameter (async on/off)

walac updated this revision to Diff 30801.Jun 29 2020, 12:52 PM

Use native windows threads in mingw

walac updated this revision to Diff 30802.Jun 29 2020, 1:17 PM

Fix undefined behavior in eina_thread_self

walac marked 13 inline comments as done.Jun 29 2020, 1:36 PM
raster added a subscriber: raster.Jun 30 2020, 1:08 AM

as per inline comments

122 ↗(On Diff #30802)

eina array and inarray use eina log and thus log needs to be initted before these are as already pointed out. this has o be solved in some form.


APIs inside an ifdef in a public header... bad... :( make it always there or not there... :)


as above. should always be there, or not exist in public API


i know it isn't 100% necessary but can you at least init these to 0 or something explicitly? :)


i know it isn't 100% necessary but can you at least init these to 0 or something explicitly? :)


TlsGetValue returns a void *... no need to cast that. this is C not C++ :)


again - no need to cast void * to another pointer.


again - no need to cast void * to another pointer.


shouldn't this cast to Eina_Thread ? :)


again - no need to cast void * to another pointer.


can you explain _beginthreadex() more. any function from the system starting with a _ is instantly suspect as internal call to system lib and very likely to not be stable and break in future". that;'s the convention of starting with a _ char - don't do this basically... so can you justify the stability of this call that it won't change or disappear on us in a future windows update?


with if's it's cleaner to use brackets to clearly show intended grouping so here:

if ((affinity >= 0) && (!SetThreadAffinityMask(thr->handle, 1 << affinity)))

it;s a good habit to avoid mistakes on order-of-operation and makes it clear and explicit how the author wanted the logic grouped. can you do the same to other if's as well?


if () grouping here too


no cast needed here (void *)

vtorri added inline comments.Jun 30 2020, 1:52 AM

microsoft has added plenty of underscores in front of CRT function names, see the left of the msdn page above :)

note that, for ecore, the handle returned by _beginthreadex() must wait with the WaitFor* functions, that is, it must be used with ecore_main_win32_handler_add()

ok. find with _beginthreadex(). a windows thing that _xxx funcs are ok... :|

walac updated this revision to Diff 30805.Jun 30 2020, 6:45 AM

Address raster's review comments

walac added inline comments.Jun 30 2020, 6:49 AM

The problem is that pthread_cleanup_push is not available on Windows, and it is implemented as a macro that requires pthread_cleanup_push and pthread_cleanup_pop be in the same scope, which makes impossible to isolate them in a function.

walac updated this revision to Diff 30806.Jun 30 2020, 11:38 AM

Add tests and fix bugs

walac updated this revision to Diff 30840.Jul 7 2020, 11:11 AM

Fix minor compatibility corner cases with the posix counterpart.