RFR (M): 8182367: [MVT] Byte-code verifier support for minimal value types

[Lois Foltan]

On 9/6/2017 12:42 PM, Karen Kinnear wrote:
> Lois,
>
> Looks good. Many thanks.

Hi Karen,
Thank you for the review!  Comments below.

>
> Very happy with the creative classFileParser containment of the temporary derived value class naming
> and with the clean verificationType extension.
>
> Couple of questions/comments:
> 1. verificationType.hpp
> line 180
> Did I read this incorrectly? There is a Symbol alignment assumption that leaves 0x3, i.e. two bits as 0
> Did you want to change that alignment check to be 0x7 for alignment so there is room for the ValueType bit?
> And change the hardcoded 0x3 to TypeMask?
Agreed, fixed.

>
>
> 2. verificationType.hpp
> line 203
> Could you possibly explain to me the need for the && !is_valuetype_check()? Or is that potentially an assertion?
So this might be a bit defensive on my part.  But, I don't think there 
is a guarantee that is_reference() is necessarily always called with a 
non-check VerificationType.  As a matter of fact, I know there are cases 
where is_reference is called specifically with a TypeQuery (0x03) check 
VerificationType because I had added a more general assertion initially. 
Something like --> assert(!is_check(), "is_reference called with a check 
VerificationType");

This assert is hit right away when building the JDK, again called with a 
TypeQuery (0x03).  So I became concerned that if is_reference() is 
called with a ValueTypeQuery check VerificationType it would erroneously 
return true.

is_reference() --> { return (ValueTypeQuery (0x08) & TypeMask (0x07)) == 
Reference (0x00)); }

Thus the need for !is_valuetype_check().

>
> 3. verificationType.hpp
> is_valuetype_assignable_from
>    Is it worth adding an assertion that neither is_null() nor from.is_null() ? Or did you already throw an exception earlier for that
> case?
If either is_null() or from.is_null() is true then the current assertion 
that checks for is_valuetype() and from.is_valuetype() will catch that.

>
> 4. We discussed temporarily using 53.1 for value type versioning until JDK10 moves to 54
I have changed VALUETYPE_MAJOR_VERSION to 53 and VALUETYPE_MINOR_VERSION 
remains at 1.  However, tests are demonstrating that we are still trying 
to generate 52.0 & 53.0 version classfiles with vbytecodes.  I have 
entered bug https://bugs.openjdk.java.net/browse/JDK-8187320 to address  
this issue.  Once resolved, we can comment out the check in verifier.cpp.

>
> 5. verifier.cpp
> Thank you for the vbytecodes_allowed check. I see the special checking for EnableValhalla and invoke virtual overloading.
> Did you want to do specific checking of EnableMVT for vbox/vunbox?
Done.

>
> 6. verifier.cpp
> multianewarray
> line 1863:   there is a verify_cp_class_type — I was expecting verify_cp_class_or_value_type?
Good catch, fixed.

>
> 7. VunboxErrorIndex and VboxUnbox.java
> Cleanup. Perhaps remove comments in former lines 98/99 - since they are no longer true, and same lines in original test.
> Ok to file as a follow-on rfe
Done.

>
> I don’t need to see an update, just answers to the questions to make sure I understand,

Just in case, updates posted at:
http://cr.openjdk.java.net/~lfoltan/mvt_verifier_hotspot.2/webrev/
http://cr.openjdk.java.net/~lfoltan/mvt_verifier_jdk.2/webrev/

Thanks,
Lois

> thanks,
> Karen
>
>
>> On Sep 5, 2017, at 8:37 PM, Lois Foltan <lois.foltan at oracle.com> wrote:
>>
>> Hello,
>>
>> Review request for verifier changes to support MVT.
>>
>> Webrevs:
>>      http://cr.openjdk.java.net/~lfoltan/mvt_verifier_jdk.1/webrev/
>> http://cr.openjdk.java.net/~lfoltan/mvt_verifier_hotspot.1/webrev/
>>
>> Webrevs include changes to address preliminary review comments by John Rose and Harold Seigel.
>>
>> Testing - hotspot jtreg tests, java/lang, java/util, java/io, JDK & Hotspot valhalla tests, jprt hotspot tests, JCK vm (in progress).
>>
>> Thanks,
>> Lois
>>
>>