(Nestmates) RFR (M): 8240645: Update nest host determination in line with latest proposed JVMS changes for JEP-371

[David Holmes]

Hi Mandy,

Thanks for taking a look. Please see response to John for updated webrevs.

David

On 7/03/2020 4:23 am, Mandy Chung wrote:
> 
> Hi David,
> 
> On 3/5/20 9:29 PM, David Holmes wrote:
>> webrev: http://cr.openjdk.java.net/~dholmes/8240645/webrev/
>> bug: https://bugs.openjdk.java.net/browse/JDK-8240645
>>
> 
> This patch looks good to me.
> 
> Thanks for the detail below which is very helpful.  It's okay with me 
> that IAE thrown by core reflection and lookup MethodHandle is not 
> extended with the memoized nest host error message in this release. We 
> could improve it in the future.
> 
> Mandy
>> JEP-371 (Hidden Classes) is changing a key aspect of nest host 
>> resolution and validation compared to what we shipped originally for 
>> JEP-181 (Nestmates). Originally, any LinkageErrors during nest host 
>> resolution would be propagated, and any validation failures would 
>> result in IncompatibleClassChangeError being thrown. Now no 
>> LinkageErrors are thrown, but the fact one occurred is remembered for 
>> potential use if a private access check fails. Similarly, validation 
>> failures don't cause an exception any more. Nest host resolution 
>> always succeeds and the nest will be one of:
>> - the successfully resolved nest host as per the NestHost attribute
>> - the class itself if there is no NestHost attribute or there is an 
>> error during resolution or validation
>> - the specified nest host explicitly set if this is a hidden class 
>> injected into a nest
>>
>> as appropriate.
>>
>> This has a flow on affect to the reflection method 
>> Class::getNestMembers() which can also no longer ever fail.
>>
>> To be able to diagnose issues with nest host resolution/validation you 
>> can use unified logging to examine what happens: 
>> -Xlog:class+nestmates=trace.
>>
>> In addition IllegakAccessErrors thrown by the VM are augmented with 
>> information about any nest host resolution/validation errors. For 
>> example an original exception:
>>
>> java.lang.IllegalAccessError: class TestNestmateMembership$Caller 
>> tried to access private method 'void 
>> TestNestmateMembership$TargetSelfHost.m()' 
>> (TestNestmateMembership$Caller and 
>> TestNestmateMembership$TargetSelfHost are in unnamed module of loader 
>> 'app')
>>
>> is now expanded with an additional "(<nest host resolution error(s)>)" 
>> section which is memoized for use with other access checks:
>>
>> java.lang.IllegalAccessError: class TestNestmateMembership$Caller 
>> tried to access private method 'void 
>> TestNestmateMembership$TargetSelfHost.m()' 
>> (TestNestmateMembership$Caller and 
>> TestNestmateMembership$TargetSelfHost are in unnamed module of loader 
>> 'app')(Type TestNestmateMembership$TargetSelfHost (loader: 'app') is 
>> not a nest member of type TestNestmateMembership$TargetSelfHost 
>> (loader: 'app'): current type is not listed as a nest member)
>>
>> These exception messages are very long and unweildy but that seems 
>> unavoidable. They appear repetative in that the type names appear in 
>> the main message, the module information, and then again in the 
>> resolution error section. Unfortunately we can't avoid that as the 
>> memoized string has to be complete because it does not know the 
>> context of the IllegalAccessError in which it will be used. Please 
>> look at the mechanism by which this is done before making suggestions 
>> on how to condense.
>>
>> Also note that in the worst case there can be two nest host resolution 
>> errors reported, if both the current class and the target class had 
>> such errors.
>>
>> Note: the augmented exceptions do not extend to 
>> IllegalAccessExceptions thrown by the Java reflection or MethodHandle 
>> code. This would require an additional VM call to retrieve the 
>> information. This may be considered for a future RFE.
>>
>> Changes in detail:
>>
>> - src/hotspot/share/ci/ciField.cpp
>> - src/hotspot/share/classfile/classFileParser.cpp
>> - src/hotspot/share/runtime/reflection.cpp
>>
>> No longer a possibility of exceptions
>>
>> -  src/hotspot/share/interpreter/linkResolver.cpp
>>
>> Expands on the message for IllegalAccessErrors in the private case, to 
>> report any nest host resolution/validation errors.
>>
>> -  src/hotspot/share/oops/instanceKlass.cpp
>>
>> Implements the new specification for resolving the nest host, and adds 
>> the means to memoize the error message. The same message is used for 
>> exceptions and logging.
>>
>> Exception handling is changed as needed - no use of CHECK macros.
>>
>> Removed a case in has_nest_member where we would bail out if executing 
>> in a compiler thread and encountered an unresolved class entry in the 
>> CP. We previously bailed out as a precaution as the compiler thread 
>> can't load classes. I was surprised to find that I was hitting this 
>> code, and getting runtime failures because of it (not test failures). 
>> I don't see how the current changes would cause this. In any case the 
>> situation where actual class loading would be needed should be 
>> impossible - two classes of the same name within the same loader - so 
>> the bail out can be removed.
>>
>> Removed the recently added runtime_nest_host code which is no longer 
>> needed.
>>
>> -  src/hotspot/share/prims/jvm.cpp
>>
>> Some code simplifies now exceptions are no longer possible.
>>
>> JVM_GetNestMembers has major updates:
>> - as we no longer bail out on error we may end up with holes in the 
>> array, so we have to copy across to an array of the exact size before 
>> returning.
>> - added logging to aid in debugging
>>
>> -  src/hotspot/share/utilities/ostream.cpp
>>
>> Needed to add the ability for stringStream to return a C-Heap 
>> allocated buffer so we could use it for the memoized error messages. 
>> (This should probably be factored out to a separate RFE for mainline.)
>>
>> - src/java.base/share/classes/java/lang/Class.java
>>
>> Updated the specifications for getNestHost and getNestMembers as per 
>> Mandy's proposed spec updates (added missing @jvms links in 
>> getNestMembers()). Adjusted the implementation for the fact there are 
>> no longer any exceptions from the VM.
>>
>> - 
>> test/hotspot/jtreg/runtime/Nestmates/membership/TestNestmateMembership.java
>>
>> Had to change all the LinkageErrors/NoClassDefFoundErrors to be 
>> IllegalAccessError or IllegalAccessException and adjust the expected 
>> messages to check the underlying failure reasons.
>>
>> Other test adjusted as needed to account for no exceptions, and the 
>> changes to Class::getNestMembers().
>>
>> Testing: all nestmate and hidden class tests under normal conditions.
>>          tiers 1 - 3
>>
>> Thanks,
>> David
>