RFR: 8356941: AbstractMethodError in HotSpot Due to Incorrect Handling of Private Method [v3]

[David Holmes]

On Fri, 18 Jul 2025 14:13:06 GMT, Dan Heidinga <heidinga at openjdk.org> wrote:

>> src/hotspot/share/classfile/defaultMethods.cpp line 667:
>> 
>>> 665:         if (!already_in_vtable_slots(slots, m)) {
>>> 666:           Method* impl = klass->lookup_method(m->name(), m->signature());
>>> 667:           if (impl == nullptr || impl->is_overpass() || impl->is_static() || impl->is_private()) {
>> 
>> Trying to capture your and Coleen's discussion in a comment.  Does this correctly cover the gist of the discussion?
>> 
>> Suggestion:
>> 
>>          // Unless the klass provides a non-static public or package-private method for this name
>>          // & sig combo, we need to add the EmptyVtableSlot so default method processing finds
>>          // the correct candidate to fill in the slot later.
>>           if (impl == nullptr || impl->is_overpass() || impl->is_static() || impl->is_private()) {
>
> Alternatively, this same series of checks occurs in `FindMethodsByErasedSig::visit()` with a long comment there about the set of possible methods.  Is it worth replacing both sets of checks with a helper method that encapsulates the set of decisions?  
> 
> 
> // Private interface methods are not candidates for default methods.
> // invokespecial to private interface methods doesn't use default method logic.
> // Private class methods are not candidates for default methods.
> // Private methods do not override default methods, so need to perform
> // default method inheritance without including private methods.
> // The overpasses are your supertypes' errors, we do not include them.
> static bool is_not_excluded_method(Method* m) {
>     if (m != nullptr && !m->is_static() && !m->is_overpass() && !m->is_private()) {
>        return true;
>     }
>     return false;
> }
> 
> 
> The name isn't great - feel free to replace with something better - but the idea is to capture the requirements in one place

Thanks for the review @DanHeidinga . I have opted to just adjust the commentary around that piece of code.

I disagree with Coleen about the potential harm of also changing the upper loop - that would need further investigation.

It also occurred to me that the loop I am fixing is really just an optimization to check whether the current class could potentially use a different implementation for a default method versus its super class. We could unconditionally create an empty slot for all the super class default methods, and things still appear to work correctly. But there may be performance implications. I think what is causing us all to struggle somewhat with understanding the existing logic is that it starts from a position of filling everything in, then goes through and creates an empty slot for all those things that could be wrong; whereas it seems more understandable to me to start with an empty table and then insert everything we know is right. But I suspect the starting assumption here is that most of the time a subclass vtable is just a superclass vtable with a few additions and fewer modifications (ie. overrides), so the current approach is more optimal in general.

Anyway I appreciate the reviews and discussion. I will need someone to re-approve after the comment change. Thanks.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/26302#discussion_r2217988564