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

[David Holmes]

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