RFR(L): 8197405: Improve messages of AbstractMethodErrors and IncompatibleClassChangeErrors.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Wed Feb 28 11:41:59 UTC 2018
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