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

[windows] Implement pthread_mutex for Windows #257

Merged
merged 1 commit into from
Jun 21, 2020

Conversation

walac
Copy link
Contributor

@walac walac commented May 22, 2020

When compiling for Windows, pthread libraries may not be available. For
Win32 applications in which an external implementation of pthread is not available, we build libcompat with the pthread_mutex.c module, which provides a simple implementation of pthread_mutex for the Win32 API.

@walac walac force-pushed the windows-mutex-lock branch 2 times, most recently from eea1348 to 93143eb Compare May 22, 2020 17:56
@brarcher
Copy link
Contributor

Can you give some background on what versions or variations of Windows does not have pthread? There are tests run on AppVeyor, so at least some version of Windows has it. Though, I'm not familiar with what version of Windows is actually used there. At least the compiler versions we test against expose pthreads.

If Check were to support Windows not having pthread we would need a way to test it. That is, some confidence the lack of pthread can be properly detected and when build to use critical sections the result works correctly.

src/check_pack.c Outdated Show resolved Hide resolved
@walac
Copy link
Contributor Author

walac commented May 23, 2020

Can you give some background on what versions or variations of Windows does not have pthread?

afaict, Windows doesn't natively support pthreads. So, unless the client uses a third-party implementation of pthreads for Windows, the code won't be protected by a mutex. And that's is exactly the situation we are facing, we are porting the EFL to Windows, using libcheck for tests. For thread tests, an assertion is raised inside the C runtime when libcheck callsfwrite, due to the FILE * being called from multiple threads without a protecting mutex.

@brarcher
Copy link
Contributor

Thanks for that explanation. It did not occur to me that Windows would not have pthreads natively, though that does make sense. So all this time the protections have been no-ops.... A bit of a miss -- thanks for pointing it out and volunteering to fix it!

If that is the case, Check's existing automated tests on AppVeyor should provide coverage. Disregard that part of my earlier comments.

Given that not having either pthreads caused an issue that Check was unable to detect in its unit tests, maybe it should be a configure-time failure if pthreads (and now critical sections) is unavailable. Or, maybe at least a warning of some sort.

I see that I did not finish the migration from Travis-CI to Github Actions, so the Linux and OSX automated tests did not run for your PR. I'll finish that up now. In a bit I'll let you know, so you can update your branch to pickup testing for Linux and OSX.

@brarcher
Copy link
Contributor

I've enabled the Linux and OSX tests now on Github. Update your branch to pick them up.

@walac walac changed the title [windows] Use critical section when pthread mutex is not available [windows] Implement pthread_mutex for Windows Jun 10, 2020
@walac walac force-pushed the windows-mutex-lock branch 5 times, most recently from 6fdab75 to b74c43e Compare June 10, 2020 19:53
@walac
Copy link
Contributor Author

walac commented Jun 10, 2020

@brarcher Sorry for the delay, I updated the patch according to our previous discussion.

@walac walac requested a review from brarcher June 10, 2020 21:39
@walac walac force-pushed the windows-mutex-lock branch 3 times, most recently from 587d3e6 to 1115e47 Compare June 11, 2020 18:27
@@ -112,7 +114,7 @@ if(WIN32)
if(MSVC60)
set(WINVER 0x0400)
else()
set(WINVER 0x0500)
set(WINVER 0x0600)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with Windows' versioning. Can you explain this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The One-Time Initialization functions were introduced in Windows Vista. The WINVER value for Windows Vista is 0x0600.

src/check_pack.c Outdated Show resolved Hide resolved
@brarcher
Copy link
Contributor

I appreciate your adding support for both Autotools and CMake. (:

When compiling for Windows, pthread libraries may not be available. For
Win32 applications in which an external implementation of pthread is not
available, we build libcompat with the pthread_mutex.c module, which
provides a simple implementation of pthread_mutex for the Win32 API.
@brarcher
Copy link
Contributor

Thanks for implementing this!

@brarcher brarcher merged commit 2ed5f32 into libcheck:master Jun 21, 2020
@walac
Copy link
Contributor Author

walac commented Jun 21, 2020

@brarcher Is it possible we get a new release of the library? I would like to update the meson wrapdb.

@brarcher
Copy link
Contributor

Sure. I'll create a release today.

@brarcher
Copy link
Contributor

Check 0.15.0 has been released.

@walac
Copy link
Contributor Author

walac commented Jun 22, 2020

Check 0.15.0 has been released.

Thanks a lot \o/

@walac walac deleted the windows-mutex-lock branch June 22, 2020 15:40
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.

2 participants