RFR: 8338471: Refactor Method::get_new_method() for better NoSuchMethodError handling [v2]
Coleen Phillimore
coleenp at openjdk.org
Mon Sep 9 13:32:13 UTC 2024
On Mon, 9 Sep 2024 03:29:57 GMT, David Holmes <dholmes at openjdk.org> wrote:
>> src/hotspot/share/oops/method.hpp line 853:
>>
>>> 851: Method* new_method = method_holder()->method_with_idnum(orig_method_idnum());
>>> 852: assert(this != new_method, "sanity check");
>>> 853: return (new_method == nullptr || is_deleted()) ? Universe::throw_no_such_method_error() : new_method;
>>
>> 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?
>
> 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.
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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20874#discussion_r1750263229
More information about the hotspot-dev
mailing list