Skip to content

Add SystemNative_LowLevelMonitor_TimedWait to System.Native#47325

Merged
CoffeeFlux merged 6 commits intodotnet:masterfrom
CoffeeFlux:native-timedwait
Feb 8, 2021
Merged

Add SystemNative_LowLevelMonitor_TimedWait to System.Native#47325
CoffeeFlux merged 6 commits intodotnet:masterfrom
CoffeeFlux:native-timedwait

Conversation

@CoffeeFlux
Copy link
Contributor

First of 3 PRs.

The second will be adding some of the managed Threading implementations from NativeAOT, and the third will modify them, wire them up to the Mono build, and implement the necessary Mono-specific changes.

@CoffeeFlux CoffeeFlux requested a review from jkotas January 22, 2021 10:38
@ghost ghost added the area-System.Net label Jan 22, 2021
@ghost
Copy link

ghost commented Jan 22, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

First of 3 PRs.

The second will be adding some of the managed Threading implementations from NativeAOT, and the third will modify them, wire them up to the Mono build, and implement the necessary Mono-specific changes.

Author: CoffeeFlux
Assignees: -
Labels:

area-System.Net

Milestone: -

@CoffeeFlux
Copy link
Contributor Author

This PR reminds me—we should really start differentiating between DARWIN and MACOS in System.Native, as there are cases where we want one behavior on desktop and another on, say, tvOS. As it is, TARGET_OSX is used for all those cases, but the _XOPEN_SOURCE change in this PR is only needed on desktop MacOS afaik.

I know historically there was interest in just using TARGET_MACOS in place of TARGET_OSX everywhere due to OSX being outdated, but it doesn't work in Mono for exactly this reason. You have to distinguish between Darwin and MacOS.

@CoffeeFlux
Copy link
Contributor Author

The current iOS/tvOS failures are due to pthread_condattr_setclock not being defined on those platforms. In Mono, we're currently very aggressive about what platforms we don't try to use this/CLOCK_MONOTONIC on:

#if !defined(CLOCK_MONOTONIC) || defined(HOST_DARWIN) || defined(HOST_ANDROID) || defined(HOST_WASM)
#define BROKEN_CLOCK_SOURCE
#endif

This is probably overkill nowadays, but the difference is worth noting. I'll add a check for pthread_condattr_setclock and see if that fixes things for now, but the target distinction I mentioned above would be very helpful for cases like these.

@CoffeeFlux
Copy link
Contributor Author

I think this logic should work, unless I'm misremembering and these are broken at runtime on iOS as opposed to unavailable.

monojenkins pushed a commit to monojenkins/mono that referenced this pull request Jan 22, 2021
Depends on dotnet/runtime#47325 and dotnet/runtime#47327, draft until they're in and I rebase this.

Fixes dotnet/runtime#44795

Best reviewed commit by commit—the commits starting with "Remote appropriate icalls from Mono" are new. In some cases, I've left comments in the commit description. I expect the most interesting commits to be the last few, in particular the annotations.
@jkotas
Copy link
Member

jkotas commented Jan 22, 2021

Still does not build...

@CoffeeFlux
Copy link
Contributor Author

Yeah, I forgot that in System.Native we use #cmakedefine01 instead of just #cmakedefine. Old habits die hard :)

@CoffeeFlux
Copy link
Contributor Author

CoffeeFlux commented Jan 22, 2021

It's not clear to me why this is breaking the CoreCLR desktop builds but not the Mono ones... Is CoreCLR using the System.Native CMake stuff in a way I'm not familiar with? Build error it's hitting is here

#error "Don't know how to perform timed wait on this platform"

@jkotas
Copy link
Member

jkotas commented Jan 22, 2021

I suspect that it is because HAVE_PTHREAD_CONDATTR_SETCLOCK is defined differently under CoreCLR, and the two definitions collide somehow.

@jkotas
Copy link
Member

jkotas commented Jan 22, 2021

It may have something to do with _XOPEN_SOURCE define.

@CoffeeFlux
Copy link
Contributor Author

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@CoffeeFlux CoffeeFlux merged commit d5be845 into dotnet:master Feb 8, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants