RFR(L): 8197405: Improve messages of AbstractMethodErrors and IncompatibleClassChangeErrors.
David Holmes
david.holmes at oracle.com
Fri Mar 2 08:29:15 UTC 2018
Looks fine.
Thanks,
David
On 2/03/2018 6:15 PM, Lindenmaier, Goetz wrote:
> Hi David,
>
> thanks for the final ok! Final webrev:
> http://cr.openjdk.java.net/~goetz/wr18/8197405-ameExMsg/webrev.06/
>
>> I guess time will tell.
> I hope time tells the improved messages are useful and nothing else :)
>
>> I think you need a "return;" after throw_abstract_method_error().
> Yes, that was the problem.
>
>> For the test .jasm files could you please put the explanatory comment at
>> the top of the file rather than the bottom - thanks.
> I moved the comments up.
>
> Best regards,
> Goetz.
>
>
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Freitag, 2. März 2018 08: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 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