RFR(L): 8197405: Improve messages of AbstractMethodErrors and IncompatibleClassChangeErrors.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Thu Mar 1 14:05:01 UTC 2018
Hi,
tonight, all our internal test (including jck) passed on all our platforms.
Also the jdk_submit tests went through.
Mach5 Tasks Results Summary
* KILLED: 0
* PASSED: 81
* EXECUTED_WITH_FAILURE: 0
* UNABLE_TO_RUN: 0
* NA: 0
* FAILED: 0
Martin, thanks for your heads up.
Best regards,
Goetz.
> -----Original Message-----
> From: Doerr, Martin
> Sent: Mittwoch, 28. Februar 2018 14:51
> 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,
>
> yes, I have reviewed the platform files except arm/aarch64. They look good.
>
> Best regards,
> Martin
>
>
> -----Original Message-----
> From: Lindenmaier, Goetz
> Sent: Mittwoch, 28. Februar 2018 12:42
> To: Doerr, Martin <martin.doerr 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,
>
> 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