RFR(S) 8212200 assert when shared java.lang.Object is redefined by JVMTI agent
Ioi Lam
ioi.lam at oracle.com
Mon Oct 22 17:56:49 UTC 2018
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;
}
> - 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) ......
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