RFR JDK-8233527: Update Lookup::hasPrivateAccess and Lookup::defineClass spec w.r.t. full power lookup

[Mandy Chung]

On 11/15/19 1:32 AM, Peter Levart wrote:
> Hi Mandy,
>
>>
>> http://cr.openjdk.java.net/~mchung/jdk14/8233527/webrev.03/
>
> In Lookup's findBoundCallerClass and checkSecurityManager the javadoc 
> is still talking about private access only although the checks are now 
> made against full privilege access.
>

Fixed.  @return in MethodHandles::lookup is also fixed.

>
>
> Some possible nano optimization / simplification suggestions. Take 
> each with a grain of salt...
>
>
>         public Class<?> defineClass(byte[] bytes) throws 
> IllegalAccessException {
>             if (allowedModes != TRUSTED && !hasFullPrivilegeAccess()) {
>
>
> TRUSTED == -1, which has all bits set. Therefore allowedModes == 
> TRUSTED implies hasFullPrivilegeAccess(). In other words, 
> !hasFullPrivilegeAccess() implies allowedModes != TRUSTED. Above 
> condition could be simplified to:
>
> if (!hasFullPrivilegeAccess()) { ...
>

OK with me.

>
> Please correct me if I'm wrong: 

These are interesting and I don't have the answer to final fields in 
Lookup class.  Maybe someone may chime in here.   Otherwise I will find out.

> All final instance fields in java.lang.invoke package are treated as 
> though they were annotated with @Stable right? If that is the case, 
> then all these checks will be folded into a constant if Lookup 
> instance can be folded into a constant when JITed, since allowedModes 
> field is final. But anyway, less branches in code sometimes makes code 
> faster. In this respect, the following:
>
>         public boolean hasFullPrivilegeAccess() {
>             return (allowedModes & PRIVATE) != 0 && (allowedModes & 
> MODULE) != 0;
>         }
>
> ...could also be written as:
>
>         public boolean hasFullPrivilegeAccess() {
>             return (allowedModes & (PRIVATE | MODULE)) == (PRIVATE | 
> MODULE);
>         }
>
> So it's just a matter of taste probably which one is nicer to read.
>

I did consider both and no strong preference.  No objection to use the 
latter.

> Also in the following part:
>
>         void checkSecurityManager(Class<?> refc, MemberName m) {
>             SecurityManager smgr = System.getSecurityManager();
>             if (smgr == null)  return;
>             if (allowedModes == TRUSTED)  return;
>
> ...the allowedModes == TRUSTED may be folded into a constant, while 
> security manager check above can not. Perhaps JIT is smart enough to 
> reorder these two checks since they both have the same path (return), 
> but if it doesn't, reordering them in code might be more optimal:
>
>         void checkSecurityManager(Class<?> refc, MemberName m) {
>             if (allowedModes == TRUSTED)  return;
>             SecurityManager smgr = System.getSecurityManager();
>             if (smgr == null)  return;
>

Regarding the performance difference, I think moving TRUSTED check to 
the top is fine as it clearly will skip SM check.

http://cr.openjdk.java.net/~mchung/jdk14/8233527/webrev.04

Mandy