CDS and JVM-TI agent questions and crash

[Ioi Lam]

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.

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


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

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