CDS and JVM-TI agent questions and crash

[Ioi Lam]

I sent RFR to

http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-October/030612.html
http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-October/025640.html

Let's move the discussion to those threads.
Thanks
- Ioi

On 10/19/18 1:48 PM, Jiangli Zhou wrote:
> Hi Ioi,
>
>
> On 10/17/18 10:31 PM, Ioi Lam wrote:
>> On 10/17/18 7:39 PM, Jiangli Zhou wrote:
>>
>>> On 10/17/18 4:54 PM, Ioi Lam wrote:
>>>
>>>> Hi Michael & Volker,
>>>>
>>>>
>>>> Thanks for the bug report and the initial investigation!
>>>>
>>>> Following Volker's investigation, I tried to make a less drastic fix
>>>> (than the "nuclear option" of disable CDS entirely), but run into a 
>>>> lot of
>>>> issues when class-file-load-hook is specified:
>>>>
>>>>
>>>> 1. The shared heap objects are mapped at JVM start-up. At this
>>>> point, we don't know if any of the archived object's class may
>>>> be replaced by the CFLH.
>>> I've given more thoughts to this. Besides Strings, other archived 
>>> objects are already disabled automatically. Archived mirrors and cp 
>>> reference arrays are not installed if their archived class are not 
>>> used. The archived subg-raphs are not used if any of the classes 
>>> (for objects in the subgraphs) are not the same as the archived ones.
>>
>> Thanks for the clarification. This makes the patch simpler.
>>
>>>>
>>>> 2. If java.lang.{Object, Cloneable, Serializable} are replaced
>>>> by CFLH, we cannot use the archived primitive arrays.
>>>>
>>>> 3. There's a lot of code that assumes if UseSharedSpaces==true,
>>>> certain InstanceKlasses must be loaded from CDS. E.g.,
>>>>    JavaClasses::compute_offsets() does nothing if 
>>>> UseSharedSpaces==true,
>>>>    because it assumes that offsets calculated during dump time will 
>>>> match
>>>>    the class loaded at runtime.
>>> Yes, the assumption is baked in our code. We'd have to exam each 
>>> UseSharedSpaces check if we want to support the behavior.
>>
>> I think all of the UseSharedSpaces assumptions affect only classes 
>> loaded inside SystemDictionary::resolve_preloaded_classes(), which is 
>> called in the JVMTI "early" stage. So I've changed the code to 
>> disable CDS only like this (as Michael suggested):
>>
>>   if (JvmtiExport::should_post_class_file_load_hook() && 
>> JvmtiExport::early_class_hook_env()) {
>>     // CDS assumes that no classes resolved in 
>> SystemDictionary::resolve_preloaded_classes
>>     // are replaced at runtime by JVMTI ClassFileLoadHook. All of 
>> those classes are resolved
>>     // during the JVMTI "early" stage, so we're OK if 
>> JvmtiExport::early_class_hook_env()
>>     // is not specified by the agent(s).
>>     FileMapInfo::fail_continue("CDS is disabled because early JVMTI 
>> ClassFileLoadHook is in use.");
>>     return false;
>>   }
>>
>> Updated webrev:
>>
>> http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v02/ 
>>
>>
>> I'll add more asserts and test cases to validate the patch.
>
> Do you have an updated webev with more test cases ready, or you are 
> still working on it? We should consider including more classes from 
> the default classlist in the test. Archived classes loaded during both 
> 'early' phase and after should be tested. We want to make sure with 
> shared archive being used, the visible behavior should be the same as 
> with -Xshare:off (no crash, no assert, no different exception) when 
> working with a JVMTI agent.
>
>>
>>>>
>>>>
>>>> I have developed a jtreg test case for this, and attempted to do a
>>>> less drastic fix, but I am nowhere near done and it's already messy:
>>>>
>>>> http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v00/ 
>>>>
>>>>
>>>>
>>>> Instead, I think we should do this:
>>>>
>>>> http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v01/ 
>>>>
>>> This would brings back the behavior of JDK-8141341. To be more 
>>> cautious this time, is there any case that's covered by the check 
>>> for 
>>> can_generate_all_class_hook_events/can_generate_early_class_hook_events 
>>> capabilities but not for 
>>> JvmtiExport::should_post_class_file_load_hook()?
>>>
>>
>> I looked at JvmtiClassFileLoadHookPoster::post_to_env(). The 
>> "preloaded" classes can be replaced only if at least one agent has
>>
>> (1) set the following 2 capabilities:
>>
>>     caps.can_generate_all_class_hook_events = 1;
>>     caps.can_generate_early_class_hook_events = 1;
>>
>> AND
>>
>> (2) Specified JVMTI_EVENT_CLASS_FILE_LOAD_HOOK
>>
>> See
>>
>> http://hg.openjdk.java.net/jdk/jdk/file/672bc2213cef/src/hotspot/share/prims/jvmtiExport.cpp#l932 
>>
>
> Ok, thanks. To make sure we don't miss any corner cases, we should 
> include serviceability-dev mailing list for the code review also.
>
>>
>>
>>> As part of the JDK-8141341, we also stop loading any additional 
>>> archived classes if JvmtiExport::should_post_class_file_load_hook() 
>>> is set after mapping the shared archive. Should we do that also if 
>>> we are going with this approach?
>>>
>>
>> I don't think that's necessary. It should be OK to replace any 
>> classes that are not in the "preloaded" list.
>>
>> Originally (before JDK 9) we would disable CDS so that classes are 
>> loaded normally, in order for the class file data to be posted to the 
>> CFLH.
>>
>> However, since JDK 9, we have added the ability for posting the class 
>> file data of shared classes to the CFLH, so there's no need to 
>> disable CDS.
>
> For future optimizations, we might want to prevent loading additional 
> shared classes if any of the archived system classes is changed. 
> However, that's out of the scope of the current issue.
>
> Please let me know when you have a new webrev ready.
>
> Thanks,
> Jiangli
>
>>
>> Thanks
>> - Ioi
>>
>>
>>> Thanks,
>>> Jiangli
>>>>
>>>>
>>>> I will test the patch (probably some old tests may fail because 
>>>> they assumed
>>>> that CDS can be enabled when CFLH is in use). I'll post a formal 
>>>> RFR afterwards.
>>>>
>>>> I'll also see if it's possible to allow CDS if 
>>>> can_generate_early_class_hook_events==false.
>>>> I am not entirely sure if all classes affected by UseSharedSpaces 
>>>> are all
>>>> loaded in the "early" stage.
>>>>
>>>> We can try the "messy" fix later, if we have more time to get it 
>>>> right.
>>>>
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>>
>>>> On 10/15/2018 03:35 PM, Jiangli Zhou wrote:
>>>>> On 10/15/18 3:30 PM, Michael Rasmussen wrote:
>>>>>
>>>>>>> One simple alternative fix is to disable sharing when some 
>>>>>>> events (like
>>>>>>> CFLH) were enabled. This in fact was the behavior before JDK 9. 
>>>>>>> In JDK
>>>>>>> 9, we enabled CDS for debugging. Looks like there are corner 
>>>>>>> cases that
>>>>>>> are not uncovered by our testing. I'd like to seek opinions and 
>>>>>>> feedback
>>>>>>> on the importance of supporting debugging with CDS enabled.
>>>>>> Or at least CFLH together with can_generate_all_class_hook_events 
>>>>>> and can_generate_early_class_hook_events capabilities.
>>>>>>
>>>>>> CDS hasn't been part of our own testing scenarios so far, which 
>>>>>> is why we haven't notice these issues on our end before now.
>>>>>> We only just discovered it because of user reports about crashes, 
>>>>>> which turned out to be because the java-11-openjdk package on 
>>>>>> Fedora comes with pre-generated CDS archive.
>>>>>>
>>>>>> But especially with JEP 341 just around the corner, it makes it 
>>>>>> very important to find and weed out these bugs now!
>>>>>
>>>>> Totally agree. These issues are now more important with the 
>>>>> default CDS archive. Thanks for reporting them!
>>>>>
>>>>> Best regards,
>>>>> Jiangli
>>>>>>
>>>>>> /Michael
>>>>>
>>>>
>>>
>>
>