RFR 8185103: TestThreadDumpMonitorContention.java crashed due to SIGSEGV in G1SATBCardTableModRefBS::write_ref_field_pre_work

[coleen.phillimore at oracle.com]

Yes, I like it too.  Looks good.
Coleen


On 8/7/17 9:41 PM, David Holmes wrote:
> Hi Harold,
>
> On 7/08/2017 11:13 PM, harold seigel wrote:
>> Hi David,
>>
>> Thanks for your comments!
>>
>> Please review this updated webrev.  It contains the change that you 
>> suggested.  It also simplifies the implementation by statically 
>> allocating the fixup lists before their first use.
>>
>> http://cr.openjdk.java.net/~hseigel/bug_8185103.2/webrev/index.html
>
> I like it! Not much point in lazy initialization if things will always 
> be initialized anyway. And presumably this has now removed the 
> potential for allocation failure that was significant in setting the 
> klass mirror second, so now we can set it first.
>
> Thanks,
> David
>
>> Thanks, Harold
>>
>> On 8/3/2017 7:03 PM, David Holmes wrote:
>>> Hi Harold,
>>>
>>> On 4/08/2017 7:24 AM, David Holmes wrote:
>>>> Hi Harold,
>>>>
>>>> On 3/08/2017 11:03 PM, harold seigel wrote:
>>>>> Hi,
>>>>>
>>>>> Please review this JDK-10 fix for JDK-8185103.  The problem 
>>>>> occurred because classes were being put on the 
>>>>> fixup_module_field_list before their mirror field was set.  If a 
>>>>> (different) thread called method patch_javabase_entries() before 
>>>>> the class's mirror field was set then 
>>>>
>>>> The code that calls patch_javabase_entries has this:
>>>>
>>>>   // Only the thread that actually defined the base module will get 
>>>> here,
>>>>   // so no locking is needed.
>>>>
>>>>    // Patch any previously loaded class's module field with 
>>>> java.base's java.lang.Module.
>>>>    ModuleEntryTable::patch_javabase_entries(module_handle);
>>>>
>>>> so it seems that comment is wrong and that locking is indeed needed 
>>>> somewhere! At a minimum your setting of the mirror needs a 
>>>> following storestore barrier, or (better) the set/get of the mirror 
>>>> uses load-acquire/store-release.
>>>
>>> Sorry - looking in more detail the necessary locking is already in 
>>> place. A class is only added to the fixup list, under the 
>>> Module_lock, if the base module is not yet defined. The finalization 
>>> of that definition also occurs under the Module_lock, which in turn 
>>> occurs before the fixup list is processed (without the lock). So as 
>>> long as the mirror is set before the class is added to the fixup 
>>> list, the mirror will be visible to the main thread when it 
>>> processes it.
>>>
>>> Looking at the original code:
>>>
>>>  881     // set the module field in the java_lang_Class instance
>>>  882     set_mirror_module_field(k, mirror, module, THREAD);
>>>  883
>>>  884     // Setup indirection from klass->mirror
>>>  885     // after any exceptions can happen during allocations.
>>>  886     k->set_java_mirror(mirror());
>>>
>>> it would seem simplest to just reorder the two actions - except for 
>>> that comment about exceptions. Is the allocation exception issue 
>>> less of an issue when doing VM initialization? What will happen?
>>>
>>> Thanks,
>>> David
>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>> this would cause a SIGSEGV because patch_javabase_entries() 
>>>>> eventually calls obj_field_put() which tries to dereference the 
>>>>> class's mirror field.
>>>>>
>>>>> This change fixes the problem by setting the class's mirror field 
>>>>> before putting the class on the fixup_module_field_list.
>>>>>
>>>>> Open Webrev: 
>>>>> http://cr.openjdk.java.net/~hseigel/bug_8185103/webrev/index.html
>>>>>
>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8185103
>>>>>
>>>>> The fix was tested with the JCK Lang and VM tests, the JTreg 
>>>>> hotspot, java/io, java/lang, java/util and other tests, the 
>>>>> co-located NSK tests, and with JPRT.
>>>>>
>>>>> Additionally, the fix was tested by temporarily adding a 
>>>>> naked_short_sleep(50) to method initialize_mirror_fields() shortly 
>>>>> after it put a class on the fixup_module_field_list. The sleep was 
>>>>> added in order to enhance the likelihood of 
>>>>> patch_javabase_entries() being called before the class's mirror 
>>>>> field got set.  Without the fix, the 
>>>>> TestThreadDumpMonitorContent.java test and the test reported in 
>>>>> JDK-8183309 <https://bugs.openjdk.java.net/browse/JDK-8183309> 
>>>>> reliably got the reported SIGSEGVs.  With the fix, the tests passed.
>>>>>
>>>>> Thanks, Harold
>>>>>
>>