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

[Karen Kinnear]

Code looks good to push. Thank you for the explanations. 

thanks,
Karen

> On Sep 7, 2017, at 12:27 PM, Lois Foltan <lois.foltan at oracle.com> wrote:
> 
> 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
>>> 
>>> 
>