This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Implement semaphores for windows
ClosedPublic

Authored by mstorsjo on Feb 26 2021, 3:27 AM.

Details

Summary

Also add WIN32_LEAN_AND_MEAN before including windows.h, for consistency with other sources.

Diff Detail

Event Timeline

mstorsjo requested review of this revision.Feb 26 2021, 3:27 AM
mstorsjo created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 26 2021, 3:27 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
curdeius added inline comments.
libcxx/src/support/win32/thread_win32.cpp
11–12

A bit unrelated to this patch...

I know that compile times are not a problem in libc++, but could you define WIN32_LEAN_AND_MEAN before including windows.h?

I don't think we use any of the following (https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers#faster-builds-with-smaller-header-files):

Define WIN32_LEAN_AND_MEAN to exclude APIs such as Cryptography, DDE, RPC, Shell, and Windows Sockets.
310

No need for chrono::.
Maybe it would be clearer to write + milliseconds(1) - nanoseconds(1)?
Or even:

milliseconds __ms = std::chrono::ceil<milliseconds>(__ns);

Even if ceil is a C++17 addition, we only compile libc++ with C++17 or C++20.

mstorsjo added inline comments.Feb 26 2021, 6:01 AM
libcxx/src/support/win32/thread_win32.cpp
11–12

I guess I could add that here as well.

310

That looks nice to me. FWIW this bit is a copy from line 250 (but I wouldn't go and change that in this patch).

mstorsjo updated this revision to Diff 326675.Feb 26 2021, 6:13 AM
mstorsjo edited the summary of this revision. (Show Details)

Added WIN32_LEAN_AND_MEAN, using chrono::ceil.

curdeius requested changes to this revision.Feb 26 2021, 8:21 AM

LGTM apart from unnecessary std:: (I wrote std:: without thinking too much before).

libcxx/src/support/win32/thread_win32.cpp
308

OK. Now, there's no need for std::. :)

This revision now requires changes to proceed.Feb 26 2021, 8:21 AM
curdeius accepted this revision.Feb 26 2021, 8:21 AM
This revision now requires review to proceed.Feb 26 2021, 8:21 AM
mstorsjo added inline comments.Feb 26 2021, 8:35 AM
libcxx/src/support/win32/thread_win32.cpp
308

Doh, I thought I had removed the superfluous namespaces, will fix locally.

rnk accepted this revision.Feb 26 2021, 11:55 AM
rnk added a subscriber: ldionne.

Looks good to me, but I am not a libc++ owner, so please wait for @ldionne.

amccarth accepted this revision.Feb 26 2021, 4:04 PM

Looks good enough to me. (But I'm also not a libc++ owner.)

Given that these semaphores cannot be shared across processes, there's possibly a more efficient way to implement them other than mapping them to the corresponding kernel synchronization object (just as a CRITICAL_SECTION is a more efficient mutex if you only need to exclude other threads in the same process). But that's probably not important at this stage.

(I'm amused by the bike-shedding over a nanosecond on the timeout. Windows only times things down to 100 ns.)

libcxx/src/support/win32/thread_win32.cpp
284

The MS docs caution against using SEMAPHORE_ALL_ACCESS if you don't need it, but the explanation isn't clear. https://docs.microsoft.com/en-us/windows/win32/sync/synchronization-object-security-and-access-rights

Instead, they seem to be suggesting STANDARD_RIGHTS_ALL | SEMAPHORE_MODIFY_STATE, but I'm pretty sure that evaluates to the same thing.

This _probably_ doesn't matter, since I don't think the end user will have a way to share these across processes; they aren't named, and--with nullptr for the security attributes--they cannot be inherited. Whether other processes can read or change the ownership and ACL for the object seems moot.

Quuxplusone resigned from this revision.Mar 4 2021, 3:12 PM
Quuxplusone added inline comments.
libcxx/include/__threading_support
154

Does this file actually #include <limits>?
Also: This is a sneaky way to prevent people from using the macro outside of namespace std. 😛

ldionne accepted this revision.Mar 4 2021, 3:16 PM

I trust other reviewers with the implementation.

I've said it before, but I would be happier if we had a tester setup for windows. @rnk @amccarth Do any of you think that would be something feasible? The BuildKite setup is really easy to interface with.

libcxx/include/__threading_support
153

Let's use ::std::numeric_limits<long>::max(), just as good hygiene for a macro.

This revision is now accepted and ready to land.Mar 4 2021, 3:16 PM

I've said it before, but I would be happier if we had a tester setup for windows. @rnk @amccarth Do any of you think that would be something feasible? The BuildKite setup is really easy to interface with.

Yeah, that'd be very much appreciated. With the latest couple patches and the updated instructions in D97166, it now runs pretty well out of the box overall. Continuous testing is sorely needed though, given the recent handful of breakages in MSVC configurations and/or when running tests overall. With current top of tree, there's still around 50 or so failed tests in a full check-cxx, but if someone would set up a buildbot, we could slap XFAIL on those, to be able to guard against further regressions and track what's left if things get cleaned up further.

libcxx/include/__threading_support
153

Ok, will do. FWIW I copied the current form from the <semaphore> header, from the _LIBCPP_NO_NATIVE_SEMAPHORES case (which is the one used on e.g. apple platforms afaik), which also lacks the ::std:: prefix and the explicit <limits> include.

154

It doesn't include <limits> yet, I can add that before pushing.

This revision was automatically updated to reflect the committed changes.