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

C++11 thread requirements #274

Merged
merged 3 commits into from
Jul 1, 2020
Merged

Conversation

thrimbor
Copy link
Member

@thrimbor thrimbor commented Jan 6, 2020

This implements all the winapi functionality required to enable the C++11 thread support in libc++: The InitOnce* API, Slim Reader/Writer Locks, and condition variables (I intended to split them up, but it just caused merge conflicts everywhere).

The support for condition variables is basically equivalent to how I implemented it for PDCLib, but it's modified for the lock types winapi provides. One crucial difference to the way this works on Windows, is that Windows uses keyed events to implement condition variables. We don't have that feature in the Xbox kernel, so instead, all functions use the InitOnce* API for initialization of normal events, and an additional function (which doesn't exist on Windows) is provided, DestroyConditionVariable, which frees the events. My thread-support patch for libc++ hooks that function up when building on nxdk.

I've written some test code for SRW locks here, and some test code for the InitOnce* API here.

@dracc
Copy link
Contributor

dracc commented Mar 1, 2020

Looks good to me! 👍
Read the code, compiled and tried it.

I ran the supplied tests successfully.

srwTest
initOnce

The tests behave as expected. 💪

@sam-itt
Copy link
Contributor

sam-itt commented Mar 5, 2020

Built and ran the two test cases: Gives the expected output on both xqemu and real hardware.
Note: The lock/unlock sequence of SRW locks sample is exactly the same on xqemu and my real xbox.

@thrimbor
Copy link
Member Author

Just noticed that one of the code changes was accidentally spread over two commits. I fixed it with a rebase, the actual code didn't change.

lib/winapi/sync.c Outdated Show resolved Hide resolved
lib/winapi/sync.c Outdated Show resolved Hide resolved
lib/winapi/sync.c Outdated Show resolved Hide resolved
lib/winapi/synchapi.h Outdated Show resolved Hide resolved
lib/winapi/sync.c Outdated Show resolved Hide resolved
@dracc
Copy link
Contributor

dracc commented Apr 2, 2020

Tested 3aae926 using the supplied tests both on XQEMU and on real hardware. Still behaves as expected. 👍

lib/winapi/sync.c Outdated Show resolved Hide resolved
@thrimbor thrimbor force-pushed the libcxx_threads_clean branch 2 times, most recently from c43260b to 29e0272 Compare April 8, 2020 13:11
lib/winapi/sync.c Outdated Show resolved Hide resolved
lib/winapi/sync.c Outdated Show resolved Hide resolved
lib/winapi/sync.c Outdated Show resolved Hide resolved
lib/winapi/sync.c Outdated Show resolved Hide resolved
}

if (dwFlags & INIT_ONCE_INIT_FAILED) {
if (lpContext || (dwFlags & INIT_ONCE_ASYNC)) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see anything about this disallowing lpContext on MSDN; I assume this was tested against Windows?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe? Tbh, I don't remember and currently can't find any test code on my HDD. I did switch the machine I do my experiments on a while ago though, so maybe I just wiped it with the old Windows installation.

I did just look up that part in ReactOS though, and it does the same here.

lib/winapi/sync.c Outdated Show resolved Hide resolved
lib/winapi/winbase.h Outdated Show resolved Hide resolved
lib/winapi/sync.c Outdated Show resolved Hide resolved
lib/winapi/sync.c Outdated Show resolved Hide resolved
lib/winapi/sync.c Outdated Show resolved Hide resolved
lib/winapi/sync.c Outdated Show resolved Hide resolved
lib/winapi/winbase.h Outdated Show resolved Hide resolved
return TRUE;
}

BOOL SleepConditionVariableSRW (PCONDITION_VARIABLE ConditionVariable, PSRWLOCK SRWLock, DWORD dwMilliseconds, ULONG Flags)
Copy link
Member

Choose a reason for hiding this comment

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

nit: There's a lot of redundancy with SleepConditionVariableCS that could have been factored out; fine as-is though.

Copy link
Member Author

Choose a reason for hiding this comment

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

True, but the lock stuff is pretty much spread over the entire function. I think factoring it out would make the control flow much harder to understand.

lib/winapi/sync.c Outdated Show resolved Hide resolved
lib/winapi/sync.c Outdated Show resolved Hide resolved
static BOOL WINAPI condvar_init (PINIT_ONCE InitOnce, PVOID Parameter, PVOID *Context)
{
PCONDITION_VARIABLE ConditionVariable = Parameter;
NTSTATUS status;
Copy link
Member

@JayFoxRox JayFoxRox May 12, 2020

Choose a reason for hiding this comment

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

nit: Before creating new events, you could probably check (assert) if this thing is really clean at this point:

  • ConditionVariable->initOnce.Ptr == 0 (this assumption would currently be broken after re-using a CONDITION_VARIABLE without manually setting CONDITION_VARIABLE_INIT after UninitializeConditionVariable)
  • ConditionVariable->waitCount == 0
  • ConditionVariable->eventHandles[0] == INVALID_HANDLE_VALUE
  • ConditionVariable->eventHandles[1] == INVALID_HANDLE_VALUE

You'd just have to reintroduce the initialization of the events to INVALID_HANDLE_VALUE in InitializeConditionVariable.
These assumptions should always be true then, because they come from CONDITION_VARIABLE_INIT or InitializeConditionVariable.

However, I'll leave up to you wether you consider this a useful addition that's worth adding.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I forgot about this.
We can't really check for ConditionVariable->initOnce.Ptr == 0 here, because the InitOnce API functions will change the value of this variable before calling this function, so it's never zero here.
I did add checks for the other conditions though.

@JayFoxRox
Copy link
Member

I'm merging this untested, because it has been here for way too long.
We can iterate if it's broken.

[...] condition variables is basically equivalent to how I implemented it for PDCLib

There have been a couple of improvements in this new implementation during this review, which probably aren't in PDCLib yet.
So please consider improving other variants of this code in PDCLib if they still exist @thrimbor

@JayFoxRox JayFoxRox merged commit a004066 into XboxDev:master Jul 1, 2020
@thrimbor thrimbor deleted the libcxx_threads_clean branch July 3, 2020 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants