Skip to content

8358890: VM option -XX:AllowRedefinitionToAddDeleteMethods should be obsoleted then expired #26232

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/hotspot/share/classfile/javaClasses.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4572,8 +4572,7 @@ oop java_lang_invoke_ResolvedMethodName::find_resolved_method(const methodHandle
NoSafepointVerifier nsv;

if (method->is_old()) {
method = (method->is_deleted()) ? Universe::throw_no_such_method_error() :
method->get_new_method();
method = method->get_new_method();
}

InstanceKlass* holder = method->method_holder();
Expand Down
6 changes: 0 additions & 6 deletions src/hotspot/share/oops/cpCache.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,6 @@ void ConstantPoolCache::adjust_method_entries(bool * trace_name_printed) {
if (old_method == nullptr || !old_method->is_old()) {
continue;
}
assert(!old_method->is_deleted(), "cannot delete these methods");
Method* new_method = old_method->get_new_method();
resolved_indy_entry_at(j)->adjust_method_entry(new_method);
log_adjust("indy", old_method, new_method, trace_name_printed);
Expand All @@ -657,11 +656,6 @@ void ConstantPoolCache::adjust_method_entries(bool * trace_name_printed) {
if (old_method == nullptr || !old_method->is_old()) {
continue; // skip uninteresting entries
}
if (old_method->is_deleted()) {
// clean up entries with deleted methods
method_entry->reset_entry();
continue;
}
Method* new_method = old_method->get_new_method();
method_entry->adjust_method_entry(new_method);
log_adjust("non-indy", old_method, new_method, trace_name_printed);
Expand Down
1 change: 0 additions & 1 deletion src/hotspot/share/oops/instanceKlass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3474,7 +3474,6 @@ void InstanceKlass::adjust_default_methods(bool* trace_name_printed) {
if (old_method == nullptr || !old_method->is_old()) {
continue; // skip uninteresting entries
}
assert(!old_method->is_deleted(), "default methods may not be deleted");
Method* new_method = old_method->get_new_method();
default_methods()->at_put(index, new_method);

Expand Down
3 changes: 0 additions & 3 deletions src/hotspot/share/oops/klassVtable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1022,8 +1022,6 @@ void klassVtable::adjust_method_entries(bool * trace_name_printed) {
if (old_method == nullptr || !old_method->is_old()) {
continue; // skip uninteresting entries
}
assert(!old_method->is_deleted(), "vtable methods may not be deleted");

Method* new_method = old_method->get_new_method();
put_method_at(new_method, index);

Expand Down Expand Up @@ -1383,7 +1381,6 @@ void klassItable::adjust_method_entries(bool * trace_name_printed) {
if (old_method == nullptr || !old_method->is_old()) {
continue; // skip uninteresting entries
}
assert(!old_method->is_deleted(), "itable methods may not be deleted");
Method* new_method = old_method->get_new_method();
ime->initialize(_klass, new_method);

Expand Down
1 change: 0 additions & 1 deletion src/hotspot/share/oops/method.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1224,7 +1224,6 @@ void Method::remove_unshareable_flags() {
// clear all the flags that shouldn't be in the archived version
assert(!is_old(), "must be");
assert(!is_obsolete(), "must be");
assert(!is_deleted(), "must be");

set_is_prefixed_native(false);
set_queued_for_compilation(false);
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/oops/methodFlags.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class MethodFlags {
status(has_jsrs , 1 << 1) \
status(is_old , 1 << 2) /* RedefineClasses() has replaced this method */ \
status(is_obsolete , 1 << 3) /* RedefineClasses() has made method obsolete */ \
status(is_deleted , 1 << 4) /* RedefineClasses() has deleted this method */ \
status(RESERVED , 1 << 4) \
status(is_prefixed_native , 1 << 5) /* JVMTI has prefixed this native method */ \
status(monitor_matching , 1 << 6) /* True if we know that monitorenter/monitorexit bytecodes match */ \
status(queued_for_compilation , 1 << 7) \
Expand Down
135 changes: 20 additions & 115 deletions src/hotspot/share/prims/jvmtiRedefineClasses.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,7 @@ Array<Method*>* VM_RedefineClasses::_old_methods = nullptr;
Array<Method*>* VM_RedefineClasses::_new_methods = nullptr;
Method** VM_RedefineClasses::_matching_old_methods = nullptr;
Method** VM_RedefineClasses::_matching_new_methods = nullptr;
Method** VM_RedefineClasses::_deleted_methods = nullptr;
Method** VM_RedefineClasses::_added_methods = nullptr;
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;
Expand Down Expand Up @@ -926,12 +922,6 @@ static jvmtiError check_permitted_subclasses_attribute(InstanceKlass* the_class,
scratch_class->permitted_subclasses());
}

static bool can_add_or_delete(Method* m) {
// Compatibility mode
return (AllowRedefinitionToAddDeleteMethods &&
(m->is_private() && (m->is_static() || m->is_final())));
}

jvmtiError VM_RedefineClasses::compare_and_normalize_class_versions(
InstanceKlass* the_class,
InstanceKlass* scratch_class) {
Expand Down Expand Up @@ -1184,51 +1174,18 @@ jvmtiError VM_RedefineClasses::compare_and_normalize_class_versions(
++ni;
break;
case added:
// method added, see if it is OK
if (!can_add_or_delete(k_new_method)) {
log_info(redefine, class, normalize)
("redefined class %s methods error: added method: %s [%d]",
the_class->external_name(), k_new_method->name_and_sig_as_C_string(), ni);
return JVMTI_ERROR_UNSUPPORTED_REDEFINITION_METHOD_ADDED;
}
{
u2 num = the_class->next_method_idnum();
if (num == ConstMethod::UNSET_IDNUM) {
// cannot add any more methods
log_info(redefine, class, normalize)
("redefined class %s methods error: can't create ID for new method %s [%d]",
the_class->external_name(), k_new_method->name_and_sig_as_C_string(), ni);
return JVMTI_ERROR_UNSUPPORTED_REDEFINITION_METHOD_ADDED;
}
u2 new_num = k_new_method->method_idnum();
Method* idnum_owner = scratch_class->method_with_idnum(num);
if (idnum_owner != nullptr) {
// There is already a method assigned this idnum -- switch them
// Take current and original idnum from the new_method
idnum_owner->set_method_idnum(new_num);
idnum_owner->set_orig_method_idnum(k_new_method->orig_method_idnum());
}
k_new_method->set_method_idnum(num);
k_new_method->set_orig_method_idnum(num);
if (thread->has_pending_exception()) {
return JVMTI_ERROR_OUT_OF_MEMORY;
}
}
log_trace(redefine, class, normalize)
("Method added: new: %s [%d]", k_new_method->name_and_sig_as_C_string(), ni);
++ni; // advance to next new method
// method added, report the error
log_info(redefine, class, normalize)
("redefined class %s methods error: added method: %s [%d]",
the_class->external_name(), k_new_method->name_and_sig_as_C_string(), ni);
return JVMTI_ERROR_UNSUPPORTED_REDEFINITION_METHOD_ADDED;
break;
case deleted:
// method deleted, see if it is OK
if (!can_add_or_delete(k_old_method)) {
log_info(redefine, class, normalize)
("redefined class %s methods error: deleted method %s [%d]",
the_class->external_name(), k_old_method->name_and_sig_as_C_string(), oi);
return JVMTI_ERROR_UNSUPPORTED_REDEFINITION_METHOD_DELETED;
}
log_trace(redefine, class, normalize)
("Method deleted: old: %s [%d]", k_old_method->name_and_sig_as_C_string(), oi);
++oi; // advance to next old method
// method deleted, report the error
log_info(redefine, class, normalize)
("redefined class %s methods error: deleted method %s [%d]",
the_class->external_name(), k_old_method->name_and_sig_as_C_string(), oi);
return JVMTI_ERROR_UNSUPPORTED_REDEFINITION_METHOD_DELETED;
break;
default:
ShouldNotReachHere();
Expand Down Expand Up @@ -3918,26 +3875,6 @@ int VM_RedefineClasses::check_methods_and_mark_as_obsolete() {
}
old_method->set_is_old();
}
for (int i = 0; i < _deleted_methods_length; ++i) {
Method* old_method = _deleted_methods[i];

assert(!old_method->has_vtable_index(),
"cannot delete methods with vtable entries");;

// Mark all deleted methods as old, obsolete and deleted
old_method->set_is_deleted();
old_method->set_is_old();
old_method->set_is_obsolete();
++obsolete_count;
// With tracing we try not to "yack" too much. The position of
// this trace assumes there are fewer obsolete methods than
// EMCP methods.
if (log_is_enabled(Trace, redefine, class, obsolete, mark)) {
ResourceMark rm;
log_trace(redefine, class, obsolete, mark)
("mark deleted %s(%s) as obsolete", old_method->name()->as_C_string(), old_method->signature()->as_C_string());
}
}
assert((emcp_method_count + obsolete_count) == _old_methods->length(),
"sanity check");
log_trace(redefine, class, obsolete, mark)("EMCP_cnt=%d, obsolete_cnt=%d", emcp_method_count, obsolete_count);
Expand Down Expand Up @@ -4051,7 +3988,7 @@ class TransferNativeFunctionRegistration {
prefixes = JvmtiExport::get_all_native_method_prefixes(&prefix_count);
}

// Attempt to transfer any of the old or deleted methods that are native
// Attempt to transfer any of the old methods that are native
void transfer_registrations(Method** old_methods, int methods_length) {
for (int j = 0; j < methods_length; j++) {
Method* old_method = old_methods[j];
Expand All @@ -4073,7 +4010,6 @@ class TransferNativeFunctionRegistration {
// Don't lose the association between a native method and its JNI function.
void VM_RedefineClasses::transfer_old_native_function_registrations(InstanceKlass* the_class) {
TransferNativeFunctionRegistration transfer(the_class);
transfer.transfer_registrations(_deleted_methods, _deleted_methods_length);
transfer.transfer_registrations(_matching_old_methods, _matching_methods_length);
}

Expand Down Expand Up @@ -4114,18 +4050,14 @@ void VM_RedefineClasses::flush_dependent_code() {
JvmtiExport::set_all_dependencies_are_recorded(true);
}

void VM_RedefineClasses::compute_added_deleted_matching_methods() {
void VM_RedefineClasses::compute_matching_methods() {
Copy link
Member

Choose a reason for hiding this comment

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

I can't see that this method actually still does anything useful. ??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it does:

  • has asserts on added/deleted methods
  • collects _matching_old_methods and _matching_new_methods

It seems that the _old_methods is same as _matching_old_methods and _new_methods is same as _matching_new_methods. But I do not want to make a deeper refactoring at this point until we have a decision on the full removal of added/deleted methods support. It feels like we may need to keep some support for lambda expression changes in class redefinitions/retransformations.

Copy link
Contributor

Choose a reason for hiding this comment

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

What this does now is that the redefined class can have matching method names in a different order because of the signature. So this sorts this out. I don't know why we don't sort methods according to name and signature when creating the klass though. That would make this just a method to check that the methods match.

Method* old_method;
Method* new_method;

_matching_old_methods = NEW_RESOURCE_ARRAY(Method*, _old_methods->length());
_matching_new_methods = NEW_RESOURCE_ARRAY(Method*, _old_methods->length());
_added_methods = NEW_RESOURCE_ARRAY(Method*, _new_methods->length());
_deleted_methods = NEW_RESOURCE_ARRAY(Method*, _old_methods->length());

_matching_methods_length = 0;
_deleted_methods_length = 0;
_added_methods_length = 0;

int nj = 0;
int oj = 0;
Expand All @@ -4135,14 +4067,10 @@ void VM_RedefineClasses::compute_added_deleted_matching_methods() {
break; // we've looked at everything, done
}
// New method at the end
new_method = _new_methods->at(nj);
_added_methods[_added_methods_length++] = new_method;
++nj;
assert(false, "unexpected added method at the end");
} else if (nj >= _new_methods->length()) {
// Old method, at the end, is deleted
old_method = _old_methods->at(oj);
_deleted_methods[_deleted_methods_length++] = old_method;
++oj;
assert(false, "unexpected deleted method at the end");
} else {
old_method = _old_methods->at(oj);
new_method = _new_methods->at(nj);
Expand All @@ -4155,24 +4083,21 @@ void VM_RedefineClasses::compute_added_deleted_matching_methods() {
} else {
// added overloaded have already been moved to the end,
// so this is a deleted overloaded method
_deleted_methods[_deleted_methods_length++] = old_method;
++oj;
assert(false, "unexpected deleted overloaded method");
}
} else { // names don't match
if (old_method->name()->fast_compare(new_method->name()) > 0) {
// new method
_added_methods[_added_methods_length++] = new_method;
++nj;
assert(false, "unexpected added method");
} else {
// deleted method
_deleted_methods[_deleted_methods_length++] = old_method;
++oj;
assert(false, "unexpected deleted method");
}
}
}
}
assert(_matching_methods_length + _deleted_methods_length == _old_methods->length(), "sanity");
assert(_matching_methods_length + _added_methods_length == _new_methods->length(), "sanity");
assert(_matching_methods_length == _old_methods->length(), "sanity");
assert(_matching_methods_length == _new_methods->length(), "sanity");
}


Expand Down Expand Up @@ -4218,7 +4143,7 @@ void VM_RedefineClasses::redefine_single_class(Thread* current, jclass the_jclas
_old_methods = the_class->methods();
_new_methods = scratch_class->methods();
_the_class = the_class;
compute_added_deleted_matching_methods();
compute_matching_methods();
update_jmethod_ids();

_any_class_has_resolved_methods = the_class->has_resolved_methods() || _any_class_has_resolved_methods;
Expand Down Expand Up @@ -4573,26 +4498,6 @@ void VM_RedefineClasses::dump_methods() {
m->access_flags().print_on(&log_stream);
log_stream.cr();
}
log_trace(redefine, class, dump)("_deleted_methods --");
for (j = 0; j < _deleted_methods_length; ++j) {
LogStreamHandle(Trace, redefine, class, dump) log_stream;
Method* m = _deleted_methods[j];
log_stream.print("%4d (%5d) ", j, m->vtable_index());
m->access_flags().print_on(&log_stream);
log_stream.print(" -- ");
m->print_name(&log_stream);
log_stream.cr();
}
log_trace(redefine, class, dump)("_added_methods --");
for (j = 0; j < _added_methods_length; ++j) {
LogStreamHandle(Trace, redefine, class, dump) log_stream;
Method* m = _added_methods[j];
log_stream.print("%4d (%5d) ", j, m->vtable_index());
m->access_flags().print_on(&log_stream);
log_stream.print(" -- ");
m->print_name(&log_stream);
log_stream.cr();
}
}

void VM_RedefineClasses::print_on_error(outputStream* st) const {
Expand Down
9 changes: 2 additions & 7 deletions src/hotspot/share/prims/jvmtiRedefineClasses.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -337,11 +337,7 @@ class VM_RedefineClasses: public VM_Operation {
static Array<Method*>* _new_methods;
static Method** _matching_old_methods;
static Method** _matching_new_methods;
static Method** _deleted_methods;
static Method** _added_methods;
static int _matching_methods_length;
static int _deleted_methods_length;
static int _added_methods_length;
static bool _has_redefined_Object;
static bool _has_null_class_loader;

Expand Down Expand Up @@ -403,9 +399,8 @@ class VM_RedefineClasses: public VM_Operation {
jvmtiError compare_and_normalize_class_versions(
InstanceKlass* the_class, InstanceKlass* scratch_class);

// Figure out which new methods match old methods in name and signature,
// which methods have been added, and which are no longer present
void compute_added_deleted_matching_methods();
// Figure out which new methods match old methods in name and signature.
void compute_matching_methods();

// Change jmethodIDs to point to the new methods
void update_jmethod_ids();
Expand Down
4 changes: 1 addition & 3 deletions src/hotspot/share/prims/resolvedMethodTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -358,9 +358,7 @@ class AdjustMethodEntries : public StackObj {

if (old_method->is_old()) {

Method* new_method = (old_method->is_deleted()) ?
Universe::throw_no_such_method_error() :
old_method->get_new_method();
Method* new_method = old_method->get_new_method();
java_lang_invoke_ResolvedMethodName::set_vmtarget(mem_name, new_method);

ResourceMark rm;
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/runtime/arguments.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,6 @@ void Arguments::init_version_specific_system_properties() {
static SpecialFlag const special_jvm_flags[] = {
// -------------- Deprecated Flags --------------
// --- Non-alias flags - sorted by obsolete_in then expired_in:
{ "AllowRedefinitionToAddDeleteMethods", JDK_Version::jdk(13), JDK_Version::undefined(), JDK_Version::undefined() },
{ "FlightRecorder", JDK_Version::jdk(13), JDK_Version::undefined(), JDK_Version::undefined() },
{ "DumpSharedSpaces", JDK_Version::jdk(18), JDK_Version::jdk(19), JDK_Version::undefined() },
{ "DynamicDumpSharedSpaces", JDK_Version::jdk(18), JDK_Version::jdk(19), JDK_Version::undefined() },
Expand All @@ -532,6 +531,7 @@ static SpecialFlag const special_jvm_flags[] = {
#ifdef _LP64
{ "UseCompressedClassPointers", JDK_Version::jdk(25), JDK_Version::jdk(26), JDK_Version::undefined() },
#endif
{ "AllowRedefinitionToAddDeleteMethods", JDK_Version::jdk(13), JDK_Version::jdk(26), JDK_Version::jdk(27) },
{ "ParallelRefProcEnabled", JDK_Version::jdk(26), JDK_Version::jdk(27), JDK_Version::jdk(28) },
{ "ParallelRefProcBalancingEnabled", JDK_Version::jdk(26), JDK_Version::jdk(27), JDK_Version::jdk(28) },
{ "PSChunkLargeArrays", JDK_Version::jdk(26), JDK_Version::jdk(27), JDK_Version::jdk(28) },
Expand Down
4 changes: 0 additions & 4 deletions src/hotspot/share/runtime/globals.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -873,10 +873,6 @@ const int ObjectAlignmentInBytes = 8;
"and can affect tests that expect specific methods to be EMCP. " \
"This option should be used with caution.") \
\
product(bool, AllowRedefinitionToAddDeleteMethods, false, \
"(Deprecated) Allow redefinition to add and delete private " \
"static or final methods for compatibility with old releases") \
\
develop(bool, TraceBytecodes, false, \
"Trace bytecode execution") \
\
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,6 @@ public class VMDeprecatedOptions {
ArrayList<String[]> deprecated = new ArrayList(
Arrays.asList(new String[][] {
// { <flag name> , <expected default value> }
// deprecated non-alias flags:
{"AllowRedefinitionToAddDeleteMethods", "true"},

// deprecated alias flags (see also aliased_jvm_flags):
{"CreateMinidumpOnCrash", "false"}
}
Expand Down
Loading