RFR: 8141341: CDS should be disabled if JvmtiExport::should_post_class_file_load_hook() is true

[Jiangli Zhou]

Hi Serguei,

Thank you for looking into the changes. With Ioi and Karen’s suggestion for dynamically attached agent and -Xshare:on, I’m reworking the changes. I’m planning to remove the check below and do that in a later place. I’ll post the new webrev next week.

Thanks,
Jiangli

> On Jul 8, 2016, at 12:59 PM, serguei.spitsyn at oracle.com wrote:
> 
> Hi Jiangli,
> 
> 
> src/share/vm/memory/metaspace.cpp
> 
> 3177 if (UseSharedSpaces) {
> 3178 FileMapInfo* mapinfo = new FileMapInfo();
> 3179
> 3180 if (JvmtiExport::should_post_class_file_load_hook()) {
> 3181 // Currently CDS does not support JVMTI CFHL when loading shared class.
> 3182 // If JvmtiExport::should_post_class_file_load_hook is already enabled,
> 3183 // just disable UseSharedSpaces.
> 3184 FileMapInfo::fail_continue("Tool agent requires sharing to be disabled.");
> 3185 } else {
> 
> 
> I understood, the FileMapInfo::fail_continue() will fail if the RequireSharedSpaces is enabled which is expected.
> Should the mapinfo be deallocated in the case FileMapInfo::fail_continue() is called?
> 
> Also, there is a comment below.
> 
> 
> On 7/7/16 18:56, Ioi Lam wrote:
>> (Adding serviceability-dev at openjdk.java.net)
>> 
>> Hi Jiangli,
>> 
>> I think the two blocks of
>> 
>>    if (RequireSharedSpaces) {
>>      tty->print_cr("An error has occurred while loading shared class.");
>>      tty->print_cr("Tool agent requires sharing to be disabled.");
>>      vm_exit(1);
>>    }
>> 
>> should be removed.
>> 
>> If the agent is specified in the command line, the JVM start would be aborted. This is already handled by your changes to src/share/vm/memory/metaspace.cpp
>> 
>> If the agent is attached later, after the VM has started, I think we should not quit the VM. Consider this scenario -- a production server could be running for a long time without issues using -Xshare:on, then when the user attaches a diagnostic agent the JVM suddenly quits.  I think it's better to cause the agent attachment to fail with something like
>> 
>> jvmtiError
>> JvmtiEnv::SetEventNotificationMode(jvmtiEventMode mode, jvmtiEvent event_type, jthread event_thread,   ...) {
>> 
>> ...
>>  if (event_type == JVMTI_EVENT_CLASS_FILE_LOAD_HOOK && enabled) {
>> +   if (RequireSharedSpaces && !universe::is_bootstraping()) {
>> +     tty->print_cr("Tool agent that requires JVMTI_EVENT_CLASS_FILE_LOAD_HOOK cannot be dynamically loaded after JVM has started");
>> +     return JVMTI_ERROR_ILLEGAL_ARGUMENT;
>> +   }
>>    record_class_file_load_hook_enabled();
>>  }
>> ...
>> }
> 
> 
> This is a good concern and suggestion.
> We may need more sophisticated handling for this.
> Sorry, I did not notice it earlier.
> 
> There is a capability can_generate_all_class_hook_events.
> We already have this in the prims/jvmtiManageCapabilities.cpp:
> 
> // if the capability sets are initialized in the onload phase then // it happens before class data sharing (CDS) is initialized. If it // turns out that CDS gets disabled then we must adjust the always // capabilities. To ensure a consistent view of the capabililties // anything we add here should already be in the onload set. void JvmtiManageCapabilities::recompute_always_capabilities() { if (!UseSharedSpaces) { jvmtiCapabilities jc = always_capabilities; jc.can_generate_all_class_hook_events = 1; always_capabilities = jc; } } If enabled can_generate_all_class_hook_events sets the flagJvmtiExport::can_modify_any_class() in jvmtiManageCapabilities.cpp:
> JvmtiExport::set_can_modify_any_class( avail.can_generate_breakpoint_events || avail.can_generate_all_class_hook_events); The flag JvmtiExport::can_modify_any_class() is used here:
> 
> char* FileMapInfo::map_region(int i) { . . . // If a tool agent is in use (debugging enabled), we must map the address space RW if (JvmtiExport::can_modify_any_class() || JvmtiExport::can_walk_any_space()) { si->_read_only = false; }
> 
> So, I'm thinking if we could just skip the CFLH events for the shared classes if the capability can_generate_all_class_hook_events is not enabled and continue posting the events for non-shared classes. Then we do not need to tweak the SetEventNotificationMode() implementation. Otherwise, the fragment above may need to be updated too. To summarize, I understood that the current approach/plan for JDK 9 is:   - Disable the CDS if detected early that the CFLH events are enabled   - Skip the CFLH events for shared classes if CFLH is enabled late Is this right? Is it the same as in the JDK 8? Thanks, Serguei
>> Thanks - Ioi On 7/7/16 6:08 PM, Jiangli Zhou wrote:
>>> Please review the following webrev that disables CDS when JvmtiExport::should_post_class_file_load_hook() is enabled at runtime.    webrev: http://cr.openjdk.java.net/~jiangli/8141341/webrev.00/    bug: JDK-8141341 <https://bugs.openjdk.java.net/browse/JDK-8141341> With -Xshare:on If -Xshare:on is used and JvmtiExport::should_post_class_file_load_hook() is required, the VM now reports "Tool agent requires sharing to be disabled” and terminates. This is the same behavior as jdk8. With -Xshare:auto The change in meatspace.cpp is to detect the case where JvmtiExport::should_post_class_file_load_hook() is enabled very early during JVM initialization. In that case, we disable CDS entirely, including all shared classes, symbols, and Strings objects. The change in systemDictionary.cpp is to detect the cases where JvmtiExport::should_post_class_file_load_hook() is enabled late, which include agent dynamically attached at runtime. In those cases, JVM does not allow loading classes from the shared archive. The shared symbols and Strings can still be used. Thanks, Jiangli