Sounds good. Thanks for trying that.
Thanks,
Jiangli
> On Oct 22, 2018, at 6:43 PM, Ioi Lam <ioi....@oracle.com> wrote:
>
>
>
>> On 10/22/18 3:06 PM, Jiangli Zhou wrote:
>>> On 10/22/18 10:56 AM, Ioi Lam wrote:
>>>
>>>
>>>
>>>> On 10/22/18 10:25 AM, Jiangli Zhou wrote:
>>>> Hi Ioi,
>>>>
>>>> Looks good. Please see comments below.
>>>>
>>>> - src/hotspot/share/classfile/javaClasses.cpp
>>>>
>>>> 4254 assert(JvmtiEnvBase::get_phase() <= JVMTI_PHASE_PRIMORDIAL,
>>>> 4255 "Field offsets of well-known classes must be computed in
>>>> JVMTI_PHASE_PRIMORDIAL or before");
>>>>
>>>> Maybe it is worth adding a function (for example, is_primordial()) in
>>>> jvmtiEnvBase.hpp, so we can avoid using JVMTI details here?
>>>>
>>> I'll add JvmtiExport::is_early_phase(), since the phase can be either
>>> JVMTI_PHASE_ONLOAD or JVMTI_PHASE_PRIMORDIAL.
>>>
>>>> I'm not too sure if the assert is necessary. Why well known-classes' field
>>>> offsets must be computed in JVMTI_PHASE_PRIMORDIAL or before? Currently
>>>> they are, however that's because they are used early by the VM. That
>>>> doesn't directly relate to any JVMTI phase necessarily. With the assert,
>>>> we are explicitly making a connection to the JVMTI phases. To me, that
>>>> doesn't seem to be necessary.
>>>>
>>>
>>> JavaClasses::compute_offsets uses many different classes. I would need to
>>> check that each of them were in the well-known class list, so that we know
>>> the offsets stored in the CDS archive are still valid. However, I couldn't
>>> find a single place to make such a check, and it would be much easier to
>>> add the above assert, which means any shared class used inside
>>> compute_offsets cannot be replaced by JVMTI.
>>>
>>> How about this:
>>>
>>> void JavaClasses::compute_offsets() {
>>> if (UseSharedSpaces) {
>>> assert(JvmtiEnvBase::is_early_phase() &&
>>> !JvmtiExport::has_early_class_hook_env(),
>>> "JavaClasses::compute_offsets() must be called in early JVMTI
>>> phase.");
>>> // None of the classes used by the rest of this function can be
>>> replaced by
>>> // JMVTI ClassFileLoadHook.
>>> // We are safe to use the archived offsets, which have already been
>>> restored
>>> // by JavaClasses::serialize_offsets, without computing the offsets
>>> again.
>>> return;
>>> }
>>
>> You could do assert(k->is_shared() || !UseSharedSpaces) in the
>> DO_SERIALIZE_OFFSETS macro to make sure the expected shared classes are used
>> when UseSharedSpaces is enabled during loading the archived field offsets.
>> The extra !UseSharedSpaces here is because serialize_offsets() function is
>> used for both dump time and runtime. It could be removed because the
>> 'is_shared' flag is probably already set when we writing out data at dump
>> time, but please double check that.
>>
>> #define DO_SERIALIZE_OFFSETS(k) k::serialize_offsets(soc);
>>
>> If JvmtiExport::early_class_hook_env is requested by a JVMTI agent,
>> UseSharedSpaces should already be disabled at runtime, otherwise a shared
>> class should be used in this case.
>>
> Hi Jiangli,
>
> Thanks for the suggestion.
>
> I tried this, but The "k" parameter to this macro is not an InstanceKlass.
> Instead, it's these guys
>
> #define BASIC_JAVA_CLASSES_DO_PART2(f) \
> f(java_lang_System) \
> f(java_lang_ClassLoader) \
> f(java_lang_Throwable) \
> f(java_lang_Thread) \
> f(java_lang_ThreadGroup) \
> f(java_lang_AssertionStatusDirectives) \
> f(java_lang_ref_SoftReference) \
> f(java_lang_invoke_MethodHandle) \
> f(java_lang_invoke_DirectMethodHandle) \
> f(java_lang_invoke_MemberName) \
> f(java_lang_invoke_ResolvedMethodName) \
> f(java_lang_invoke_LambdaForm) \
> f(java_lang_invoke_MethodType) \
> f(java_lang_invoke_CallSite) \
> f(java_lang_invoke_MethodHandleNatives_CallSiteContext) \
> f(java_security_AccessControlContext) \
> f(java_lang_reflect_AccessibleObject) \
> f(java_lang_reflect_Method) \
> f(java_lang_reflect_Constructor) \
> f(java_lang_reflect_Field) \
> f(java_nio_Buffer) \
> f(reflect_ConstantPool) \
> f(reflect_UnsafeStaticFieldAccessorImpl) \
> f(java_lang_reflect_Parameter) \
> f(java_lang_Module) \
> f(java_lang_StackTraceElement) \
> f(java_lang_StackFrameInfo) \
> f(java_lang_LiveStackFrameInfo) \
> f(java_util_concurrent_locks_AbstractOwnableSynchronizer) \
> //end
>
>
> I can somehow force this to work by reworking all macros related to
> BASIC_JAVA_CLASSES_DO, but that doesn't seem to be worth it. I think my
> current assert is stronger than necessary so it's safe and simple.
>
> Thanks
> - Ioi
>
>
>>>
>>>
>>>> - src/hotspot/share/classfile/systemDictionary.cpp
>>>>
>>>> 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 }
>>>>
>>>> Please include the above block under #ifdef ASSERT.
>>>>
>>> OK
>>>
>>>> -// preloading is actually performed by resolve_preloaded_classes.
>>>> +// class resolution is actually performed by resolve_well_known_classes.
>>>>
>>>> The original comment is more accurate. Maybe use 'eager loading' if you
>>>> want to avoid the confusion between 'preloading' and CDS's term?
>>>>
>>> I can see that "class resolution" could have different meanings, although
>>> resolve_well_known_classes does call SystemDictionary::resolve_or_fail, not
>>> any the SystemDictionary::load* functions. So using the word "resolve"
>>> would be more appropriate.
>>>
>>> How about changing the comments to the following to avoid ambiguity.
>>>
>>> #define WK_KLASS_ENUM_NAME(kname) kname##_knum
>>>
>>> // Certain classes, such as java.lang.Object and java.lang.String,
>>> // are "well-known", in the sense that no class loader is allowed
>>> // to provide a different definition.
>>> //
>>> // Each well-known class has a short klass name (like object_klass),
>>> // and a vmSymbol name (like java_lang_Object).
>>> //
>>> // The order of these definitions is significant: the classes are
>>> // resolved during early VM start-up by resolve_well_known_classes
>>> // in this order. Changing the order may require careful restructuring
>>> // of the VM start-up sequence.
>>> //
>>> #define WK_KLASSES_DO(do_klass) ......
>>
>> Looks ok.
>>
>> Thanks,
>> Jiangli
>>>
>>>
>>> Thanks
>>> - Ioi
>>>> The test looks good. Thanks for filling the gap in this area!
>>>>
>>>> Thanks,
>>>> Jiangli
>>>>
>>>>> On 10/21/18 10: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::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
>>>>>>>
>>>>>
>>>>
>>>
>>
>
