RFR(L): 8197405: Improve messages of AbstractMethodErrors and IncompatibleClassChangeErrors.
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Thu Mar 1 14:57:02 UTC 2018
I think this change looks very good. The most trivial comment I can
think of:
http://cr.openjdk.java.net/~goetz/wr18/8197405-ameExMsg/webrev.05/src/hotspot/share/interpreter/linkResolver.cpp.frames.html
1767 ResourceMark rm;
You can add THREAD argument since you have it.
I'll sponsor it once you have David's go ahead.
Thanks,
Coleen
On 2/28/18 6:41 AM, Lindenmaier, Goetz wrote:
> Hi,
>
> The debug build of our nightly test showed an issue of our latest builds.
> Thus new webrev with change in throw_AbstractMethodErrorWithMethod()
> http://cr.openjdk.java.net/~goetz/wr18/8197405-ameExMsg/webrev.05/
>
> Besides the one problem in my test which is fixed by the new webrev,
> all nightly tests were green.
>
> Martin, thanks for your review! Can I consider the platform changes as
> reviewed by you?
>
>> I wonder if we could use
>> "SharedRuntime::get_handle_wrong_method_stub()" instead of"
>> StubRoutines::throw_IncompatibleClassChangeError_entry()" in the method
>> handle code
> I'll check our nightly tests whether there is a test case for this execution path.
>
> Best regards,
> Goetz.
>
>
>> -----Original Message-----
>> From: Doerr, Martin
>> Sent: Dienstag, 27. Februar 2018 16:16
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; 'David Holmes'
>> <david.holmes at oracle.com>; hotspot-runtime-dev at openjdk.java.net
>> Subject: RE: RFR(L): 8197405: Improve messages of AbstractMethodErrors
>> and IncompatibleClassChangeErrors.
>>
>> Hi Götz,
>>
>> thanks for contributing this change. Especially for writing all the tests. I really
>> like it.
>>
>> Thanks for removing the unrelated vtableStubs_s390 part which may
>> interfere with Lutz' planned improvement of the size computations. Cleanup
>> should better be done in his change.
>>
>> I wonder if we could use
>> "SharedRuntime::get_handle_wrong_method_stub()" instead of"
>> StubRoutines::throw_IncompatibleClassChangeError_entry()" in the method
>> handle code to get better messages there as well. In addition, this would
>> allow us to get rid of the latter.
>> At the moment, I can't find a test for this case. Maybe there is one in the jck
>> test suite.
>>
>> Also thanks for all the explanations. They sound feasible to me.
>>
>> The issues concerning our nightly builds are resolved. Tests are currently
>> running.
>>
>> Best regards,
>> Martin
>>
>>
>>
>> -----Original Message-----
>> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
>> bounces at openjdk.java.net] On Behalf Of Lindenmaier, Goetz
>> Sent: Dienstag, 27. Februar 2018 11:11
>> To: 'David Holmes' <david.holmes at oracle.com>; hotspot-runtime-
>> dev at openjdk.java.net
>> Subject: RE: RFR(L): 8197405: Improve messages of AbstractMethodErrors
>> and IncompatibleClassChangeErrors.
>>
>> Hi David,
>>
>> Thanks for looking at this again!
>>
>> Incremental and new webrev:
>> http://cr.openjdk.java.net/~goetz/wr18/8197405-ameExMsg/webrev.04-
>> inc/
>> 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