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

David Holmes david.holmes at oracle.com
Tue Oct 23 02:11:19 UTC 2018

Hi Ioi,

Sorry for all the extra work in the test. ;-)

The updates look good to me (and I've seen follow up comments).

Thanks, David

On 22/10/2018 3: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