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

[David Holmes]

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