Skip to content

8338471: Refactor Method::get_new_method() for better NoSuchMethodError handling #20874

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

8338471: Refactor Method::get_new_method() for better NoSuchMethodError handling #20874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f3d4f23
8338471: Refactor Method::get_new_method() for better NoSuchMethodErr…
matias9927 Aug 30, 2024
ba1cb1b
Fixed conditional to check for is_deleted()
matias9927 Sep 6, 2024
bd1cc1e
Added assert
matias9927 Sep 9, 2024
e95d2bd
Coleen suggestion
matias9927 Sep 11, 2024
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4417,8 +4417,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
10 changes: 5 additions & 5 deletions src/hotspot/share/oops/method.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

#include "code/compressedStream.hpp"
#include "compiler/compilerDefinitions.hpp"
#include "memory/universe.hpp"
#include "oops/annotations.hpp"
#include "oops/constantPool.hpp"
#include "oops/methodFlags.hpp"
Expand Down Expand Up @@ -847,12 +848,11 @@ class Method : public Metadata {
void release_C_heap_structures();

Method* get_new_method() const {
InstanceKlass* holder = method_holder();
Method* new_method = holder->method_with_idnum(orig_method_idnum());

assert(new_method != nullptr, "method_with_idnum() should not be null");
assert(is_old(), "must be");
Method* new_method = method_holder()->method_with_idnum(orig_method_idnum());
assert(this != new_method, "sanity check");
return new_method;
assert(new_method != nullptr || is_deleted(), "must be");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert(new_method != nullptr || is_deleted(), "must be");
assert((new_method == nullptr) == (old_method->is_deleted()), "must be");

return (new_method == nullptr || is_deleted()) ? Universe::throw_no_such_method_error() : new_method;
Copy link
Member

Choose a reason for hiding this comment

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

I am still confused by the different possibilities here. Under what conditions will we get nullptr? Is it the case that get_new_method should only be called when is_old() is true? Can is_old and is_deleted be true at the same time?

Copy link
Member

Choose a reason for hiding this comment

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

To answer some of my own questions:

  • yes get_new_method should only be called if is_old is true. (Should we assert that?)
  • yes a method can be old and deleted at the same time.

I remain unclear how nullptr can appear here.

Copy link
Contributor

@coleenp coleenp Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This redefine code is really complicated. Obsolete methods, which includes deleted methods get an incremented idnum in check_methods_and_mark_as_obsolete.

    // obsolete methods need a unique idnum so they become new entries in
    // the jmethodID cache in InstanceKlass
    assert(old_method->method_idnum() == new_method->method_idnum(), "must match");
    u2 num = InstanceKlass::cast(_the_class)->next_method_idnum();
    if (num != ConstMethod::UNSET_IDNUM) {
      old_method->set_method_idnum(num);
    }

When get_new_method() is called it compares InstanceKlass::_methods[idnum], then compares the Method at that index to the idnum that the method has stored. For deleted methods, this will return nullptr because the idnum comparison in method_with_idnum will not match, or idnum for the deleted method is greater than InstanceKlass::_methods.length(). I think it is sufficient to check for nullptr in get_new_method() for deleted methods but also the explicit is_deleted() comparison is much easier to understand that it's the right answer.

Yes, there could be an assert in get_new_method(), the method passed in is_old(). All redefined methods are marked as is_old(). I believe all callers test this before making this call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Coleen is correct, all the callers do indeed check that the method is_old() but I think it's fine if we assert that in get_new_method() to make it clear that it is a requirement. The method method_with_idnum() can return nullptr here:

  if (m == nullptr || m->method_idnum() != idnum) {
    for (int index = 0; index < methods()->length(); ++index) {
      m = methods()->at(index);
      if (m->method_idnum() == idnum) {
        return m;
      }
    }
    // None found, return null for the caller to handle.
    return nullptr;

As the comment suggests, the caller should handle nullptr, which in this case is get_new_method(). The callers of get_new_method() try to handle this but I think it's cleaner to check inside the method.

Copy link
Member

Choose a reason for hiding this comment

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

So is it the case that nullptr implies deleted, and deleted implies nullptr? If so checking for both is redundant and confusing because it makes it look like there are two distinct cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

So that implies that you trust my reading of this code. It is complicated enough that testing both seems like a safe thing to do and somewhat clarifying, or else adding an assert like:

assert(new_method != nullptr || old_method->is_deleted(), "this is the only way this happens");
return new_method == nullptr ? nsme : new_method;

Copy link
Member

Choose a reason for hiding this comment

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

The assert works for me.

Copy link
Member

Choose a reason for hiding this comment

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

Can we assert the stronger statement: (new_method == nullptr) == (old_method->is_deleted()) ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I like that better too.

}

// Printing
Expand Down
4 changes: 1 addition & 3 deletions src/hotspot/share/prims/resolvedMethodTable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,9 +353,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