RFR(S) 8212200 assert when shared java.lang.Object is redefined by JVMTI agent
Jiangli Zhou
jiangli.zhou at oracle.com
Mon Oct 22 22:06:06 UTC 2018
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.
>
>
>> - 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 serviceability-dev
mailing list