Request for review: 8015385 Remove RelaxAccessControlCheck for JDK 8 bytecodes

[Karen Kinnear]

Harold, David,

Code looks good. I would go with the way this is written.

I could argue you either direction in terms of which matters most - accessor or accessee version number
  accessor - the older bytecode accessor will be surprised with the change in behavior
    - the original point of the flag was to avoid surprise if the accessee changed without the accessor knowing
    - if the accessor rebuilds they have a chance to fix the problem, so that would argue for the accessor version check

  accessee - the newer code wants to enforce access checking - that is why we have the issue to start with 
    - so they would expect their new change to actually make a difference

I think following the model the flag was already using will be the least confusing to customers. We will need
to document this.

I think we gave the flag to give a breathing period for folks to make the appropriate code changes. Classfile 49
to 52 seems like long enough to make the transition. There is still a workaround -Xverify:none, so there is a
backup plan in the field.

I also read the code as having the logic we want.

thanks,
Karen

On May 31, 2013, at 9:14 AM, harold seigel wrote:

> Hi David,
> 
> Thanks for the review.  Please see inline comments.
> 
> Thanks, Harold
> 
> On 5/31/2013 3:31 AM, David Holmes wrote:
>> Hi Harold, 
>> 
>> I'm not sure what the actual rules for this should be in terms of the accessor and accessee having different versions. But isn't the main issue the version of the accessee? 
> I'm also not sure what the actual rules should be.  I chose the stricter path of requiring the access check if either class is >= 52 in part because the existing checks for pre 49 class files require both accessor and accessee to be < 49.  If people feel strongly I can change it.  
>> 
>> Further your change will now require RelaxAccessControlCheck  be true for pre 49 class files, but I thought this old code was supposed to continue working? 
> Here's the code change using numbers instead of enums to make it easier to see the parentheses.  I think it shows that RelaxAccessControlCheck is not needed for pre 49 class files.
> 
>   if ((RelaxAccessControlCheck && accessor_ik->major_version() < 52 && accessee_ik->major_version() < 52) ||
>       (accessor_ik->major_version() < 49 && accessee_ik->major_version() < 49)) {
>     return classloader_only &&
>       Verifier::relax_verify_for(accessor_ik->class_loader()) &&
>       accessor_ik->protection_domain() == accessee_ik->protection_domain() &&
>       accessor_ik->class_loader() == accessee_ik->class_loader();
>   } else {
>     return false;
>   }
> }
> 
>> 
>> Thanks, 
>> David 
>> 
>> On 29/05/2013 11:25 PM, harold seigel wrote: 
>>> Hi, 
>>> 
>>> Please review this fix for bug 8015385. 
>>> 
>>> This fix removes the use of the RelaxAccessControlCheck option for 
>>> bytecode versions >= 52 and so requires the stricter access checking 
>>> that previously could be avoided by using this option. Note that the 
>>> original purpose of this option was for use with pre-Java_1_5 bytecodes 
>>> that did not contain the necessary accessors needed to properly access 
>>> private fields. 
>>> 
>>> This change was tested with JCK lang and vm tests, UTE vm.quick.testlist 
>>> and vm.mlvm.testlist tests, JPRT, and jtreg tests. Tests were run on 
>>> Linux and Solaris. 
>>> 
>>> Open webrev at http://cr.openjdk.java.net/~hseigel/bug_8015385/ 
>>> <http://cr.openjdk.java.net/%7Ehseigel/bug_8015385/> 
>>> 
>>> Bug link at http://bugs.sun.com/view_bug.do?bug_id=8015385 
>>> 
>>> Thanks, Harold 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130531/ce3e3ff0/attachment.html