Page MenuHomePhabricator

Implement eina_thread for native windows
Needs ReviewPublic

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

Details

Summary

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

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

https://github.com/shr-project/enlightenment/blob/master/EXAMPLES/ecore_thread/fractale-fp.c

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

src/lib/eina/eina_main.c
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

src/lib/eina/eina_thread.h
257

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.Mon, Jun 29, 9:24 AM
src/lib/eina/eina_thread.h
259

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.

295

Same things here.

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

vtorri added inline comments.Mon, Jun 29, 9:37 AM
src/lib/eina/meson.build
277

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 (https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createthread) :

so simplify all this by using only _beginthreadex

vtorri added inline comments.Mon, Jun 29, 9:39 AM
src/lib/eina/meson.build
208

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

walac added inline comments.Mon, Jun 29, 10:04 AM
src/lib/eina/eina_main.c
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.

src/lib/eina/eina_thread.h
257

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?

259

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.

src/lib/eina/meson.build
208

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

try also expedite (https://git.enlightenment.org/tools/expedite.git/) with the software (gdi or ddraw) backend to benchmark it, compared to the current efl git

walac added inline comments.Mon, Jun 29, 10:07 AM
src/lib/eina/meson.build
277

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.Mon, Jun 29, 10:24 AM
src/lib/eina/eina_thread.h
259

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

vtorri added inline comments.Mon, Jun 29, 10:44 AM
src/lib/eina/eina_main.c
147 ↗(On Diff #30800)

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

src/lib/eina/eina_thread.h
257

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

src/lib/eina/meson.build
208

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

277

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.Mon, Jun 29, 12:52 PM

Use native windows threads in mingw

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

Fix undefined behavior in eina_thread_self

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

as per inline comments

src/lib/eina/eina_main.c
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.

src/lib/eina/eina_thread.h
259

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

297

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

src/lib/eina/eina_thread_win32.c
57

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

59

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

68

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

76

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

85

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

102

shouldn't this cast to Eina_Thread ? :)

115

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

122

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?

145

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?

154

if () grouping here too

285

no cast needed here (void *)

vtorri added inline comments.Tue, Jun 30, 1:52 AM
src/lib/eina/eina_thread_win32.c
122

https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/beginthread-beginthreadex?view=vs-2019

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.Tue, Jun 30, 6:45 AM

Address raster's review comments

walac added inline comments.Tue, Jun 30, 6:49 AM
src/lib/eina/eina_thread.h
259

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.Tue, Jun 30, 11:38 AM

Add tests and fix bugs

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

Fix minor compatibility corner cases with the posix counterpart.