CDS and JVM-TI agent questions and crash

[Jiangli Zhou]

I logged issue in JDK-8212200, 
https://bugs.openjdk.java.net/browse/JDK-8212200.

Thanks,

Jiangli


On 10/15/18 11:50 AM, Jiangli Zhou wrote:
> Hi Volker,
>
> Thanks for taking this! I was thinking about this issue also. We 
> probably should disable sharing by turning off UseSharedSpaces (in 
> KlassFactory::check_shared_class_file_load_hook) if a shared class 
> loaded by boot loader is redefined. Archived sub-classes are 
> invalidated and not used if the super class is different. When shared 
> java.lang.Object is redefined, all archived classes are invalidated.
>
> Michael, please see more comments below.
>
> On 10/15/18 11:07 AM, Volker Simonis wrote:
>> Just a quick analysis (will look into a fix tomorrow).
>>
>> - When saving a class to the CDS archive, the VM calls
>> ConstantPool::remove_unshareable_info() on its constant pool:
>>
>> void ConstantPool::remove_unshareable_info() {
>> ...
>>    // Shared ConstantPools are in the RO region, so the _flags cannot
>> be modified.
>>    // The _on_stack flag is used to prevent ConstantPools from
>> deallocation during
>>    // class redefinition. Since shared ConstantPools cannot be
>> deallocated anyway,
>>    // we always set _on_stack to true to avoid having to change _flags
>> during runtime.
>>    _flags |= (_on_stack | _is_shared);
>>
>> This calls sets the '_on_stack' flag on the constant pool for the
>> reasons described in the comment.
>>
>> - When a class is being loaded from the CDS archive, the VM calls
>> ConstantPool::restore_unshareable_info(TRAPS) on its constant pool:
>>
>> // CDS support. Create a new resolved_references array.
>> void ConstantPool::restore_unshareable_info(TRAPS) {
>>    assert(is_constantPool(), "ensure C++ vtable is restored");
>>    assert(on_stack(), "should always be set for shared constant pools");
>>    assert(is_shared(), "should always be set for shared constant 
>> pools");
>>    assert(_cache != NULL, "constant pool _cache should not be NULL");
>>
>> This calls asserts for the '_on_stack' flag being set.
>>
>> - When loading a class from the CSD archive
>> withSystemDictionary::load_shared_class(),
>> ConstantPool::restore_unshareable_info(TRAPS) is actually called from
>> InstanceKlass::restore_unshareable_info(TRAPS) after the instanceKlass
>> was loaded:
>>
>> InstanceKlass* SystemDictionary::load_shared_class(InstanceKlass* ik,
>>                                                     Handle class_loader,
>>                                                     Handle
>> protection_domain, TRAPS) {
>> ...
>>      InstanceKlass* new_ik = 
>> KlassFactory::check_shared_class_file_load_hook(
>>          ik, class_name, class_loader, protection_domain, CHECK_NULL);
>>      if (new_ik != NULL) {
>>        // The class is changed by CFLH. Return the new class. The 
>> shared class is
>>        // not used.
>>        return new_ik;
>>      }
>>
>>      // Adjust methods to recover missing data.  They need addresses for
>>      // interpreter entry points and their default native method address
>>      // must be reset.
>>
>>      // Updating methods must be done under a lock so multiple
>>      // threads don't update these in parallel
>>      //
>>      // Shared classes are all currently loaded by either the 
>> bootstrap or
>>      // internal parallel class loaders, so this will never cause a 
>> deadlock
>>      // on a custom class loader lock.
>>
>>      ClassLoaderData* loader_data =
>> ClassLoaderData::class_loader_data(class_loader());
>>      {
>>        HandleMark hm(THREAD);
>>        Handle lockObject = compute_loader_lock_object(class_loader, 
>> THREAD);
>>        check_loader_lock_contention(lockObject, THREAD);
>>        ObjectLocker ol(lockObject, THREAD, true);
>>        // prohibited package check assumes all classes loaded from 
>> archive call
>>        // restore_unshareable_info which calls ik->set_package()
>>        ik->restore_unshareable_info(loader_data, protection_domain, 
>> CHECK_NULL);
>>      }
>>
>> - However, when a class loaded from the CDS archive is being redefined
>> there's a shortcut in SystemDictionary::load_shared_class() which
>> instantly returns the new, redefined class, without calling
>> InstanceKlass::restore_unshareable_info() on it:
>>
>>      InstanceKlass* new_ik = 
>> KlassFactory::check_shared_class_file_load_hook(
>>          ik, class_name, class_loader, protection_domain, CHECK_NULL);
>>      if (new_ik != NULL) {
>>        // The class is changed by CFLH. Return the new class. The 
>> shared class is
>>        // not used.
>>        return new_ik;
>>      }
>>
>> - For the well known class java.lang.Object the procedure is a little
>> different. It gets loaded early in
>> SystemDictionary::resolve_preloaded_classes() and for some reason we
>> directly call ConstantPool::restore_unshareable_info() right on its
>> constant pool after it was loaded:
>>
>> #if INCLUDE_CDS
>>    if (UseSharedSpaces) {
>> resolve_wk_klasses_through(WK_KLASS_ENUM_NAME(Object_klass), scan, 
>> CHECK);
>> ...
>>      // Initialize the constant pool for the Object_class
>> Object_klass()->constants()->restore_unshareable_info(CHECK);
>>
>> - This fails if java.lang.Object was redefined, because in that case
>> its constant pool won't have its '_on_stack' flag set to 'true'.
>>
>> - I don't think that this extra call to
>> Object_klass()->constants()->restore_unshareable_info() is required
>> any more, because the call to
>> resolve_wk_klasses_through(WK_KLASS_ENUM_NAME(Object_klass)) already
>> loads java.lang.Object through SystemDictionary::load_shared_class()
>> which will call restore_unshareable_info() on it (if it was not
>> redefined). But I'll have to check this more thoroughly...
>>
>> I'll do that and open a bug for this issue tomorrow.
>>
>> Regards,
>> Volker
>>
>> On Mon, Oct 15, 2018 at 5:32 PM Michael Rasmussen
>> <Michael.Rasmussen at roguewave.com> wrote:
>>> Hi
>>>
>>> I have some questions regarding Class Data Sharing and class file 
>>> transforming agents:
>>>
>>> 1. Is it possible to detect during Agent_OnLoad if CDS is used or not?
>>> Checking for "sharing" in the "java.vm.info" system property, which 
>>> seems to be how the tests are detecting it, is not an option, as 
>>> this property contains bogus information during Agent_OnLoad.
>>>
>>> 2. Should the registration of a ClassFileHook, especially with 
>>> can_generate_all_class_hook_events and 
>>> can_generate_early_class_hook_events capabilities set, disable CDS?
>>>
>>> 3. Is it possible to explicitly disable CDS from a JVM-TI agent 
>>> during Agent_OnLoad?
> Currently, there is no easy to disable CDS from a JVMTI agent that I 
> can think of. We could provide a property for this.
>
> Best regards,
> Jiangli
>>>
>>>
>>> The reason for these questions is that we (JRebel) recently became 
>>> aware of an issue if CDS is used, where transforming some of the 
>>> boot classes causes the JVM to crash.
>>> The simplest example of this is replacing Object's byte in a 
>>> ClassFileLoadHook with a copy of themselves, see inlined example 
>>> agent code below.
>>> I don't know if there are more issues than that, as that's where we 
>>> are currently stuck.
>>>
>>> Running on a 11+28 fastdebug (with classes.jsa file present) it 
>>> produces the following assert:
>>> #  Internal Error (.../src/hotspot/share/oops/constantPool.cpp:325), 
>>> pid=589, tid=590
>>> #  assert(on_stack()) failed: should always be set for shared 
>>> constant pools
>>>
>>> if running with -Xshare:off (or no classes.jsa file), it works 
>>> without any issue.
>>>
>>> Kind regards
>>> Michael Rasmussen
>>>
>>> /* --- snip --- */
>>> #include <string.h>
>>> #include <jni.h>
>>> #include <jvmti.h>
>>>
>>> void JNICALL callback_ClassFileLoadHook(jvmtiEnv *jvmti, JNIEnv* env,
>>>          jclass class_being_redefined, jobject loader, const char* 
>>> name, jobject protection_domain,
>>>          jint class_data_len, const unsigned char* class_data,
>>>          jint* new_class_data_len, unsigned char** new_class_data) {
>>>
>>>          if (strcmp("java/lang/Object", name) == 0) {
>>>                  *new_class_data_len = class_data_len;
>>>                  (*jvmti)->Allocate(jvmti, *new_class_data_len, 
>>> new_class_data);
>>>                  memcpy(*new_class_data, class_data, 
>>> *new_class_data_len);
>>>          }
>>> }
>>>
>>> JNIEXPORT jint JNICALL Agent_OnLoad(JavaVM *vm, char *options, void 
>>> *reserved) {
>>>          jvmtiEnv *jvmti = NULL;
>>>          jint rc = (*vm)->GetEnv(vm, (void **)&jvmti, JVMTI_VERSION_9);
>>>
>>>          jvmtiCapabilities caps;
>>>          memset(&caps, 0, sizeof(caps));
>>>
>>>          caps.can_redefine_classes = 1;
>>>          caps.can_generate_all_class_hook_events = 1;
>>>          caps.can_generate_early_vmstart = 1;
>>>          caps.can_generate_early_class_hook_events = 1;
>>>
>>>          (*jvmti)->AddCapabilities(jvmti, &caps);
>>>
>>>          jvmtiEventCallbacks callbacks;
>>>          memset(&callbacks, 0, sizeof(callbacks));
>>>          callbacks.ClassFileLoadHook = &callback_ClassFileLoadHook;
>>>          (*jvmti)->SetEventCallbacks(jvmti, &callbacks, 
>>> sizeof(callbacks));
>>>          (*jvmti)->SetEventNotificationMode(jvmti, JVMTI_ENABLE, 
>>> JVMTI_EVENT_CLASS_FILE_LOAD_HOOK, NULL);
>>>
>>>          return 0;
>>> }
>>> /* --- snap --- */
>