RFR(S): 8148696: Race loading hsdis may cause SIGSEGV

[Vladimir Ivanov]

Looks good.

Best regards,
Vladimir Ivanov

On 2/8/16 4:24 PM, Nils Eliasson wrote:
> The link to JDK-8071374 was informative, Thanks!
>
> Here is a webrev with just a ttyLocker on the missing path
> (Disassembler::can_decode)
>
> Webrev: http://cr.openjdk.java.net/~neliasso/8148696/webrev.02/
>
> Best regards,
> Nils Eliasson
>
> On 2016-02-06 08:50, Nils Eliasson wrote:
>> Hi Vladimir,
>>
>> On 2016-02-05 17:54, Vladimir Ivanov wrote:
>>>> Summary:
>>>> We have had two crashes on Sparc where two C2 thread simultaneously
>>>> tries to load the hsdis library. Three of four code paths are locked by
>>>> the ttyLocker, but the fourth is open to races.
>>> I'd suggest to either add ttyLocker on the fourth path or try to move
>>> it down the call chain to some shared code. (Keep in mind that
>>> ttyLocker is reentrant!)
>>
>> Yes maybe I should just do that. The time in library check and load is
>> probably insignificant compared to the time spent disassembling and
>> printing.
>>
>>>
>>>> Solution:
>>>> I chose to add another lock (mutex) for this purpose and adapted the
>>>> code so that library_load code works as intented.
>>> Another mutex will not work - tty_lock has the lowest rank (event):
>>>   def(tty_lock                     , Mutex  , event,       true,
>>> Monitor::_safepoint_check_never);      // allow to lock in VM
>>
>> In my suggested code the ttylocker is not held while loading the
>> library. The critical section is slightly smaller than before.
>>
>>>
>>> I experimented with that when fixing 8071374, but decided to guard
>>> everything with tty_lock.
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>
>> I'll have a look at the bug and post a new webrev.
>>
>> Thanks!
>> Nils Eliasson
>>
>>>
>>> [1]
>>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2015-December/020417.html
>>>
>>>
>>> "The fix is to serialize access to Disassembler on tty_lock.
>>> Considering most of the calls to Disassembler::decode are performed
>>> under tty_lock (which has the lowest rank), it's too burdensome to
>>> introduce a dedicated lock and a new rank to please deadlock
>>> detection logic."
>>>
>>>
>>>
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8148696
>>>> Webrev: http://cr.openjdk.java.net/~neliasso/8148696/webrev.01/
>>>>
>>>> Regards,
>>>> Nils Eliasson
>>
>