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

[Mandy Chung]

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