RFR[S] 8241158 SA TestHeapDumpForInvokeDynamic.java fails when CDS archive is relocated

[Ioi Lam]

On 4/16/20 11:55 AM, Calvin Cheung wrote:
>
> On 4/16/20 11:12 AM, Ioi Lam wrote:
>> On 4/16/20 10:08 AM, Calvin Cheung wrote:
>>> Hi Ioi,
>>>
>>> Regarding the changes in javaClasses.cpp, I saw that in your new 
>>> code in systemDictionaryShared.cpp, the 
>>> java_lang_Class::update_archived_mirror_native_pointers(m) won't be 
>>> called if MetaspaceShared::relocation_delta() is 0. But I noticed 
>>> there's another code path to 
>>> java_lang_Class::update_archived_mirror_native_pointers() after 
>>> HeapShared::fixup_mapped_heap_regions() is called in 
>>> SystemDictionary::resolve_well_known_classes():
>>>
>>> java_lang_Class::update_archived_mirror_native_pointers(oop) : void
>>>     java_lang_Class::restore_archived_mirror(Klass *, Handle, 
>>> Handle, Handle, Thread *) : bool
>>>         java_lang_Class::fixup_mirror(Klass *, Thread *) : void
>>>             Universe::fixup_mirrors(Thread *) : void
>>> SystemDictionary::resolve_well_known_classes(Thread *) : void
>>>                     SystemDictionary::initialize(Thread *) : void
>>>                         Universe::genesis(Thread *) : void
>>>                             universe2_init() : void
>>>                                 init_globals() : ?
>>>
>>> Will MetaspaceShared::relocation_delta() always be non-zero in the 
>>> above case?
>>>
>> Hi Calvin,
>>
>> Thanks for taking a look at this.
>>
>> As part of this patch, the call path that you mentioned above has 
>> been removed. Now we always call 
>> update_archived_mirror_native_pointers on all archived classes at VM 
>> start-up, so there's no need to patch individual mirrors during class 
>> loading.
>>
>> See the change on line 1294.
>>
>> http://cr.openjdk.java.net/~iklam/jdk15/8241158-sa-heap-dump-fails.v01/src/hotspot/share/classfile/javaClasses.cpp.cdiff.html 
>>
>
> I see. The proposed change looks good.
>

Thanks Calvin!

- Ioi
> thanks,
> Calvin
>>
>> Thanks
>> - Ioi
>>
>>
>>
>>
>>> thanks,
>>>
>>> Calvin
>>>
>>> On 4/14/20 10:07 PM, Ioi Lam wrote:
>>>> Hi Chris,
>>>>
>>>> Thanks for the info. I've synced with the latest repo and updated 
>>>> the webrev in-place:
>>>>
>>>> http://cr.openjdk.java.net/~iklam/jdk15/8241158-sa-heap-dump-fails.v01/ 
>>>>
>>>>
>>>> I'll rerun tiers 1-4 as two of the affected tests 
>>>> (ClhsdbDumpheap.java, TestHeapDumpForInvokeDynamic.java) will now 
>>>> be executed on macos by mach5.
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>> On 4/14/20 3:34 PM, Chris Plummer wrote:
>>>>> [Not a review]
>>>>>
>>>>> Hi Ioi,
>>>>>
>>>>> Your ProblemList.txt is out of date. All those Solaris entries for 
>>>>> 8193639 are now gone.
>>>>>
>>>>> thanks,
>>>>>
>>>>> Chris
>>>>>
>>>>> On 4/14/20 3:01 PM, Ioi Lam wrote:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8241158
>>>>>> http://cr.openjdk.java.net/~iklam/jdk15/8241158-sa-heap-dump-fails.v01/ 
>>>>>>
>>>>>>
>>>>>> This is a bug in the CDS relocation code. When 
>>>>>> -XX:ArchiveRelocationMode=1 is
>>>>>> specified, the CDS archive is mapped to an address picked by the OS
>>>>>> (instead of the default address 0x800000000). As a result, we 
>>>>>> need to patch
>>>>>> the native Klass* pointers inside java.lang.Class instances that are
>>>>>> stored in the CDS archived heap region.
>>>>>>
>>>>>> Before this fix, the patching is done incrementally, when a class 
>>>>>> is resolved.
>>>>>> However, when SA tries to do a heap dump, it may see a 
>>>>>> java.lang.Class instance
>>>>>> that is not yet resolved, whose Klass* pointer has not been patched.
>>>>>> Dereferencing the unpatched pointer causes SA to fail.
>>>>>>
>>>>>> (The failure happens on macos only, probably because the invalid 
>>>>>> dereference
>>>>>> causes different exceptions on other platforms due to specific 
>>>>>> memory layout,
>>>>>> and SA is able to handle such exceptions and limp on).
>>>>>>
>>>>>> The fix is to unconditionally patch all the Klass* pointers 
>>>>>> during VM
>>>>>> initialization.
>>>>>>
>>>>>> We already patch a lot of stuff at start-up when the CDS archive 
>>>>>> is relocated,
>>>>>> so doing a little patching more doesn't hurt.
>>>>>>
>>>>>> ----
>>>>>>
>>>>>> Passed mach5 tiers 1-4. Ran the failed test manually on macos and 
>>>>>> it passed.
>>>>>>
>>>>>> Thanks
>>>>>> - Ioi
>>>>>
>>>>
>>