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

[serguei.spitsyn at oracle.com]

On 7/8/16 12:59, 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?
I realized, it is not correct. Sorry for the confusion. The above 
contradicts with the statement from Jiangli: > 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. So, we want to disallow loading classes 
from the shared archive if the CFLH events are enabled late. I'm still 
in process of reviewing. Thanks, Serguei
> 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 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160708/b74a34b7/attachment.html>