RFR(S) 8212200 assert when shared java.lang.Object is redefined by JVMTI agent (original) (raw)

Ioi Lam ioi.lam at oracle.com
Mon Oct 22 17:56:49 UTC 2018

On 10/22/18 10:25 AM, Jiangli Zhou wrote:

Hi Ioi,

Looks good. Please see comments below. - src/hotspot/share/classfile/javaClasses.cpp 4254     assert(JvmtiEnvBase::getphase() <= JVMTIPHASEPRIMORDIAL, 4255            "Field offsets of well-known classes must be computed in JVMTIPHASEPRIMORDIAL or before"); Maybe it is worth adding a function (for example, isprimordial()) in jvmtiEnvBase.hpp, so we can avoid using JVMTI details here? I'll add JvmtiExport::is_early_phase(), since the phase can be either JVMTI_PHASE_ONLOAD or  JVMTI_PHASE_PRIMORDIAL.

I'm not too sure if the assert is necessary. Why well known-classes' field offsets must be computed in JVMTIPHASEPRIMORDIAL or before? Currently they are, however that's because they are used early by the VM. That doesn't directly relate to any JVMTI phase necessarily. With the assert, we are explicitly making a connection to the JVMTI phases. To me, that doesn't seem to be necessary.

JavaClasses::compute_offsets uses many different classes. I would need to check that each of them were in the well-known class list, so that we know the offsets stored in the CDS archive are still valid. However, I couldn't find a single place to make such a check, and it would be much easier to add the above assert, which means any shared class used inside compute_offsets cannot be replaced by JVMTI.

How about this:

void JavaClasses::compute_offsets() {   if (UseSharedSpaces) {     assert(JvmtiEnvBase::is_early_phase() && !JvmtiExport::has_early_class_hook_env(),            "JavaClasses::compute_offsets() must be called in early JVMTI phase.");     // None of the classes used by the rest of this function can be replaced by     // JMVTI ClassFileLoadHook.     // We are safe to use the archived offsets, which have already been restored     // by JavaClasses::serialize_offsets, without computing the offsets again.     return;   }

- src/hotspot/share/classfile/systemDictionary.cpp

2108   if (UseSharedSpaces) { 2109     assert(JvmtiEnvBase::getphase() <= JVMTIPHASEPRIMORDIAL,_ _2110            "All well known classes must be resolved in_ _JVMTIPHASEPRIMORDIAL or before");_ _2111     for (int i = FIRSTWKID; i < last; i++) {_ _2112       InstanceKlass* k = wellknownklasses[i];_ _2113       assert(k->isshared(), "must not be replaced by JVMTI class file load hook"); 2114     } Please include the above block under #ifdef ASSERT. OK

-// preloading is actually performed by resolvepreloadedclasses. +// class resolution is actually performed by resolvewellknownclasses.

The original comment is more accurate. Maybe use 'eager loading' if you want to avoid the confusion between 'preloading' and CDS's term? I can see that "class resolution" could have different meanings, although resolve_well_known_classes does call SystemDictionary::resolve_or_fail, not any the SystemDictionary::load* functions. So using the word "resolve" would be more appropriate.

How about changing the comments to the following to avoid ambiguity.

#define WK_KLASS_ENUM_NAME(kname)    kname##_knum

// Certain classes, such as java.lang.Object and java.lang.String, // are "well-known", in the sense that no class loader is allowed // to provide a different definition. // // Each well-known class has a short klass name (like object_klass), // and a vmSymbol name (like java_lang_Object). // // The order of these definitions is significant: the classes are // resolved during early VM start-up by resolve_well_known_classes // in this order. Changing the order may require careful restructuring // of the VM start-up sequence. // #define WK_KLASSES_DO(do_klass) ......

Thanks

More information about the hotspot-runtime-dev mailing list