RFR(L): 8197405: Improve messages of AbstractMethodErrors and IncompatibleClassChangeErrors.
David Holmes
david.holmes at oracle.com
Tue Feb 27 00:32:22 UTC 2018
Hi Goetz,
Please note I only partially reviewed this as I can't comment on the
interpreter and platform-specific parts in detail. This needs more eyes
on it for that - in particular the S390 "padding" changes seem unrelated
to current bug and definitely needs someone familiar with that code to
review.
I'm very unclear how get_handle_wrong_method_stub() works in the context
of obtaining the additional information needed for the more verbose
error messages.
More below ...
On 27/02/2018 2:15 AM, Lindenmaier, Goetz wrote:
> 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/
I looked over all the code again to get a better sense of things. Though
still am unclear on a lot of details - as per above ref to
get_handle_wrong_method_stub.
src/hotspot/share/interpreter/interpreterRuntime.cpp
598 "Missing implementation of interface method %s",
Are we sure this is only called for interface methods? Where's the
equivalent logic for abstract class methods?
I don't really understand the distinction and implementation differences
between:
InterpreterRuntime::throw_AbstractMethodErrorWithMethod
InterpreterRuntime::throw_AbstractMethodErrorVerbose
what are the assumed preconditions for each? Why does the former throw
directly while the latter calls LinkResolver?
622 jio_snprintf(buf, sizeof(buf),
623 "Class %s does not implement the requested
interface %s",
624 recvKlass ? recvKlass->external_name() : "NULL",
625 interfaceKlass ? interfaceKlass->external_name() :
"NULL");
Not clear how recvKlass and interfaceKlass may be NULL, but in such
cases isn't it just that they are unknown rather than "NULL"? Either way
this message seems less than helpful.
And again is this always only called for interface methods?
---
src/hotspot/share/interpreter/linkResolver.cpp
1763 void LinkResolver::throw_abstract_method_error(const methodHandle&
resolved_method, Klass *recv_klass, TRAPS) {
1769 "Receiver class %s does not define or inherit an
implementation of the resolved method %s%s%s of %s %s.",
Please break up these really long lines.
Not sure printing all the method modifiers helps with understanding the
error message. I only wanted the access flag (private, public etc). I
think I'd rather do without than print the modifier list. It also
reduces the scope of the change. Other opinions on this?
1790 const char *resolvedKind = "class";
1791 if (resolved_klass->is_interface()) {
1792 resolvedKind = "interface";
1793 } else if (resolved_klass->is_abstract()) {
1794 resolvedKind = "abstract class";
1795 }
This appears to be dead code as you added klass::external_kind().
I don't think you need to define both
LinkResolver::throw_abstract_method_error and
LinkResolver::throw_abstract_method_error_abstract as the difference
between them is minimal and seems easily accommodated by a null check
for the selected method. ?
> 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.
Ok.
> Further, I print the modifiers of the methods.
See above - this seems overkill. But also see below ...
> 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.
Ok.
> 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."
I do like seeing the "abstract" on B.a. Maybe we can just print if the
selected method is private, and if it is abstract?
> (Where C is errornous.)
typo: erroneous (needs to be fixed in
test/hotspot/jtreg/runtime/exceptionMsgs/AbstractMethodError/AbstractMethodErrorTest.java)
> 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 found the tests quite complicated to follow - do we need all the
methods in the AME* classes? Can you add a top comment in each of the
jasm files clearly explaining how the jasm differs from the class
definition in Java.
Why are the tests excluded on Aarch64 and ARM? Simply not tested there yet?
Thanks,
David
>
> 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