RFR (L) : 8014013 : CallInfo structure no longer accurately reports the result of a LinkResolver operation

[Karen Kinnear]

Couple of follow-ups, including one request for a code change please.

thanks,
Karen

see below ...

>>>> 2. In method.cpp in is_final_method
>>>> Could you please delete the comment lies 513-514
>>>> "Should return true for private methods also, since there is no way to override them?"
>>>> 
>>>> Yes, you do not override private methods. However, this call is used to determine if you
>>>> need a vtable entry. Private methods always get a vtable entry to handle backward compatibility
>>>> with classes - i.e. you can have the same name of a private method local to your class and also
>>>> inherit a method of from your superclass, which will get inherited around the private method, by
>>>> your child.
>> 
>> The above information needs to be in a comment at that point, instead of the question.  The comment line to be deleted has a reasonable question with a non-obvious answer.  The updated comments at that point are not a full answer to the question.  Karen's explanation is a pretty full answer, although I would like to see a fuller hint about the nature of the corner case, and maybe an explanation of how it helps to supply a private method with a vtable index which will never get used.
>> 
>> The corresponding comment in klassVtable::update_inherited_vtable also needs expansion (with the above text, perhaps) for the same reasons:
>> // private methods always have a new entry in the vtable
>> // specification interpretation since classic has
>> // private methods not overriding
>> 
>> It would be fine to have the information in one place, and a comment in the other place with a cross-reference.  But we need documentation of this corner case.
>> 
>> (As a separate issue, I think it is a waste of space, and a needless "surprise" in the code, to give vtable entries to private methods, and that there must be a better way to handle the relevant corner cases.  I filed https://bugs.openjdk.java.net/browse/JDK-8024368 to track this.)
Actually, we use the vtable entries for private methods today including inheriting them. This is probably 
another full discussion, but I worry about backward compatibility if we "clean" this up. We have a full set of
compatibility tests for invokevirtual which you can check out if you want lots of examples.
>> 
>>> bool Method::is_final_method(AccessFlags class_access_flags) const {
>>> // or "does_not_require_vtable_entry"
>> More generally:
>> // or "impossible_to_override"
>>> // overpass can occur, is not final (reuses vtable entry)
>>> // private methods get vtable entries for backward class compatibility.
>>> if (is_overpass())  return false;
>>> return is_final() || class_access_flags.is_final();
>>> }
>>> 
>>>> 4. klassVtable.cpp
>>>> lines 773-774
>>>> I'd like to understand better what this case represents?
>>>> Same question interpreterRuntime.cpp
>>>> CharSequence.toString - use vtable not itable (706-707)
>>> 
>>> inline bool interface_method_needs_itable_index(Method* m) {
>>> if (m->is_static())           return false;   // e.g., Stream.empty
>>> if (m->is_initializer())      return false;   // <init> or <clinit>
>>> // If an interface redeclares a method from java.lang.Object,
>>> // it should already have a vtable index, don't touch it.
>>> // e.g., CharSequence.toString (from initialize_vtable)
>>> // if (m->has_vtable_index())  return false; // NO!
>>> return true;
>>> }
>> 
>> The issue here is that a method cannot have both a vtable and an itable index.  But, at the time interface_method_needs_itable_index is called, it is not absolutely guaranteed that the method has already been assigned a vtable index, even if it will eventually be assigned one.  (This happens, IIRC, during bootstrapping.)  Maybe the commented "if" could be changed to an assert:
Thank you - that was what we guessed, but a clarified comment would help.
>> 
>>> // If an interface redeclares a method from java.lang.Object,
>>> // it should already have a vtable index, don't touch it.
>>> // e.g., CharSequence.toString (from initialize_vtable)
>>> assert(!m->has_vtable_index(), "");
>> 
>>>> 5. linkResolver.hpp
>>>> direct_call line 45, and _call_kind line 55: 
>>>> I think this includes both static and invokeSpecial calls - correct? so the "static" in the comment
>>>> in 55 might want to be expanded
>>> 
>>> Fixed:
>>> CallKind     _call_kind;              // kind of call (static/special, vtable, itable)
>> 
>> Good.  At this level, "static" linkage is the kind of linkage used always for both invokestatic and invokespecial, and sometimes for invokevirtual and invokeinterface (in some corner cases).
Overuse of the term static - a bit confusing. Thanks for the explanation, perhaps the comment could clarify.
>> 
>> One last comment:  I would prefer if the reference parameters to init_method_MemberName and init_field_MemberName could be const references (to const CallInfo and const fieldDescriptor, respectively).  That would better document the intention that those descriptors are input-only, and do not accept side effects of any kind.
>> 
>> On Aug 26, 2013, at 10:22 AM, Karen Kinnear <Karen.Kinnear at oracle.com> wrote:
>> 
>>> 6. linkResolver.cpp
>>> CallInfo::CallInfo:
>>> Why are _selected_klass and _selected_method set to resolved_klass and resolve_method?
>> 
>> Mainly (I think) to prevent print statements from SEGV-ing.
Please change this back - and don't set the information if it is not accurate. Please fix the print statements
to handle null. Null provides meaningful information. If you are using the information as the selected
klass and selected method, I would like to understand cases in which you do that, that are handled outside
of the LinkResolver please, since this would imply perhaps different behavior for invokedynamic options
and I would really like to understand those.
>> 
>>> I thought the CallInfo contains the static linktime resolution information (resolved_method, resolved_klass
>>> which are passed in) and then also the receiver - but that the selected_klass and selected_method
>>> would be calculated later?
>> 
>> I'm not sure what you mean.  CallInfo contains all the output from the LinkResolver requests, and this output has always included both linktime resolution and runtime selection information.
My oops. You are right.
>> 
>>> - or are these set for when there is no receiver but replaced if there is one, i.e. a non-null placeholder?
>>> Isn't this more confusing than leaving as null and for instance in runtime_resolve_interface_method,
>>> not checking for sel_method.is_null() if !check_null_and_abstract?
>> 
>> I also find this confusing, but it is a long-time feature of LinkResolver.  That is, LinkResolver calls produce both kinds of information.  Shall we file a cleanup bug for this?
>> 
>> — John
>