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

[serguei.spitsyn at oracle.com]

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 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160708/f944ca6f/attachment-0001.html>