RFR(L): 8197405: Improve messages of AbstractMethodErrors and IncompatibleClassChangeErrors.

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Fri Feb 23 13:25:45 UTC 2018


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.

> 
> 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.

> 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"

> 
> 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?
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