RFR: 8338471: Refactor Method::get_new_method() for better NoSuchMethodError handling [v2]
Matias Saavedra Silva
matsaave at openjdk.org
Mon Sep 9 16:06:06 UTC 2024
On Mon, 9 Sep 2024 13:27:24 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:
>> 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.
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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20874#discussion_r1750534303
More information about the hotspot-dev
mailing list