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

[Vladimir Ivanov]

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.

>> 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.

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
>>