[SR-946] Unify mutexes in the Swift runtime

[gparker42]

Previous ID	SR-946
Radar	rdar://25058486
Original Reporter	@gparker42
Type	Improvement
Status	Resolved
Resolution	Done

Additional Detail from JIRA

Votes	0
Component/s	Standard Library
Labels	Improvement, Runtime, StarterBug
Assignee	None
Priority	Medium

md5: af9f82a35d32e960d03bb405bb35fd8d

Issue Description:

The Swift runtime uses two different mutexes internally. Neither one is good. We should write a new swift::mutex or adopt some LLVM mutex that solves these problems.

Some code uses C++ std::mutex. This mutex throws exceptions when errors occur. Nobody can reasonably catch and respond to these errors. Backtraces are lost when these exceptions are caught and re-thrown before the process halts (such as rdar://25058486, probably).

Some code uses pthread_mutex_t. This mutex returns non-zero when errors occur. Our code does not check these errors.

The new mutex implementation should:

• use pthread_mutex_t or some LLVM mutex if a suitable one is available

• implement C++ STL's BasicLockable and Lockable

• halt using swift::reportError when a failure occurs

• not throw exceptions

[gottesmm]

LLVM's mutex:

http://llvm.org/docs/doxygen/html/Mutex_8h_source.html
http://llvm.org/docs/doxygen/html/Mutex_8cpp_source.html

It seems like it is pthreads based and checks the error codes:

bool
MutexImpl::acquire()
{
  pthread_mutex_t* mutex = static_cast<pthread_mutex_t*>(data_);
  assert(mutex != nullptr);

  int errorcode = pthread_mutex_lock(mutex);
  return errorcode == 0;
}

I have not checked all of the entry points though. On the other hand, you are just exchanging a bool for an int.

[swift-ci]

Comment by Shawn Erickson (JIRA)

poking around at this, not yet sure I have time to attempt but looking more likely

[swift-ci]

Comment by Shawn Erickson (JIRA)

I am thinking about building a standalone swift::mutex supporting the requirements outlined. Would this live under swift/lib/basic?

[swift-ci]

Comment by Shawn Erickson (JIRA)

@gparker42 You want it to halt on error correct? I think that means I should use swift::fatalError and not swift::swift_reportError? ...or should I use reportError and then abort?

I see one complication in that internally reportOnCrash (Errors.cpp) uses a pthread mutex that we likely want to cut over to the new swift::Mutex. In theory the mutex used in the crash reporting pathway could report on itself.

[swift-ci]

Comment by Shawn Erickson (JIRA)

Also should I be looking at the totality of things under the swift tree or just subsets of the swift tree for usage of alternate mutexes? I do see use of pthread read-write locks as well? Should I deal with those?

[gparker42]

This is for the Swift runtime only, not the compiler. It should live in, and be used in, swift/include/swift/Runtime and swift/stdlib/public/runtime.

Yes, I meant swift::fatalError not swift::swift_reportError.

Good catch in the error reporting. That code can continue to use a pthread mutex, with a new comment saying why it's not a swift::Mutex. (Alternative: add API to swift::Mutex that returns errors instead of crashing, and use that in the error reporting. Then swift::Mutex could be used as an OS abstraction if Swift is ever ported to a platform that doesn't have pthreads.)

Other constructs like pthread rwlocks can be updated if they are prone to unchecked errors. That can be left to a separate commit.

[swift-ci]

Comment by Shawn Erickson (JIRA)

Thanks! That clears things up for me.

[swift-ci]

Comment by Shawn Erickson (JIRA)

Related pull requests:

• SR-946 - Unify mutexes in the Swift runtime #1731 (merged)

• [runtime] enhanced and refactored recently added Mutex abstraction #1950 (under review)

[gparker42]

This is long since done, I think. Shawn, do you remember anything else to do here?