RFR(L): 8197405: Improve messages of AbstractMethodErrors and IncompatibleClassChangeErrors.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Fri Mar 2 08:15:59 UTC 2018
Hi David,
thanks for the final ok! Final webrev:
http://cr.openjdk.java.net/~goetz/wr18/8197405-ameExMsg/webrev.06/
> I guess time will tell.
I hope time tells the improved messages are useful and nothing else :)
> I think you need a "return;" after throw_abstract_method_error().
Yes, that was the problem.
> For the test .jasm files could you please put the explanatory comment at
> the top of the file rather than the bottom - thanks.
I moved the comments up.
Best regards,
Goetz.
> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Freitag, 2. März 2018 08: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 27/02/2018 8:11 PM, Lindenmaier, Goetz wrote:
> > Hi David,
> >
> > Thanks for looking at this again!
> >
> > Incremental and new webrev:
> > http://cr.openjdk.java.net/~goetz/wr18/8197405-ameExMsg/webrev.04-
> inc/
>
> src/hotspot/share/interpreter/interpreterRuntime.cpp
>
> ! if (m.not_null() && m->is_abstract()) {
> ! LinkResolver::throw_abstract_method_error(missingMethod, THREAD);
> }
> }
> ! THROW(vmSymbols::java_lang_AbstractMethodError());
>
> I think you need a "return;" after throw_abstract_method_error().
>
> For the test .jasm files could you please put the explanatory comment at
> the top of the file rather than the bottom - thanks.
>
> Otherwise changes relative to previous version seem okay.
>
> Overall I still have some concerns about this but nothing I can point
> directly to. I guess time will tell. I suspect I may learn more when I
> have to integrate this with the Valhalla nestmates branch.
>
> Thanks,
> David
>
> > 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