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

[John Rose]

On Sep 5, 2013, at 10:00 AM, David Chase <david.r.chase at oracle.com> wrote:

> Latest webrev: http://cr.openjdk.java.net/~drchase/8014013/webrev.04/

I like the way the comments are shaping up.  One of my hopes for this change has been to illuminate the "dark corners" of the code.  (Some dark corners will always be necessary, to implement the tricky parts of the JVM spec.)

> On 2013-08-26, at 1:22 PM, Karen Kinnear <karen.kinnear at oracle.com> wrote:
> 
>> couple of questions:
>> 1. In init_method_MemberNames
>> The comment "interfaces are interconvertible" is no longer true with abstract methods.
>> The next sentence I believe is true, so perhaps you could delete the first one
> 
> Sentence deleted.

Good.

>> 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.)

> 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:

>  // 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).

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.

>  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.

>  - 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