[lworld] Handling of missing ValueTypes attributes

Fri Jul 13 12:10:24 UTC 2018

Hi Ioi,

good catch! I've replied in detail in the bug comments.

Thanks,
Tobias

On 13.07.2018 03:12, Ioi Lam wrote:
> I have a test case where compiled code fails *even* if we have the consistency check. It's due to
> inlining.
> 
> Details in https://bugs.openjdk.java.net/browse/JDK-8207219
> 
> Note that my test program is "consistent" because there are no cross-class references that involve
> Value Types.
> 
> So far the crash happens only with my patch for JDK-8206140 (Move return value null checks into the
> callee). However, I am not sure if there are other assumptions in the JIT may be broken due to
> inlining. Anyway, I think we should avoid inlining in this situation.
> 
> Thanks
> 
> - Ioi
> 
> 
> 
> On 7/11/18 12:42 PM, Karen Kinnear wrote:
>> Ioi, Tobias,
>>
>> You asked a good question about MethodHandle and Reflection. I also checked out JNI. This is my
>> understanding - please correct me
>> if my assumptions are inaccurate.
>>
>> Here are my assumptions about when we need value type consistency
>> 1. verifier - mismatches between bytecodes and type (within a classfile, no load/check for actual
>> value type)
>> 2. JIT performance optimizations
>>     2a. flattening in containers - flattenable fields and array - require check of value type vs.
>> ACTUAL loaded type
>>           - this is easy, we preload types
>>
>>     2b. JIT scalarization of field access - must be an ACTUAL value type and must be flattenable
>>        This will only work for fields that the JIT’d caller believes are value types, the declarer
>> believes are value types and the declarer does an ACTUAL check
>>        Need caller-callee agreement for a JIT’d caller.
>>
>>     2c. JIT calling convention - scalarization of arguments
>>        Need either the caller-callee in agreement if both compiled OR
>>        For caller calls by reference, adapter that can scalarize arguments it knows are ACTUAL
>> value types
>>        Today adaptor is created at callee link time, so we explicitly load types in local methods
>> in the ValueTypes attribute so they can be scalarized
>>   
>>      2d. JIT returning a value type
>>         I do not know our plans for value type return optimizations.
>>         The adaptor for returns are stored off of the return type, so they know the ACTUAL value.
>>         In general we can check caller-callee consistency so we can be in agreement about whether
>> a type is a value type.
>>         The exception is the JavaCalls::call_helper path used by Reflection, jni (and others
>> internally)
>>             - I assume we will always return a reference here (I have not studied the details yet,
>> so I don’t know where that is handled)
>>         
>> Details:
>> 1. MethodHandles - invocation and field access always goes through LinkResolver at this point.
>> There are two exceptions here:
>>     - one is when the MethodHandle creation does NOT pass in the calling class information
>>       - in that case there is no check for caller-callee consistency, we need to look at this
>> independently
>>     - one is invokespecial indirect superclass (ACC_SUPER) which performs selection in the java code.
>>        - That is a rathole I won’t follow here - we should fix that anyway - multiple potential
>> approaches.
>>
>> 2. Reflection:
>>    optimized reflection generates bytecodes, so goes through bytecode path, so goes through
>> LinkResolver.
>>    initial reflection calls JavaCalls::call->JavaCalls::call_helper
>>
>> 3. JNI:
>>    also goes through JavaCalls::call_helper
>>
>> JavaCalls::call_helper calls call_stub to invoke the entry_point which is:
>>    normally: method->from_interpreted_entry
>>    debug: method->interpreter_entry
>>
>> For argument passing, my assumption is that we are ok with the JavaCalls::call_helper path because
>> it always passes by reference
>> and uses the callee adapter from interpreter which knows the declared value types that can be
>> scalarized. So the same adaptor that works for
>> interpreted code works for call_helper where the caller always assumes everything is a reference
>> and passes by reference.
>>
>> JIT folks - does this work in practice?
>>
>> thanks,
>> Karen
>>
>>
>>
>>> Thanks Karen for the explanation. I think it will simplify my patch for JDK-8206140 [lworld] Move
>>> return value null checks into the callee.
>>>
>>> If I understand correctly, this means that when we try to resolve a ClassRef, FieldRef,
>>> MethodRef, ..., in the constant pool, and there's a cross-class mismatch of ValueType, the
>>> constant pool entry will fail to resolve, and an ICCE will be thrown.
>>>
>>> This means the compiler should never see a mismatched cross-class reference. For example,
>>>
>>> class ClassB {
>>>     void foo(Point p) {
>>>          ClassA.m1(p);  // ....., invokestatic MethodRef #4 = ClassA.m1:"(LPoint;)V"
>>>     }
>>> }
>>>
>>> When foo is compiled, the compiler will see that the constant pool entry MethodRef #4 is
>>> unresolved. Thus, the compiler will generate an uncommon trap and let the interpreter handle it.
>>> The interpreter will try to resolve #4 again, resulting in an ICCE.
>>>
>>> BTW, value-types-consistency-checking-details.pdf doesn't mention MethodHandle and Reflection
>>> explicitly. However, I assume that the same set of rules applies as well?
>>>
>>> Thanks
>>> - Ioi
>>>
>>>> From an execution perspective, we would like to ensure that neither ClassC nor ClassA can get their hands on an instance of Point, so
>>>> we are only passing null here. Tried to close all of these holes - could use review. Note to John - consistency checking for array elements
>>>> relative to ValueTypes attribute is part of closing this hole, i.e. ensuring that a class that has the wrong information about whether a type
>>>> is a value type can not get their hands on an instance of that value type.
>>>>
>>>> Does this make sense for LW1?
>>>>
>>>> thanks,
>>>> Karen
>>>>
>>>> p.s. Frederic checked in round 1 of the consistency checking - but did not get round 2 in before he left for vacation. He is out this
>>>> week and the next. I attached his patch out for review if you want to use it to test with to see if that helps the compiler. He will be back I
>>>> believe the 23rd. I will check with Harold on Frederic’s verifier question - we may want to push this (without perhaps the new test) before
>>>> he gets back so you can use it.
>>>>
>>>> Note - John asked for some refactoring - given how tight the time is before EA and the vacation schedule - that will be a postlw1 rfe.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>> On Jul 10, 2018, at 8:22 AM, Tobias Hartmann <tobias.hartmann at oracle.com> wrote:
>>>>>
>>>>> Hi John,
>>>>>
>>>>> On 10.07.2018 02:08, John Rose wrote:
>>>>>> On Jul 9, 2018, at 3:44 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>>>>>> In that case, I think the program's output should be the same as if the ValueTypes attribute had been present, although performance may differ (slower, more heap allocations, etc).
>>>>>>>
>>>>>>> Is this understanding correct?
>>>>>> Yes, that is correct.  This model is intended to make it easier for old-school classfiles
>>>>>> to link to old types which have (over time) been upgraded to value types.
>>>>> How do we then handle the following scenario?
>>>>>
>>>>> We have a method m1(MyValue vt) in a class that has no value types attribute set although MyValue is
>>>>> a value type. Once the calling convention for that method is determined (i.e. at adapter creation),
>>>>> we therefore don't know that MyValue is a value type and as a result, m1 will expect vt to be passed
>>>>> as oop.
>>>>>
>>>>> Now another compiled method m2 that calls compiled m1 might be well aware that vt is a value type
>>>>> but has no way to know that m1 does *not* expect vt to be passed as fields (especially if the call
>>>>> is virtual).
>>>>>
>>>>> Thanks,
>>>>> Tobias
>>>
>>
> 