RFR add lworld specific value type checks to classFileParser

[Karen Kinnear]

Harold,

Thanks for the updates and sharing the webrev.

#1: it is not the case that “Arrays are always flattenable”. It is the case that “Arrays of value classes are always flattenable”.
Would you mind changing that again?
#3 - thank you for the comment update
#4 - looks good.

thanks,
Karen

p.s. I don’t need to see it again :-)


> On Mar 22, 2018, at 10:08 AM, harold seigel <harold.seigel at oracle.com> wrote:
> 
> Hi Karen,
> 
> Thanks for reviewing this!
> 
> Please review this updated webrev.  It addresses the comments 1, 3, and 4 that you made below.
> http://cr.openjdk.java.net/~hseigel/valueTypes_lworld.cfp.2/webrev/index.html <http://cr.openjdk.java.net/~hseigel/valueTypes_lworld.cfp.2/webrev/index.html>
> Additionally, I opened bug JDK-8200069 <https://bugs.openjdk.java.net/browse/JDK-8200069> for comment 2.
> 
> 
Thank you - I like Mr Simms’ approach to filing rfes and bugs with the lworld label.
> Thanks, Harold
> 
> On 3/21/2018 2:48 PM, Karen Kinnear wrote:
>> Harold,
>> 
>> Thank you so much for all of these changes.Code looks good. Glad jcod worked for you.
>> 
>> Minor comments:
>> 1. classFileParser.cpp line 1622 “Arrays are not flattenable” -> actually value type arrays
>> are always flattenable with our prototype, so perhaps “Value Type arrays are always flattenable, ACC_FLATTENABLE not allowed”
>> or something. Would require a change to line 7 in the test as well
>> 
>> 2. Thanks for commenting out the <init> is not ok in a value class - could you possibly file an rfe
>> so we remember to clean this up when this is decided?
>> 
>> 3. lines 4013-4016
>> Thank you for the comments as to why we can check flattenable_field circularity and superclass circularity 
>> independently.
>> Could you also add a comment as to why we can check flattenable_field circularity and superinterface
>> circularity separately? That is because interfaces can not be value types.
>> 
>> 4. systemDictionary.cpp
>> resolve_flattenable_field_or_fail
>>   I don’t think you need to do the register_loader here - I think resolve_or_fail will do that for you - but
>> you can double-check with Coleen.
>> 
>> thanks!
>> Karen
>> 
>> 
>> 
>>> On Mar 19, 2018, at 4:11 PM, harold seigel <harold.seigel at oracle.com> <mailto:harold.seigel at oracle.com> wrote:
>>> 
>>> Hi,
>>> 
>>> Please review the following webrev:
>>> 
>>>   http://cr.openjdk.java.net/~hseigel/valueTypes_lworld.cfp.Mar19/ <http://cr.openjdk.java.net/~hseigel/valueTypes_lworld.cfp.Mar19/>
>>> 
>>> The webrev adds checks to the JVM classFileParser.cpp source for the following:
>>> 
>>> * A class with ACC_VALUE and ACC_INTERFACE or ACC_ABSTRACT or ACC_ENUM
>>>   is invalid
>>> * java.lang.Object is the only valid super type for value types
>>> * A class with ACC_VALUE must be final
>>> * All instance fields in an ACC_VALUE class must be marked ACC_FINAL
>>> * A class with ACC_VALUE cannot have a method called <init>
>>> * A class with ACC_VALUE cannot have any synchronized instance methods
>>> * A field marked ACC_FLATTENABLE cannot be an array
>>> 
>>> Also, all ACC_FLATTENABLE fields are now pre-loaded and checked for circularity.
>>> 
>>> (Note that the change to test ValueOops.java is just to remove a <tab> character.)
>>> 
>>> The webrev was tested with hotspot/jtreg tests and many jdk/jtreg tests.
>>> 
>>> Thanks, Harold
>