RFR(L): 8197405: Improve messages of AbstractMethodErrors and IncompatibleClassChangeErrors.
Doerr, Martin
martin.doerr at sap.com
Tue Feb 27 15:16:20 UTC 2018
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