CDS and JVM-TI agent questions and crash

[Jiangli Zhou]

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 --- */