CDS and JVM-TI agent questions and crash

[Jiangli Zhou]

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.
>
> 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 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()?

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?

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