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

[Jiangli Zhou]

Thanks!

Jiangli


On 8/16/18 11:21 AM, coleen.phillimore at oracle.com wrote:
>
> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>