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

[Ioi Lam]

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

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