[lworld] Handling of missing ValueTypes attributes

[Ioi Lam]

I found two cases where the current repo doesn't generate ICCE as expected:

https://bugs.openjdk.java.net/browse/JDK-8208090
     [lworld] MethodHandle invocation should check for ValueType consistency

https://bugs.openjdk.java.net/browse/JDK-8207788
     [lworld] multinewarray bytecode should check for ValueType 
inconsistency

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