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

David Holmes david.holmes at oracle.com
Mon Oct 22 01:57:55 UTC 2018

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::resolve_well_known_classes must not be replaced
> by JVMTI ClassFileLoadHook during run time, including
> - field offsets computed in JavaClasses::compute_offsets
> - the layout of the strings objects in the shared strings table
> The "well-known" classes can be replaced by ClassFileLoadHook only
> when JvmtiExport::early_class_hook_env() 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 JVMTI_PHASE_PRIMORDIAL?

> 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:


+ bool SystemDictionary::is_well_known_klass(Symbol* class_name) {
+   for (int i = 0; ; i++) {
+     int sid = wk_init_info[i];
+     if (sid == 0) {
+       break;
+     }

I take it a zero value is a guaranteed end-of-list sentinel?

+     for (int i=FIRST_WKID; i<last; i++) {

Style nit: need spaces around = and <



New file should have current copyright year only.

  31  * @comment CDS should not be disabled -- these critical classes 
will be replaced because JvmtiExport::early_class_hook_env() is true.

Comment seems contradictory: if we replace critical classes then CDS 
should be disabled right??

I expected to see tests that checked for:

"CDS is disabled because early JVMTI ClassFileLoadHook is in use."

in the output. ??


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