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

[Karen Kinnear]

David,

In general, I really like the cleanup - I like the CallInfo usage and the relying on the
linkResolver logic in a single location. These are all requests for improved comments,
except for the last one, which is of course a personal preference, so ok to check in without
changing.

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

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.

And yes, you can have an overpass method, which does not require a vtable entry,
so the "can this happen?" answer is yes - so you can remove the question.

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)

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

6. linkResolver.cpp
CallInfo::CallInfo:
  Why are _selected_klass and _selected_method set to resolved_klass and resolve_method?
  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?

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

thanks,
Karen


On Aug 22, 2013, at 5:04 PM, David Chase wrote:

> After further study, we think the different-wrong-answers are a symptom of a different bug
> (to be filed) where Methodhandle processing does the wrong thing (fails to null-check or
> do appropriate handshakes) when presented with empty (null) method from a virtual or
> interface invocation.
> 
> So, if I may have another Reviewer (I think one more is needed for something this large)?
> 
> On 2013-08-19, at 6:46 PM, David Chase <david.r.chase at oracle.com> wrote:
> 
>> Karen Kinnear would like me to study, carefully, the wrong answer that occur in one of the default methods tests.
>> Apparently this is an instance of a test where the wrong answers are still interesting -- this patch fails less, but
>> in a few cases it fails differently.
>> 
>> David
>> 
>> On 2013-08-19, at 6:15 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>> 
>>> Looks good.  -- Chris
>>> 
>>> On Aug 19, 2013, at 11:13 AM, David Chase <david.r.chase at oracle.com> wrote:
>>> 
>>>> New version, with Christian's and John's fixes (and limited use of err_msg_res):
>>>> 
>>>> http://cr.openjdk.java.net/~drchase/8014013/webrev.03/
>>>> 
>>>> retested (jtreg) on Mac against hotspot/test/compiler, jdk/test/{jdk,java/util}
>>>> 
>>>> David
>>>> 
>>>> 
>>>> On 2013-08-19, at 12:04 AM, David Holmes <david.holmes at oracle.com> wrote:
>>>> 
>>>>> On 19/08/2013 8:24 AM, David Chase wrote:
>>>>>> A question about err_msg_res -- should that generally be used in place of err_msg,
>>>>>> or only in certain source directories, and if so, which ones?
>>>>> 
>>>>> I can't answer that but be aware that there is an issue with using err_msg_res as it is unclear as to whom has responsibility for ensuring there is a ResourceMark on the stack. There's an open bug on that - 8022187.
>>>>> 
>>>>> David
>>>>> 
>>>>>> On 2013-08-17, at 1:41 AM, John Rose <john.r.rose at oracle.com> wrote:
>>>>>> 
>>>>>>> On Aug 16, 2013, at 1:05 PM, David Chase <david.r.chase at oracle.com> wrote:
>>>>>>> 
>>>>>>>> How does this look?
>>>>>>>> 
>>>>>>>> void fieldDescriptor::reinitialize(InstanceKlass* ik, int index) {
>>>>>>>> if (_cp.is_null() || field_holder() != ik) {
>>>>>>>> _cp = constantPoolHandle(Thread::current(), ik->constants());
>>>>>>>> // _cp should now reference ik's constant pool; i.e., ik is now field_holder.
>>>>>>>> assert(field_holder() == ik, "must be already initialized to this class");
>>>>>>> 
>>>>>>> Good.
>>>>>>> 
>>>>>>> As a side note, this bit of code (and many others) assume that there is a 1-1 correspondence between constant pools and instance klasses.
>>>>>>> 
>>>>>>> I hope we can change this some day; the tracking bug is http://bugs.sun.com/view_bug.do?bug_id=6711913
>>>>>>> 
>>>>>>> — John
>>>>>> 
>>>> 
>>> 
>> 
>