Page MenuHomePhabricator

[darwin] switch blocking mutex from osspinlock to os_unfair_lock
ClosedPublic

Authored by aralisza on Feb 25 2021, 4:04 PM.

Details

Summary

OSSpinLock is deprecated, so we are switching to os_unfair_lock. However, os_unfair_lock isn't available on older OSs, so we keep OSSpinLock as fallback.

Also change runtime assumption check to static since they only ever check constant values.

rdar://69588111

Diff Detail

Event Timeline

aralisza requested review of this revision.Feb 25 2021, 4:04 PM
aralisza created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2021, 4:04 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
aralisza updated this revision to Diff 326583.Feb 25 2021, 7:43 PM

make owner check static as well

aralisza edited the summary of this revision. (Show Details)Feb 25 2021, 7:44 PM
aralisza edited the summary of this revision. (Show Details)
aralisza edited the summary of this revision. (Show Details)Feb 26 2021, 2:37 PM
aralisza added reviewers: delcypher, yln.
yln accepted this revision.Feb 26 2021, 5:13 PM

LGTM with nits!

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
514
515

I think this "not locked" (no owner) assert got lost accidentally.

517

Good catch for spotting that these can be static asserts!

Let's move them out of the function to show that they don't execute when it is called and make the function easier to read.
https://stackoverflow.com/questions/3718910/can-i-get-the-size-of-a-struct-field-w-o-creating-an-instance-of-the-struct

This revision is now accepted and ready to land.Feb 26 2021, 5:13 PM
delcypher requested changes to this revision.Feb 26 2021, 5:31 PM

LGTM apart from the owner_ check. Turning as many of the runtime checks into compile time checks seems like a good idea :)

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
514

Note:

Availability on this API is

#define OS_UNFAIR_LOCK_AVAILABILITY \
		__API_AVAILABLE(macos(10.12), ios(10.0), tvos(10.0), watchos(3.0))

but our minimum deployment targets are

macOS: 10.10
iOS: 9.0
tvOS: 9.0
watchOS: 2.0

which makes me sad because it means we have to guard use of the API like this.

@yln You've done more of these conditional API checks recently. Does this look the right thing to do?

517

I'm not sure this should really be a static check. At this point in the code we don't know which constructor was used for initialization. This static assert is actually only checking the constexpr constructor but it's not checking the other constructor (because it can't).

We're not actually using the owner_ field AFAICT so I'm not really sure if us checking the value of the owner_ field actually really matters here. If there's agreement here we could drop the check. If not then we should probably keep CHECK_EQ(owner_, 0).

This revision now requires changes to proceed.Feb 26 2021, 5:31 PM
aralisza updated this revision to Diff 326858.Feb 26 2021, 5:56 PM

move static checks and make owner check runtime check again

aralisza added inline comments.Feb 26 2021, 5:58 PM
compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
517

the owner_ field is private, so I can't access it outside of the method definitions. I moved it to the constructor, is that a good alternative? Or is there a better way?

delcypher accepted this revision.Feb 26 2021, 7:19 PM
delcypher added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
517

@yln I wouldn't expect that static_asserts would ever be executed. But you're right that moving them elsewhere might help clarity.

517

There are terrible alternatives (like adding a "friend" function) but let's not do that. I think that moving the static_asserts that we can't move to global scope into the BlockingMutex constructor is reasonable.

This revision is now accepted and ready to land.Feb 26 2021, 7:19 PM
hans added a subscriber: hans.Mar 4 2021, 3:23 AM

This broke Chromium's AddressSanitizer build on Mac: https://crbug.com/1184481

Based on D97821 and D97880 it looks like you're planning on reverting this, so I've gone ahead and pushed 840a16d3c4cbb193dde4e1f41f9137a2a234d0f8 to do that.