RFR(S) 8187040: ThreadCritical crashes on Solaris if used between os::init and os::init_2

[Mikael Gerdin]

Hi Thomas,

On 2017-09-01 19:31, Thomas Stüfe wrote:
> Hi Mikael,
> 
> (I never understood why we cannot just use pthread mutexes on Solaris. 
> Why all this dynamic loading magic, are pthread functions not available 
> on all Solaris versions?)

Beats me, but I suspect that David is right. At some point someone 
thought one was better and now nobody remembers how and nobody wants to 
put in the performance work to determine if it actually makes a difference.

> 
> Small nit, instead of adding a new variable 
> _synchronization_initialized, how about _mutex_lock != NULL (in 
> ThreadCritical()) and _mutex_unlock != NULL (in ~ThreadCritical())?

I didn't want to expose the function pointers through accessors in 
os::Solaris and I'm worried that if we check a different thing in the 
lock versus unlock paths we can end up with a ThreadCritical which tries 
to unlock a lock which was never locked (because the TC was created 
before _mutex_lock was set).
Also, I think it's clearer to the reader of 
os::Solaris::synchronization_init that the "initialization completed" 
state is exposed to an external caller.



> 
> I am okay with the removal of ::release(). Even if it were used, it is 
> really safer to let the mutex live until process end.

I agree.

> 
> I leave it up to you if you take my suggestion above. The patch is fine 
> for me in the current form.

Thanks for the review, Thomas.
/Mikael

> 
> Kind Regards, Thomas
> 
> 
> On Fri, Sep 1, 2017 at 5:23 PM, Mikael Gerdin <mikael.gerdin at oracle.com 
> <mailto:mikael.gerdin at oracle.com>> wrote:
> 
>     Hi,
> 
>     Please review this small fix to ThreadCritical.
>     When working on a piece of code which allocates memory early on I
>     noticed that it crashed if I enabled NMT.
>     The reason is that NMT uses ThreadCritical and os::Solaris sets the
>     ThreadCritical::_initialized flag before it actually sets up the
>     function pointers the flag is supposed to guard.
>     os::Solaris::_mutex_lock is not initialized until the init_2 phase
>     (after command line flag parsing).
> 
>     My suggested fix is to replace the current short-circuit of
>     ThreadCritical with a flag set when the Solaris mutex code is
>     initialized and thereby getting rid of the initialize function on
>     all platforms.
>     Additionally, ThreadCritical::release is unreachable code and from
>     my research has never actually been called, we might as well get rid
>     of it.
> 
>     Webrev: http://cr.openjdk.java.net/~mgerdin/8187040/webrev.0/
>     <http://cr.openjdk.java.net/~mgerdin/8187040/webrev.0/>
>     Bug: https://bugs.openjdk.java.net/browse/JDK-8187040
>     <https://bugs.openjdk.java.net/browse/JDK-8187040>
>     Testing: JPRT
> 
>     Thanks
>     /Mikael
> 
>