RFR(L): 8197405: Improve messages of AbstractMethodErrors and IncompatibleClassChangeErrors.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Fri Mar 9 08:11:58 UTC 2018
Hi Coleen,
I had added the THREAD argument. David agreed to the
change. (http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-March/026750.html)
I would appreciate if you could sponsor this, but I can also try
to find someone else if you are very busy.
The final webrev:
http://cr.openjdk.java.net/~goetz/wr18/8197405-ameExMsg/webrev.07/
Best regards,
Goetz.
> -----Original Message-----
> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
> bounces at openjdk.java.net] On Behalf Of coleen.phillimore at oracle.com
> Sent: Donnerstag, 1. März 2018 15:57
> To: hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(L): 8197405: Improve messages of AbstractMethodErrors
> and IncompatibleClassChangeErrors.
>
>
> 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.fram
> es.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