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

Jiangli Zhou jiangli.zhou at oracle.com
Mon Oct 22 17:25:21 UTC 2018

Hi Ioi,

Looks good. Please see comments below.

4254     assert(JvmtiEnvBase::get_phase() <= JVMTI_PHASE_PRIMORDIAL, 4255            "Field offsets of well-known classes must be computed in JVMTI_PHASE_PRIMORDIAL or before");

Maybe it is worth adding a function (for example, is_primordial()) in jvmtiEnvBase.hpp, so we can avoid using JVMTI details here?

I'm not too sure if the assert is necessary. Why well known-classes' field offsets must be computed in JVMTI_PHASE_PRIMORDIAL 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.

2108   if (UseSharedSpaces) { 2109     assert(JvmtiEnvBase::get_phase() <= JVMTI_PHASE_PRIMORDIAL, 2110            "All well known classes must be resolved in JVMTI_PHASE_PRIMORDIAL or before"); 2111     for (int i = FIRST_WKID; i < last; i++) { 2112       InstanceKlass* k = _well_known_klasses[i]; 2113       assert(k->is_shared(), "must not be replaced by JVMTI class file load hook"); 2114     }

Please include the above block under #ifdef ASSERT.

-// preloading is actually performed by resolve_preloaded_classes. +// class resolution is actually performed by resolve_well_known_classes.

The original comment is more accurate. Maybe use 'eager loading' if you want to avoid the confusion between 'preloading' and CDS's term?

The test looks good. Thanks for filling the gap in this area!

Thanks, Jiangli

On 10/21/18 10:49 PM, Ioi Lam wrote:

Hi David,

Thanks for the review. Updated webrev: http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04/ http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v04.delta/

More comments below:

On 10/21/18 6:57 PM, David Holmes wrote: Hi Ioi, Generally seems okay. On 22/10/2018 11:15 AM, Ioi Lam wrote: Re-sending to the correct mailing lists. Please disregard the other email.

http://cr.openjdk.java.net/~iklam/jdk12/8212200-cds-jvmti-clfh-critical-classes.v03/ https://bugs.openjdk.java.net/browse/JDK-8212200 Hi, CDS has various built-in assumptions that classes loaded by SystemDictionary::resolvewellknownclasses must not be replaced by JVMTI ClassFileLoadHook during run time, including - field offsets computed in JavaClasses::computeoffsets - the layout of the strings objects in the shared strings table The "well-known" classes can be replaced by ClassFileLoadHook only when JvmtiExport::earlyclasshookenv() is true. Therefore, the fix is to disable CDS under this condition. I'm a little unclear why we have to iterate JvmtiEnv list when this has to be checked during JVMTIPHASEPRIMORDIAL? I think you are asking about this new function? I don't like the name "earlyclasshookenv()". Maybe I should change it to "hasearlyclasshookenv()"? bool JvmtiExport::earlyclasshookenv() { JvmtiEnvIterator it; for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) { if (env->earlyclasshookenv()) { return true; } } return false; } This function matches condition in the existing code that would call into ClassFileLoadHook: class JvmtiClassFileLoadHookPoster {  ... void postallenvs() { JvmtiEnvIterator it; for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) { .. posttoenv(env, true); } } ... void posttoenv(JvmtiEnv* env, bool cachingneeded) { if (env->phase() == JVMTIPHASEPRIMORDIAL && !env->earlyclasshookenv()) { return; } postallenvs() is called just before a class is about to be loaded in the JVM. So if any env->earlyclasshookenv() returns true, there's a chance that it will replace a well-known class.So, as a preventive measure, CDS will be disabled. I have added a few test cases to try to replace shared classes, including well-known classes and other classes. See comments in ReplaceCriticalClasses.java for details. As a clean up, I also renamed all use of "preloaded" in the source code to "well-known". They refer to the same thing in HotSpot, so there's no need to use 2 terms. Also, The word "preloaded" is ambiguous -- it's unclear when "preloading" happens, and could be confused with what CDS does during archive dump time. A few specific comments: src/hotspot/share/classfile/systemDictionary.cpp + bool SystemDictionary::iswellknownklass(Symbol* classname) { +   for (int i = 0; ; i++) { +     int sid = wkinitinfo[i]; +     if (sid == 0) { +       break; +     } I take it a zero value is a guaranteed end-of-list sentinel? Yes. The array is defined just a few lines above: static const short wkinitinfo[] = { _#define WKKLASSINITINFO(name, symbol) _ ((short)vmSymbols::VMSYMBOLENUMNAME(symbol)), WKKLASSESDO(WKKLASSINITINFO) #undef WKKLASSINITINFO 0 }; Also, class vmSymbols: AllStatic { enum SID { NOSID = 0, .... + for (int i=FIRSTWKID; i<last; i++) {_ _Style nit: need spaces around = and <_ _Fixed._ _---_ _test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java_ _New file should have current copyright year only._ _Fixed._  _31  * @comment CDS should not be disabled -- these critical classes_ _will be replaced because JvmtiExport::earlyclasshookenv() is true._ _Comment seems contradictory: if we replace critical classes then CDS_ _should be disabled right??_ _Fixed._ _I expected to see tests that checked for:_ _"CDS is disabled because early JVMTI ClassFileLoadHook is in use."_ _in the output. ??_ _ It would have been easy if jtreg lets you check the output of @run easily. Instead, your innocent suggestion has turned into 150+ lines of new code :-( Maybe "let's write all shell tests in Java" isn't such a great idea after all. Now the test checks that whether CDS is indeed disabled, whether the affected class is loaded from the shared archive, etc. Thanks - Ioi Thanks, David

 > In early e-mails Jiangli wrote:  >  > We should consider including more classes from the default classlist  > in the test. Archived classes loaded during both 'early' phase and after  > should be tested. Done.  > For future optimizations, we might want to prevent loading additional  > shared classes if any of the archived system classes is changed. What's the benefit of doing this? Today we already stop loading a shared class if its super class was not loaded from the archive. Thanks - Ioi

More information about the hotspot-runtime-dev mailing list