[9] RFR (XS): 8139247: Improper locking of MethodData::_extra_data_lock

[David Holmes]

On 6/03/2016 10:49 PM, Vladimir Ivanov wrote:
> Roland, Dan, David, thanks for the reviews.
>
> Updated fix:
>   http://cr.openjdk.java.net/~vlivanov/8139247/webrev.01/
>
> I found that ciSpeculativeTrapData::translate_from may block on
> Compile_lock (in ciMethod ctor [1]), so MDO extra lock has to be
> unlocked first since it has leaf rank.

That's interesting - it means even if other compilers were not unlocking 
the mutex immediately, they were unlocking it prior to this modified 
code (else the problem would have been detected earlier). Which all 
suggest that as far as locking is concerned the behaviour of this code 
is somewhat untested!

Why was the assertion removed?

Don't forget to update copyright year.

>>> We've been bitten by the "MutexLocker /*missing name*/ (lock);" before.
>>> It's interesting that fixing that runs into JDK-8146616. We should
>>
>> Yeah - how did Vladimir detect that? Detailed check of the generated
>> code?
> Well, initially I missed that the locker doesn't have a name and went
> right to the generated code, since I vaguely remembered that there was
> some bug in SS12u4. And the control flow shape was similar
>
> When I added a name, I looked at it again and noticed unlock() is missed.
>
>>> probably make an audit run on finding missing name instances (again).
>>
>> Yep here's another one in  ./share/vm/utilities/decoder.cpp
>>
>> DecoderLocker::DecoderLocker() :
>>    MutexLockerEx(DecoderLocker::is_first_error_thread() ?
>>                  NULL : Decoder::shared_decoder_lock(), true) {
>>    _decoder = is_first_error_thread() ?
>>      Decoder::get_error_handler_instance() :
>> Decoder::get_shared_instance();
>>    assert(_decoder != NULL, "null decoder");
>> }
> Are you sure? It is a constructor call:
>
> class DecoderLocker : public MutexLockerEx {
>
> All DecodeLocker usages are named.

Yep my mistake - thanks. I was only looking at the grep results, I 
hadn't check the actual usage.

Thanks,
David

>
> Best regards,
> Vladimir Ivanov
>
> [1]
> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/tip/src/share/vm/ci/ciMethod.cpp#l108
>
>
>>
>> Cheers,
>> David
>> -----
>>
>>
>>> Thumbs up!
>>>
>>> Dan
>>>
>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8139247
>>>>
>>>> Solaris Studio C++ compiler generates tightly coupled
>>>> Monitor::lock/unlock calls for unnamed MutexLocker which breaks
>>>> synchronization between ciMethodData::load_extra_data() and
>>>> MethodData::bci_to_extra_data().
>>>>
>>>>   MutexLocker(mdo->extra_data_lock());
>>>>
>>>> is compiled into:
>>>>
>>>>   load_extra_data+0x001d: movq %r13,%rdi
>>>>   load_extra_data+0x0020: call lock [ 0xfffffd7fe6df46c0, .+0x7cc5c0 ]
>>>>   load_extra_data+0x0025: movq %r13,%rdi
>>>>   load_extra_data+0x0028: call unlock [ 0xfffffd7fe6df47f0, .+0x7cc6e8
>>>>
>>>> But after adding a name, we hit a compiler bug in SS12u4 and
>>>> Monitor::unlock call is missed. So, have to add a workaround (same as
>>>> in JDK-8146616 [1]).
>>>>
>>>> Testing: verified machine code for ciMethodData::load_extra_data().
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
>>>>
>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8146616
>>>