RFR(L): 8197405: Improve messages of AbstractMethodErrors and IncompatibleClassChangeErrors.
David Holmes
david.holmes at oracle.com
Fri Mar 2 07:34:24 UTC 2018
Hi Goetz,
On 27/02/2018 8:11 PM, Lindenmaier, Goetz wrote:
> Hi David,
>
> Thanks for looking at this again!
>
> Incremental and new webrev:
> http://cr.openjdk.java.net/~goetz/wr18/8197405-ameExMsg/webrev.04-inc/
src/hotspot/share/interpreter/interpreterRuntime.cpp
! if (m.not_null() && m->is_abstract()) {
! LinkResolver::throw_abstract_method_error(missingMethod, THREAD);
}
}
! THROW(vmSymbols::java_lang_AbstractMethodError());
I think you need a "return;" after throw_abstract_method_error().
For the test .jasm files could you please put the explanatory comment at
the top of the file rather than the bottom - thanks.
Otherwise changes relative to previous version seem okay.
Overall I still have some concerns about this but nothing I can point
directly to. I guess time will tell. I suspect I may learn more when I
have to integrate this with the Valhalla nestmates branch.
Thanks,
David
> http://cr.openjdk.java.net/~goetz/wr18/8197405-ameExMsg/webrev.04/
>
>> 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.
> That's just dead code I found when checking whether the code already
> contains the call to get_handle_wrong_method_stub(). I'll revert that.
>
>> 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.
> test_icc_compiled_itable_stub() excercises this code. The callsite is reset
> and then resolved again. It's the slow way, but it does not matter
> in case of an exception. The problem is that you can not fuddle with
> the argument registers here. You get the following callstack:
> LinkResolver::runtime_resolve_interface_method()
> LinkResolver::resolve_interface_call()
> LinkResolver::resolve_invokeinterface()
> LinkResolver::resolve_invoke()
> SharedRuntime::find_callee_info_helper()
> SharedRuntime::find_callee_method()
> SharedRuntime::reresolve_call_site()
> SharedRuntime::handle_wrong_method()
>
>> 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?
> You are right, abstract classes pop up here, too. See the changes I
> had to do to the messages in the test after fixing this.
> I'm now also calling LinkResolver::throw_abstract_method_error
> from here, too.
>
>> I don't really understand the distinction and implementation differences
>> between:
>> InterpreterRuntime::throw_AbstractMethodErrorWithMethod
>> InterpreterRuntime::throw_AbstractMethodErrorVerbose
> Where I call the first, the receiver is not known. I could call the second
> with null as receiver in that place, but I like to avoid touching registers in
> assembly.
>
>> what are the assumed preconditions for each? Why does the former throw
>> directly while the latter calls LinkResolver?
> Because the former lacks the receiver.
> The method in LinkResolver is called in all places where the receiver and
> the resolved method are known to avoid to duplicate the code and to avoid
> that the error messages differ.
> But I changed that, see above.
>
>>> 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.
> ... You mean it's not helpful if it says NULL? Yes.
> This is just for safety. They never should be NULL. But the code is well tested
> like this, (we run it since 2010) so I'd like to leave it as is.
>
>> And again is this always only called for interface methods?
> It's from invokeinterface.
>
>> 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.
> Done.
>
>> 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?
> OK, I'll only print private, abstract.
>
>> 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().
> Yes, dead, removed.
>
>> 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. ?
> Well, the print statement is complex enough already, but I'll merge them.
> But looks fine :)
>
>>> 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/Abstract
>> MethodErrorTest.java)
> Consistent spelling, but wrong :)
>
>>> 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.
> I added comments.
>
>> Why are the tests excluded on Aarch64 and ARM? Simply not tested there
>> yet?
> I did some simple edits. But I can not test these. I asked some
> people I know do aarch to contribute. If they don't I'll revert the platform
> changes. If they do, I'll enable the tests.
>
> Our tests didn't run, the infrastructure had a problem ... I'll try again
> tonight with all the improvements.
>
> Best regards,
> Goetz.
>
>
>> 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