RFR: 8218975: Bug in macOSX kernel's pthread support

[David Holmes]

Hi Kim,

On 19/03/2019 9:04 am, Kim Barrett wrote:
>> On Mar 17, 2019, at 9:43 PM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> Hi Kim and Patricio
>>
>> On 16/03/2019 9:03 am, Kim Barrett wrote:
>>> Please review this fix for intermittent Mac-only crashes involving the
>>> OWSTTaskTerminator.
>>> This is joint work with Patricio Chilano Mateo.
>>
>> Fantastic job in pinning this one down!
> 
> For the record, Patricio did the heavy lifting on getting to the
> bottom of this, including building and running patched versions of the
> macOSX kernel and pthread library, developing the standalone reproducer,
> and analyzing its behavior.
> 
>> The fix generally looks good though I agree with Thomas that X_ptr() should just be X().
> 
> OK, _ptr suffix dropped.  In my defence, that convention is used by
> the C++ standard, both in standard APIs and in examples.  Though
> following "standard" conventions would probably use references in a
> lot of places where HotSpot code uses pointers.
> 
>> There is one minor issue:
>>
>> + pthread_mutex_t os::PlatformMonitor::_freelist_lock = PTHREAD_MUTEX_INITIALIZER;
>>
>> on FreeBSD the default mutex initialization doesn't use PTHREAD_MUTEX_NORMAL and so we get a slow debug version IIRC. That's why we explicitly use non-static pthread_mutex initialization using a pthread_mutex_attr. You should be able to move the _freelist_lock initialization to pthread_init_common() with the PLATFORM_MONITOR_IMPL_INDIRECT guard.
> 
> The workaround isn't in FreeBSD code, it is macOSX-specific, where the

Yes my bad! Was thinking too broadly.

> implemented default is "normal" rather than "errorcheck".  But the
> documentation seems to be vague on that (as with FreeBSD and POSIX
> documentation, but not Linux, where the defaults are spelled out).
> Since the cost of explicitly ensuring we use "normal" mutexes is
> small, let's be paranoid and do so.

Okay. That's consistent with the existing unconditional dynamic 
initialization of mutexes etc.

> New webrevs:
> full: http://cr.openjdk.java.net/~kbarrett/8218975/open.02/
> incr: http://cr.openjdk.java.net/~kbarrett/8218975/open.02.inc/

Looks good!

Thanks,
David