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

[David Holmes]

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