RFR(L): 8197405: Improve messages of AbstractMethodErrors and IncompatibleClassChangeErrors.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Mon Feb 26 16:15:09 UTC 2018
Hi
I prepared a new webrev, and two incremental ones.
First, a cleanup passing less arguments. The holder can be taken from
the method passed in:
http://cr.openjdk.java.net/~goetz/wr18/8197405-ameExMsg/webrev.03-inc/
Second, incremental, the edits I did to improve the message as proposed
by David:
http://cr.openjdk.java.net/~goetz/wr18/8197405-ameExMsg/webrev.03-incMsg/
Last, the complete webrev:
http://cr.openjdk.java.net/~goetz/wr18/8197405-ameExMsg/webrev.03/
David, as you proposed I'm now using the word 'receiver' in the
message. I don't think any of the alternatives would be better.
Further, I print the modifiers of the methods.
Also, I print "abstract class", "interface" or just "class".
Then, I figured sometimes the selected method, sometimes the
resolved method was passed to my helper method. This explains
the unexpected message.
I split the helper method so that I can report selected AND resolved
method if both are available.
I also call it at two sites we missed (probably because we could not
write test cases for them).
So for the example below, if you call
new C().a()
you get
"Receiver class C does not define or inherit an implementation of the resolved method " +
public abstract a()V of abstract class B."
and if you call
A a = new C(); a.a()
you get
"Receiver class C does not define or inherit an implementation of the resolved method
public a()V of class A. Selected method is public abstract B.a()V."
(Where C is errornous.)
Besides these changes and the cleanups I added four tests. Two
of them implement the example from the mail thread, two others
force the code through stubs.
I'm running this through our tests tonight.
Best regards,
Goetz.
> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Montag, 26. Februar 2018 07:34
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-
> dev at openjdk.java.net
> Subject: Re: RFR(L): 8197405: Improve messages of AbstractMethodErrors and
> IncompatibleClassChangeErrors.
>
> Hi Goetz,
>
> On 23/02/2018 11:25 PM, Lindenmaier, Goetz wrote:
> > Hi David,
> >
> > thanks for looking at the change.
> >
> > I'll implement a new test case using your example. I also think your
> > wording is better. Except the word 'Receiver', I think not all Java
> > developers will be familiar with that wording. I would prefer the
> > word 'Class' here, what do you think?
> > But see also my comments inline.
> >
> >> -----Original Message-----
> >> From: David Holmes [mailto:david.holmes at oracle.com]
> >> Sent: Freitag, 23. Februar 2018 04:22
> >> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-
> >> dev at openjdk.java.net
> >> Subject: Re: RFR(L): 8197405: Improve messages of AbstractMethodErrors
> >> and IncompatibleClassChangeErrors.
> >>
> >> Hi Goetz,
> >>
> >> I couldn't work through all of this but it touches on some areas I'm
> >> working on with JEP 181 (Nestmates).
> >>
> >> I think this is well intentioned but I think there are far more complex
> >> possibilities that make your improved messages inaccurate by trying to
> >> be too specific.
> >>
> >> Also at least for ICCE there may be existing tests that check the ICCE
> >> error message and which will now need updating.
> > I ran all jdk and hotspot tests on all the platforms we support. None fail due to
> my change.
> > There are the selection_resolution tests which run into timeouts sporadically.
> > They test ICCE etc., but I could not find a place that checks
> > for the exception message.
>
> Okay. Some tests _should_ be checking for specific ICCE but that's a
> different matter. Thanks for checking.
>
> >>
> >> More below ...
> >>
> >> On 10/02/2018 12:01 AM, Lindenmaier, Goetz wrote:
> >>> Hi,
> >>>
> >>> I changed the test to use jasm.
> >>> http://cr.openjdk.java.net/~goetz/wr18/8197405-ameExMsg/webrev.02/
> >>
> >> src/hotspot/share/interpreter/linkResolver.cpp
> >>
> >> 1774 void LinkResolver::throw_abstract_method_error(methodHandle&
> >> method, Klass *abstract_klass,
> >>
> >> The Klass* pointer is the resolved method class - yes? The name
> >> "abstract_klass" is misleading as it need not be abstract.
> > Yes, abstract class is the superclass/interface of the klass of the
> > concrete object the method is called on.
> > I would like to simplify the code by using method->method_holder()
> > instead.
>
> That would be the resolved method class I think.
>
> >> Relatedly
> >> your error messages may not be accurate. Suppose you have:
> >>
> >> class A {
> >> public void m() {}
> >>
> >> void test(A a) {
> >> a.m();
> >> }
> >> }
> >>
> >> abstract class B extends A {
> >> public abstract void m();
> >> }
> >>
> > class C extends B {} // as .jasm
> >
> >> If you pass test() a receiver that is a subclass of B and which doesn't
> >> define an implementation of m, then IIUC the resolved method class is A,
> >> which is not abstract - but your error message would say "abstract class A".
> > I implemented it. It says
> > "Class C does not implement the requested method m()V inherited from
> abstract class B"
>
> Hmmm that doesn't seem right. If you are actually passing in the
> resolved method class then that should be A not B
>
> >>
> >> I think if you want to report whether a class is abstract then you need
> >> to actually query is_abstract(). Or simply report things more directly:
> > I'll implement this. It's a good point.
> >
> >> "Receiver class R does not define or inherit an implementation of the
> >> resolved method X.y"
> > I'll change the wording accordingly. See also my comment above on 'Receiver'.
> >
> >> It may also be useful to report the access modifier of the resolved
> >> method (and the selected method that was found to be abstract) as that
> >> can also affect selection.
> > Don't get this.What's the difference between resolved and selected here?
>
> Resolution is based on the types expressed in the bytecode. Selection
> can start from the type of the receiver. When looking for a method with
> a given name/signature private methods can be skipped during selection.
> So someone might wonder why a particular implementation was not selected
> ... but thinking more on that the cases this could happen would likely
> require separate compilation.
>
> Thanks,
> David
>
> > If I understand your comment below correctly, there is no resolved method
> > as the exception must be thrown.
> > I'll print the access modifier of the method at hand :)
> >
> >> 1780 // If method is null, use void signature.
> >>
> >> How can method be null? Surely it is either the selected method (which
> >> is abstract) or it's the resolved method (for which no selected method
> >> could be found).
> > We had to add this for coverity. The code changed over the years,
> > especially with the invokeinterface fix. So it might be superfluous now.
> >
> >> 1785 if(recv_klass) {
> >>
> >> Style: if (recv_klass != NULL) {
> >>
> >> But how can the receiver class be null here ?
> > Yes, nonsense. will remove. (also, the code messes up recv and abstract class
> arguments ...)
> >
> > Best regards,
> > Goetz.
> >
> >
> >
> >
> >>
> >>
> >> Cheers,
> >> David
> >> -----
> >>
> >>> Also, I fixed a bug and disabled the test on aarch/arm.
> >>>
> >>> I started to port the simple stuff to aarch/arm.
> >>> I'm happy to include a further patch for these, but if I
> >>> have time I will also try to implement the rest, can't test
> >>> it though.
> >>>
> >>> Best regards,
> >>> Goetz.
> >>>
> >>>> -----Original Message-----
> >>>> From: David Holmes [mailto:david.holmes at oracle.com]
> >>>> Sent: Donnerstag, 8. Februar 2018 23:54
> >>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-
> >>>> dev at openjdk.java.net
> >>>> Subject: Re: RFR(L): 8197405: Improve messages of AbstractMethodErrors
> >>>> and IncompatibleClassChangeErrors.
> >>>>
> >>>> Hi Goetz,
> >>>>
> >>>> Binary class files can not be checked-in to the repo. You'll need to use
> >>>> jasm or jcod files that then get compiled as part of the test.
> >>>>
> >>>> Thanks,
> >>>> David
> >>>>
> >>>> On 9/02/2018 12:41 AM, Lindenmaier, Goetz wrote:
> >>>>> Hi,
> >>>>>
> >>>>> This change improves the messages of AbstractMethodErrors and
> >>>> IncompatibleClassChangeErrors.
> >>>>> Please review.
> >>>>> http://cr.openjdk.java.net/~goetz/wr18/8197405-
> >> ameExMsg/webrev.01/
> >>>>>
> >>>>> It adds tests that check some of the improved messages.
> >>>>> The tests check the following improvements:
> >>>>>
> >>>>> AbstractMethodErrors:
> >>>>>
> >>>>> test_ameInt, test case 1:
> >>>>> before: no message / null
> >>>>> after: Missing implementation of interface method
> >>>> MyAbstractInt.anAbstractMethod()Ljava/lang/String;
> >>>>>
> >>>>> test_ameInt, test case 2:
> >>>>> before: no message / null
> >>>>> after: Class ImplementsSomeFunctionsInt does not implement the
> >>>> requested method
> >>>>> aFunctionOfMyInterface()Ljava/lang/String; inherited from
> >> interface
> >>>> MyInterfaceInt1
> >>>>>
> >>>>> test_ame, interpreted:
> >>>>> before: no message / null
> >>>>> after: Missing implementation of interface method
> >>>> MyInterface.aFunctionOfMyInterface()V
> >>>>>
> >>>>> test_ame, compiled:
> >>>>> before: MyAbstract.aFunctionOfMyInterface()V
> >>>>> after: Class ImplementsSomeFunctions does not implement the
> >> requested
> >>>> method
> >>>>> aFunctionOfMyInterface()V inherited from abstract class
> >> MyAbstract
> >>>>>
> >>>>> IncompatibleClassChangeErrors:
> >>>>>
> >>>>> test_icc_compiled_itable_stub
> >>>>> before: vtable stub
> >>>>> after: Class ICC_B does not implement the requested interface ICC_iB
> >>>>>
> >>>>> Best regards,
> >>>>> Goetz.
> >>>>>
More information about the hotspot-runtime-dev
mailing list