RFR(S) 8212200 assert when shared java.lang.Object is redefined by JVMTI agent
Ioi Lam
ioi.lam at oracle.com
Tue Oct 23 19:36:03 UTC 2018
Hi Harold,
Thanks for the review. I will add the ifdef
Thanks
Ioi
> On Oct 23, 2018, at 12:06 PM, Harold David Seigel <harold.seigel at oracle.com> wrote:
>
> Hi Ioi,
>
> Should this code in SystemDictionary.cpp have an "#ifdef ASSERT" around it?
>
> 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 }
>
> Thanks, Harold
>
>
>> On 10/23/2018 10:34 AM, Lois Foltan wrote:
>>> 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
>>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list