[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR(S) 8212200 assert when shared java.lang.Object is redefined by JVMTI agent
From:       Lois Foltan <lois.foltan () oracle ! com>
Date:       2018-10-23 14:34:14
Message-ID: 0a924689-c13c-db97-8439-d7c8376363f3 () oracle ! com
[Download RAW message or body]

On 10/23/2018 12:09 AM, Ioi Lam wrote:

>
>
> On 10/22/18 10:25 AM, Lois Foltan wrote:
>> 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.
> Do you mean this?
>
> bool SystemDictionary::is_well_known_klass(Symbol* class_name) {
>    int sid;
>    for (int i = 0; (sid = wk_init_info[i]) != 0; i++) {
>        Symbol* symbol = vmSymbols::symbol_at((vmSymbols::SID)sid);
>        if (class_name == symbol) {
>            return true;
>        }
>    }
>    return false;
> }
>
Yes, I think that's better.

>> - line#1981 - can you use class_name->fast_compare(symbol) for the 
>> equality check?
>>
> The comments around fast_compare says it's for vtable sorting only.
Right, David pointed that out to me as well.   Comment retracted.

>
>> memory/heapShared.cpp
>> - line#422 could be a ResourceMark rm(THREAD);
>>
> Fixed.
Great, thanks!
Lois

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

[prev in list] [next in list] [prev in thread] [next in thread]