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

[Lois Foltan]

On 10/22/2018 1:49 AM, 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/ 
>

Hi Ioi,

Looks good.  A couple of comments:

classfile/systemDictionary.cpp
- line#1975 - My preference is to declare the variable sid outside the 
for statement and compare sid == 0 within the for loop conditional.
- line#1981 - can you use class_name->fast_compare(symbol) for the 
equality check?

memory/heapShared.cpp
- line#422 could be a ResourceMark rm(THREAD);

Thanks,
Lois

>
> 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::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 think you are asking about this new function? I don't like the name 
> "early_class_hook_env()". Maybe I should change it to 
> "has_early_class_hook_env()"?
>
>
> bool JvmtiExport::early_class_hook_env() {
>   JvmtiEnvIterator it;
>   for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) {
>     if (env->early_class_hook_env()) {
>       return true;
>     }
>   }
>   return false;
> }
>
> This function matches condition in the existing code that would call 
> into ClassFileLoadHook:
>
> class JvmtiClassFileLoadHookPoster {
>  ...
>   void post_all_envs() {
>     JvmtiEnvIterator it;
>     for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) {
>         ..
>         post_to_env(env, true);
>     }
>   }
> ...
>   void post_to_env(JvmtiEnv* env, bool caching_needed) {
>     if (env->phase() == JVMTI_PHASE_PRIMORDIAL && 
> !env->early_class_hook_env()) {
>       return;
>     }
>
>
> post_all_envs() is called just before a class is about to be loaded in 
> the JVM. So if *any* env->early_class_hook_env() 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::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?
>>
>
> Yes. The array is defined just a few lines above:
>
> static const short wk_init_info[] = {
>   #define WK_KLASS_INIT_INFO(name, symbol) \
>     ((short)vmSymbols::VM_SYMBOL_ENUM_NAME(symbol)),
>
>   WK_KLASSES_DO(WK_KLASS_INIT_INFO)
>   #undef WK_KLASS_INIT_INFO
>   0
> };
>
> Also,
>
> class vmSymbols: AllStatic {
>   enum SID {
>     NO_SID = 0,
>     ....
>
>
>
>> + for (int i=FIRST_WKID; 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::early_class_hook_env() 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. ??
>>
> <rant>
> 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.
> </rant>
>
> 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
>>>
>