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

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore

This change looks good.  Very minor nit:

+ if(k->java_mirror() == NULL) k->set_java_mirror(mirror());


Needs a space between the if and (.   Probably should also be:

    if (k->java_mirror() == NULL) {
       // Only set the mirror if not set above while setting the module
       k->set_java_mirror();
   }

ie. with a comment.

I do not need to see a new version.

Thanks,
Coleen

On 8/3/17 9:03 AM, 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
> 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
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

harold seigel
Thanks Coleen for the review!

Harold


On 8/3/2017 1:40 PM, [hidden email] wrote:

>
> This change looks good.  Very minor nit:
>
> + if(k->java_mirror() == NULL) k->set_java_mirror(mirror());
>
>
> Needs a space between the if and (.   Probably should also be:
>
>    if (k->java_mirror() == NULL) {
>       // Only set the mirror if not set above while setting the module
>       k->set_java_mirror();
>   }
>
> ie. with a comment.
>
> I do not need to see a new version.
>
> Thanks,
> Coleen
>
> On 8/3/17 9:03 AM, 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
>> 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
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

George Triantafillou
In reply to this post by harold seigel
Hi Harold,

Looks good.

-George

On 8/3/2017 9:03 AM, 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
> 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
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

harold seigel
Thanks George, for the review!

Harold


On 8/3/2017 2:00 PM, George Triantafillou wrote:

> Hi Harold,
>
> Looks good.
>
> -George
>
> On 8/3/2017 9:03 AM, 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
>> 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
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
In reply to this post by harold seigel
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.

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
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 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
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

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

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 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
>>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

harold seigel
Thanks David!

Harold


On 8/7/2017 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
>>>>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore
In reply to this post by David Holmes

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

Loading...