RFR: JDK-8209389: SIGSEGV in WalkOopAndArchiveClosure::do_oop_work

[coleen.phillimore at oracle.com]

This looks good. Thanks for making this change.
Coleen

On 8/16/18 1:44 PM, Jiangli Zhou wrote:
> Hi Coleen,
>
> Thanks for the review! Changed to use log_error as you suggested and 
> filed JDK-8209598 for converting the rest of the usages in CDS code.
>
> http://cr.openjdk.java.net/~jiangli/8209389/webrev.02/src/hotspot/share/memory/heapShared.cpp.frames.html 
>
>
> http://cr.openjdk.java.net/~jiangli/8209389/webrev.02/src/hotspot/share/memory/metaspaceShared.cpp.frames.html 
>
>
> I left vm_exit calls unchanged for now. If an error occurs during the 
> sub-graph archiving, rollback would be needed for the copied part if 
> we don't abort. Users might prefer exiting immediately when there is 
> an error in some cases. I'll look for approaches to refine this area.
>
> (complete webrev: http://cr.openjdk.java.net/~jiangli/8209389/webrev.02/)
>
> Thanks!
>
> Jiangli
>
>
> On 8/16/18 5:08 AM, coleen.phillimore at oracle.com wrote:
>>
>>
>> http://cr.openjdk.java.net/~jiangli/8209389/webrev.01/src/hotspot/share/memory/heapShared.cpp.udiff.html 
>>
>>
>> + if (archived == NULL) {
>> + ResourceMark rm;
>> + tty->print("Failed to archive %s object ("
>> + PTR_FORMAT "), size[" SIZE_FORMAT "] in sub-graph",
>> + obj->klass()->external_name(), p2i(obj), (size_t)obj->size());
>> + obj->print_on(tty);
>> + vm_exit(1);
>> + }
>>
>> Can you use log_error() for this printing?
>>
>> http://cr.openjdk.java.net/~jiangli/8209389/webrev.01/src/hotspot/share/memory/metaspaceShared.cpp.udiff.html 
>>
>>
>> + } else {
>> + tty->print("Cannot allocate space for object " PTR_FORMAT " in 
>> archived heap region",
>> + p2i(obj));
>> + vm_exit(1);
>> + }
>>
>>
>> Same here.  You may still need vm_exit(1) but it would be nice if you 
>> didn't.
>>
>> Is there an RFE to convert all CDS messages to logging?
>>
>> The fix looks good though.
>>
>> Thanks,
>> Coleen
>>
>>
>> On 8/15/18 8:45 PM, Jiangli Zhou wrote:
>>> Here is the updated webrev: 
>>> http://cr.openjdk.java.net/~jiangli/8209389/webrev.01/.
>>>
>>> Thanks,
>>>
>>> Jiangli
>>>
>>>
>>> On 8/14/18 9:34 PM, Jiangli Zhou wrote:
>>>> Hi Ioi,
>>>>
>>>> Thanks for the review and suggestions. I'll incorporate them.
>>>>
>>>> Thanks,
>>>>
>>>> Jiangli
>>>>
>>>>
>>>> On 8/14/18 6:05 PM, Ioi Lam wrote:
>>>>> Hi Jiangli,
>>>>>
>>>>> The changes look good. I think it's OK to exit the dumping VM 
>>>>> because normally we should not be archiving such large objects.
>>>>>
>>>>> For the various messages, I think we should include the object 
>>>>> size (in bytes).
>>>>>
>>>>> Also, for this message:
>>>>>
>>>>>  395       if (archived == NULL) {
>>>>>  396         ResourceMark rm;
>>>>>  397         tty->print("Failed to archive %s object " PTR_FORMAT 
>>>>> " in sub-graph",
>>>>>  398 obj->klass()->external_name(), p2i(obj));
>>>>>  399         vm_exit(1);
>>>>>  400       }
>>>>>
>>>>> In addition to the size, I think we should also add 
>>>>> obj->print_on(tty) to help diagnosing the problems.
>>>>>
>>>>> Thanks
>>>>>
>>>>> - Ioi
>>>>>
>>>>> On 8/14/18 5:50 PM, Jiangli Zhou wrote:
>>>>>> Please review the following fix that addresses the issue for 
>>>>>> JDK-8209389. A Java object that's larger than one GC region 
>>>>>> cannot be archived as we currently don't support object spanning 
>>>>>> more than one archive heap region. The archiving code needs to 
>>>>>> check the MetaspaceShared::archive_heap_object return value and 
>>>>>> handle failure accordingly. Thanks Claes for finding the edge 
>>>>>> case and reporting the problem!
>>>>>>
>>>>>>   webrev: http://cr.openjdk.java.net/~jiangli/8209389/webrev.00
>>>>>>
>>>>>>   bug: https://bugs.openjdk.java.net/browse/JDK-8209389
>>>>>>
>>>>>> - java_lang_Class::archive_basic_type_mirrors
>>>>>>   Archived object returned by 
>>>>>> MetaspaceShared::archive_heap_object should not be NULL in these 
>>>>>> cases (basic type mirrors are not humongous). Added an assert.
>>>>>>
>>>>>> - HeapShared::archive_reachable_objects_from_static_field
>>>>>>   If the sub-graph entry object is too large, archiving is 
>>>>>> skipped for it’s referenced sub-graph and dumping process continues.
>>>>>>
>>>>>> - WalkOopAndArchiveClosure::do_oop_work
>>>>>>   Abort dumping when archiving failures due to extra-large object 
>>>>>> encountered during the sub-graph archiving.
>>>>>>
>>>>>> Tested with the new test case included in the webrev. Tier1 - 
>>>>>> tier4 testing are in progress.
>>>>>>
>>>>>> Thanks,
>>>>>> Jiangli
>>>>>
>>>>
>>>
>>
>