RFR(S) 8212200 assert when shared java.lang.Object is redefined by JVMTI agent
Ioi Lam
ioi.lam at oracle.com
Tue Oct 23 01:43:22 UTC 2018
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
>>>>>>
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list