-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8356941: AbstractMethodError in HotSpot Due to Incorrect Handling of Private Method #26302
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
Conversation
👋 Welcome back dholmes! A progress list of the required criteria for merging this PR into |
@dholmes-ora This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 97 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
@dholmes-ora The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better without logging. It is not immediately clear if we need more ResourceMark
-s around some logging messages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the logging gets in the way of understanding the code. It makes the code hard to read.
Method* impl = klass->lookup_method(m->name(), m->signature()); | ||
if (impl == nullptr || impl->is_overpass() || impl->is_static()) { | ||
if (impl == nullptr || impl->is_overpass() || impl->is_static() || impl->is_private()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still haven't figured out why this code fixes this issue, but why doesn't line 657 need the same change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why doesn't line 657 need the same change?
Are you asking why it doesn't need the change to address the current bug? That would be because that block of code has nothing at all to do with the current bug. That block is looking at superclass overpass and static methods, to see if they may be candidates for replacing by a default method. For each of those methods it checks if there is an implementation in the current class that matches the name and sig and is a real method, and if so it doesn't create an empty vtable slot for it for default method processing to fill in. I spent a long time trying to create a scenario that would hit this block and the condition you want changed, but failed. Lets say we are looking at a slot for method void m()
for simplicity. First you need a superclass that declares as static void m()
method. Then you need a subclass that implements a non-static void m()
method. But you also need to have the subclass implement an interface with a default void m()
method, but in that case we will never reach the block of code because we will find that default method is already present in the list of miranda methods that we already processed! (During this phase of processing, default methods are considered mirandas - pass 1).
Now as to the actual fix ... we have a subclass that we are processing, and a superclass that has a private void m()
method, and a super-super-interface that defines the default void m()
method. We find void m()
in the superclass's default methods - hence we reach the block of code I modified. For this slot to be processed for default method handling (i.e. to get the correct entry so we invoke the true default method) we need to decide to create an empty vtable slot. The current logic (mirroring that for superclass methods - including the comment block that may or may not be accurate) checks to see if the current class has a concrete implementation of void m()
as that would replace use of the default implementation (class instance methods "win"), except that is only true for non-private concrete implementations - a private class method is never considered the implementation of an interface method. So if the method is private we need to create the empty vtable slot - hence the fix.
Note that klass->lookup_method
looks up the method in the current class and all superclasses, and will find overpass and private methods - so an alternative fix would be to change the lookup to skip private methods (and we could also skip overpasses and then simplify the condition further). But the two approaches to the fix seem equivalent in terms of readability/complexity and what I did was slightly shorter to write and fitted in with the other checks nicely.
I hope that clarifies things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see why your fix fixes the bug in your test case. I was asking about the block above with the identical comment and code. Like you, I was trying to find a scenario where the block above your fix might also need the same condition added and couldn't find one. Because if there's a private non-static function hiding a static function in a super class, the default method is then processed and will get the vtable slot added.
So your fix is good.
Thanks for looking at this @shipilev !
Regarding
|
Thanks for looking at this @coleenp !
I don't disagree. Unfortunately to get the indentation working we need to use a very verbose form of the logging code - and there is lots of duplication. It may be possible to introduce a helper function, but it will still be quite intrusive. That said I found the logging invaluable when trying to understand why the wrong method was being executed. Though that said the logging is still far from complete in terms of getting a full understanding of how things are processed - I had to add a lot more in my local repo to do the additional experiments today. So I will remove the logging (and just stash my changes locally for next time). Thanks. |
This reverts commit 6087184.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix looks good.
Method* impl = klass->lookup_method(m->name(), m->signature()); | ||
if (impl == nullptr || impl->is_overpass() || impl->is_static()) { | ||
if (impl == nullptr || impl->is_overpass() || impl->is_static() || impl->is_private()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see why your fix fixes the bug in your test case. I was asking about the block above with the identical comment and code. Like you, I was trying to find a scenario where the block above your fix might also need the same condition added and couldn't find one. Because if there's a private non-static function hiding a static function in a super class, the default method is then processed and will get the vtable slot added.
So your fix is good.
Thanks for the review @coleenp ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current change is also fine. Both suggestions are more for leaving breadcrumbs to help the next person to dig thru this code
@@ -664,7 +664,7 @@ static void find_empty_vtable_slots(GrowableArray<EmptyVtableSlot*>* slots, | |||
// unless we have a real implementation of it in the current class. | |||
if (!already_in_vtable_slots(slots, m)) { | |||
Method* impl = klass->lookup_method(m->name(), m->signature()); | |||
if (impl == nullptr || impl->is_overpass() || impl->is_static()) { | |||
if (impl == nullptr || impl->is_overpass() || impl->is_static() || impl->is_private()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to capture your and Coleen's discussion in a comment. Does this correctly cover the gist of the discussion?
if (impl == nullptr || impl->is_overpass() || impl->is_static() || impl->is_private()) { | |
// 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()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea and I don't think there's harm in using this condition in the upper loop also, even though we couldn't find a case of a private method not getting to the code to create the empty vtable slots. I tested the upper loop with that case and even though we couldn't prove it's needed, I think it's also harmless to make it the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit happier that the comment is now not duplicated, since neither was helpful to me. This new comment is better.
Thanks again Coleen and Dan! /integrate |
Going to push as commit 0385975.
Your commit was automatically rebased without conflicts. |
@dholmes-ora Pushed as commit 0385975. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Private methods should never be considered as the implementation of a default method.
The first commit adds some additional logging I used to track down what was happening. I like it, but if reviewers think it too much I can drop it.
The second commit is the actual fix to exclude private methods, and adds the missing test case to the existing defmeth tests.
Testing:
Thanks
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26302/head:pull/26302
$ git checkout pull/26302
Update a local copy of the PR:
$ git checkout pull/26302
$ git pull https://git.openjdk.org/jdk.git pull/26302/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26302
View PR using the GUI difftool:
$ git pr show -t 26302
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26302.diff
Using Webrev
Link to Webrev Comment