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

[Calvin Cheung]

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