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

[Ioi Lam]

Hi Jiangli,

I agree what what Karen has said.

If the user runs with -Xshare:on, but later attaches an agent, we just 
pretend that the user had specified -Xshare:auto -- allow the agent to 
receive all CFLH events, and stop loading from the shared archive.

We should probably print out a warning message during the first 
load_share_class() call after the agent attachment, saying that CDS has 
been disabled due to attaching agent.

Also, maybe this check can also be removed?

1247 instanceKlassHandle SystemDictionary::load_shared_class(
1248                  Symbol* class_name, Handle class_loader, TRAPS) {
1249   if (!JvmtiExport::should_post_class_file_load_hook()) { 
<<<<<<<<<<<<<<<

Since the same check is already done in the "inner" load_shared_class() 
method.

Thanks
- Ioi


On 7/8/16 11:18 AM, Jiangli Zhou wrote:
> Hi Karen,
>
> Thank you for the feedback. I think Ioi and your comments regarding the dynamically attached agent is very reasonable. I will rework the changes.
>
> Thanks,
> Jiangli
>
>> On Jul 8, 2016, at 7:55 AM, Karen Kinnear <karen.kinnear at oracle.com> wrote:
>>
>> Jiangli,
>>
>> Thank you so much for picking up this bug fix, I very much appreciate that.
>>
>> I agree with not quitting the vm on agent attach.
>> I actually think we also want the agent attach to succeed - see below.
>>> On Jul 7, 2016, at 9:56 PM, Ioi Lam <ioi.lam at oracle.com> 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.
>> I agree with this part. I was expecting the else to return a null instanceklasshandle as if the shared class was not found,
>> but no forcing of using the shared spaces if there is an agent.
>>> 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
>> Yes, here it makes sense to do the fail_continue such that RequireSharedSpaces will prevent starting the JVM.
>>> If the agent is attached later, after the VM has started, I think we should not quit the VM.
>> Totally agree - 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();
>>> }
>>> …
>>> }
>>>
>> I was expecting the vm to continue running, letting the agent attach (customers really want to use these).
>> Jiangli - if I read your code correctly, that is what you do if UseSharedSpaces but not if RequireSharedSpaces.
>> I would propose that you do the same behavior for both cases once we are up and running - i.e. get rid of the
>> two blocks Ioi suggested removing and just fall through as if the file was not found in the archive regardless of RequireSharedSpaces.
>>> Thanks
>>> - Ioi
>> Jiangli:
>>
>> Minor code review comments:
>>
>> metaspace.cpp line 3181: CFHL-> CFLH
>>
>> metaspace.cpp:
>>    I appreciated your moving the if (UseSharedSpaces) into the #INCLUDE_CDS.
>>    I think it would make sense to do a bit more of that - e.g. cds_address appears to only be
>>    use locally, so it also could be inside the #if INCLUDE_CDS.
>>    Would it make sense to have the entire if (DumpSharedSpaces) etc. all be inside a single #if INCLUDE_CDS?
>>
>> thanks,
>> Karen
>>
>>>
>>>
>>> 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
>>>>
>>>>