Skip to content

Commit

Permalink
8222005: ClassRedefinition crashes with: guarantee(false) failed: OLD…
Browse files Browse the repository at this point in the history 
… and/or OBSOLETE method(s) found

Remove optimizations from class redefinition that cause the guarantee hit

Reviewed-by: mbaesken
Backport-of: 2ff9f53
  • Loading branch information
@TheRealMDoerr
TheRealMDoerr committed Jun 28, 2024
1 parent 2a5a4d7 commit 468d6a7
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 66 deletions.
23 changes: 13 additions & 10 deletions src/hotspot/share/oops/cpCache.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -562,15 +562,14 @@ oop ConstantPoolCacheEntry::method_type_if_resolved(const constantPoolHandle& cp
#if INCLUDE_JVMTI

void log_adjust(const char* entry_type, Method* old_method, Method* new_method, bool* trace_name_printed) {
if (log_is_enabled(Info, redefine, class, update)) {
ResourceMark rm;
if (!(*trace_name_printed)) {
log_info(redefine, class, update)("adjust: name=%s", old_method->method_holder()->external_name());
*trace_name_printed = true;
}
log_debug(redefine, class, update, constantpool)
("cpc %s entry update: %s(%s)", entry_type, new_method->name()->as_C_string(), new_method->signature()->as_C_string());
ResourceMark rm;

if (!(*trace_name_printed)) {
log_info(redefine, class, update)("adjust: name=%s", old_method->method_holder()->external_name());
*trace_name_printed = true;
}
log_trace(redefine, class, update, constantpool)
("cpc %s entry update: %s", entry_type, new_method->external_name());
}

// RedefineClasses() API support:
Expand Down Expand Up @@ -817,9 +816,13 @@ void ConstantPoolCache::adjust_method_entries(bool * trace_name_printed) {

// the constant pool cache should never contain old or obsolete methods
bool ConstantPoolCache::check_no_old_or_obsolete_entries() {
ResourceMark rm;
for (int i = 1; i < length(); i++) {
if (entry_at(i)->get_interesting_method_entry() != NULL &&
!entry_at(i)->check_no_old_or_obsolete_entries()) {
Method* m = entry_at(i)->get_interesting_method_entry();
if (m != NULL && !entry_at(i)->check_no_old_or_obsolete_entries()) {
log_trace(redefine, class, update, constantpool)
("cpcache check found old method entry: class: %s, old: %d, obsolete: %d, method: %s",
constant_pool()->pool_holder()->external_name(), m->is_old(), m->is_obsolete(), m->external_name());
return false;
}
}
Expand Down
47 changes: 27 additions & 20 deletions src/hotspot/share/oops/klassVtable.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -966,6 +966,8 @@ bool klassVtable::adjust_default_method(int vtable_index, Method* old_method, Me
// search the vtable for uses of either obsolete or EMCP methods
void klassVtable::adjust_method_entries(bool * trace_name_printed) {
int prn_enabled = 0;
ResourceMark rm;

for (int index = 0; index < length(); index++) {
Method* old_method = unchecked_method_at(index);
if (old_method == NULL || !old_method->is_old()) {
Expand All @@ -983,27 +985,29 @@ void klassVtable::adjust_method_entries(bool * trace_name_printed) {
updated_default = adjust_default_method(index, old_method, new_method);
}

if (log_is_enabled(Info, redefine, class, update)) {
ResourceMark rm;
if (!(*trace_name_printed)) {
log_info(redefine, class, update)
("adjust: klassname=%s for methods from name=%s",
_klass->external_name(), old_method->method_holder()->external_name());
*trace_name_printed = true;
}
log_debug(redefine, class, update, vtables)
("vtable method update: %s(%s), updated default = %s",
new_method->name()->as_C_string(), new_method->signature()->as_C_string(), updated_default ? "true" : "false");
if (!(*trace_name_printed)) {
log_info(redefine, class, update)
("adjust: klassname=%s for methods from name=%s",
_klass->external_name(), old_method->method_holder()->external_name());
*trace_name_printed = true;
}
log_trace(redefine, class, update, vtables)
("vtable method update: class: %s method: %s, updated default = %s",
_klass->external_name(), new_method->external_name(), updated_default ? "true" : "false");
}
}

// a vtable should never contain old or obsolete methods
bool klassVtable::check_no_old_or_obsolete_entries() {
ResourceMark rm;

for (int i = 0; i < length(); i++) {
Method* m = unchecked_method_at(i);
if (m != NULL &&
(NOT_PRODUCT(!m->is_valid() ||) m->is_old() || m->is_obsolete())) {
log_trace(redefine, class, update, vtables)
("vtable check found old method entry: class: %s old: %d obsolete: %d, method: %s",
_klass->external_name(), m->is_old(), m->is_obsolete(), m->external_name());
return false;
}
}
Expand Down Expand Up @@ -1292,8 +1296,9 @@ void klassItable::initialize_itable_for_interface(int method_table_offset, Klass
#if INCLUDE_JVMTI
// search the itable for uses of either obsolete or EMCP methods
void klassItable::adjust_method_entries(bool * trace_name_printed) {

ResourceMark rm;
itableMethodEntry* ime = method_entry(0);

for (int i = 0; i < _size_method_table; i++, ime++) {
Method* old_method = ime->method();
if (old_method == NULL || !old_method->is_old()) {
Expand All @@ -1303,25 +1308,27 @@ void klassItable::adjust_method_entries(bool * trace_name_printed) {
Method* new_method = old_method->get_new_method();
ime->initialize(new_method);

if (log_is_enabled(Info, redefine, class, update)) {
ResourceMark rm;
if (!(*trace_name_printed)) {
log_info(redefine, class, update)("adjust: name=%s", old_method->method_holder()->external_name());
*trace_name_printed = true;
}
log_trace(redefine, class, update, itables)
("itable method update: %s(%s)", new_method->name()->as_C_string(), new_method->signature()->as_C_string());
if (!(*trace_name_printed)) {
log_info(redefine, class, update)("adjust: name=%s", old_method->method_holder()->external_name());
*trace_name_printed = true;
}
log_trace(redefine, class, update, itables)
("itable method update: class: %s method: %s", _klass->external_name(), new_method->external_name());
}
}

// an itable should never contain old or obsolete methods
bool klassItable::check_no_old_or_obsolete_entries() {
ResourceMark rm;
itableMethodEntry* ime = method_entry(0);

for (int i = 0; i < _size_method_table; i++) {
Method* m = ime->method();
if (m != NULL &&
(NOT_PRODUCT(!m->is_valid() ||) m->is_old() || m->is_obsolete())) {
log_trace(redefine, class, update, itables)
("itable check found old method entry: class: %s old: %d obsolete: %d, method: %s",
_klass->external_name(), m->is_old(), m->is_obsolete(), m->external_name());
return false;
}
ime++;
Expand Down
59 changes: 23 additions & 36 deletions src/hotspot/share/prims/jvmtiRedefineClasses.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,9 @@ Method** VM_RedefineClasses::_added_methods = NULL;
int VM_RedefineClasses::_matching_methods_length = 0;
int VM_RedefineClasses::_deleted_methods_length = 0;
int VM_RedefineClasses::_added_methods_length = 0;

// This flag is global as the constructor does not reset it:
bool VM_RedefineClasses::_has_redefined_Object = false;
bool VM_RedefineClasses::_has_null_class_loader = false;


VM_RedefineClasses::VM_RedefineClasses(jint class_count,
Expand All @@ -75,8 +76,6 @@ VM_RedefineClasses::VM_RedefineClasses(jint class_count,
_any_class_has_resolved_methods = false;
_res = JVMTI_ERROR_NONE;
_the_class = NULL;
_has_redefined_Object = false;
_has_null_class_loader = false;
}

static inline InstanceKlass* get_ik(jclass def) {
Expand Down Expand Up @@ -3439,7 +3438,10 @@ void VM_RedefineClasses::AdjustAndCleanMetadata::do_klass(Klass* k) {
bool trace_name_printed = false;

// If the class being redefined is java.lang.Object, we need to fix all
// array class vtables also
// array class vtables also. The _has_redefined_Object flag is global.
// Once the java.lang.Object has been redefined (by the current or one
// of the previous VM_RedefineClasses operations) we have to always
// adjust method entries for array classes.
if (k->is_array_klass() && _has_redefined_Object) {
k->vtable().adjust_method_entries(&trace_name_printed);

Expand All @@ -3457,22 +3459,6 @@ void VM_RedefineClasses::AdjustAndCleanMetadata::do_klass(Klass* k) {
}
}

// HotSpot specific optimization! HotSpot does not currently
// support delegation from the bootstrap class loader to a
// user-defined class loader. This means that if the bootstrap
// class loader is the initiating class loader, then it will also
// be the defining class loader. This also means that classes
// loaded by the bootstrap class loader cannot refer to classes
// loaded by a user-defined class loader. Note: a user-defined
// class loader can delegate to the bootstrap class loader.
//
// If the current class being redefined has a user-defined class
// loader as its defining class loader, then we can skip all
// classes loaded by the bootstrap class loader.
if (!_has_null_class_loader && ik->class_loader() == NULL) {
return;
}

// Adjust all vtables, default methods and itables, to clean out old methods.
ResourceMark rm(_thread);
if (ik->vtable_length() > 0) {
Expand All @@ -3491,22 +3477,24 @@ void VM_RedefineClasses::AdjustAndCleanMetadata::do_klass(Klass* k) {
// constant pool cache holds the Method*s for non-virtual
// methods and for virtual, final methods.
//
// Special case: if the current class being redefined, then new_cp
// has already been attached to the_class and old_cp has already
// been added as a previous version. The new_cp doesn't have any
// cached references to old methods so it doesn't need to be
// updated. We can simply start with the previous version(s) in
// that case.
constantPoolHandle other_cp;
// Special case: if the current class is being redefined by the current
// VM_RedefineClasses operation, then new_cp has already been attached
// to the_class and old_cp has already been added as a previous version.
// The new_cp doesn't have any cached references to old methods so it
// doesn't need to be updated and we could optimize by skipping it.
// However, the current class can be marked as being redefined by another
// VM_RedefineClasses operation which has already executed its doit_prologue
// and needs cpcache method entries adjusted. For simplicity, the cpcache
// update is done unconditionally. It should result in doing nothing for
// classes being redefined by the current VM_RedefineClasses operation.
// Method entries in the previous version(s) are adjusted as well.
ConstantPoolCache* cp_cache;

if (!ik->is_being_redefined()) {
// this klass' constant pool cache may need adjustment
other_cp = constantPoolHandle(ik->constants());
cp_cache = other_cp->cache();
if (cp_cache != NULL) {
cp_cache->adjust_method_entries(&trace_name_printed);
}
// this klass' constant pool cache may need adjustment
ConstantPool* other_cp = ik->constants();
cp_cache = other_cp->cache();
if (cp_cache != NULL) {
cp_cache->adjust_method_entries(&trace_name_printed);
}

// the previous versions' constant pool caches may need adjustment
Expand Down Expand Up @@ -3947,9 +3935,8 @@ void VM_RedefineClasses::redefine_single_class(jclass the_jclass,

InstanceKlass* the_class = get_ik(the_jclass);

// Set some flags to control and optimize adjusting method entries
// Set a flag to control and optimize adjusting method entries
_has_redefined_Object |= the_class == SystemDictionary::Object_klass();
_has_null_class_loader |= the_class->class_loader() == NULL;

// Remove all breakpoints in methods of this class
JvmtiBreakpoints& jvmti_breakpoints = JvmtiCurrentBreakpoints::get_jvmti_breakpoints();
Expand Down

1 comment on commit 468d6a7

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Issues

Please sign in to comment.