[Nest mates] Moves nestmates host loading to visibility check #1224

taliamccormick · 2018-02-20T19:34:46Z

Earlier nest mates spec draft loaded a class's nest host during class loading. More recent nest mates spec puts off host loading until required for an access check. This commit moves the nest host loading and verification to the visibility check.

The spec draft for checking visibility states:

To determine whether a class or interface C belongs to the same nest as
D (that is, whether C and D are nestmates), the following steps are
performed:

If C and D are the same class or interface, they belong to the same nest.

Otherwise, the nest host of D, H, is determined. If an exception is thrown, the nest membership test fails for the same reason.

Otherwise, the nest host of C, H', is determined. If an exception is thrown, the nest membership test fails for the same reason.

Otherwise, C and D belong to the same nest if H and H' are the same class or interface.

To determine the nest host of a class M, the following steps are performed:

If M lacks a NestHost attribute (4.7.28), M is its own nest host.

Otherwise, where i is the host_class_index item of M's NestHost attribute, the symbolic reference at index i of M's run-time constant pool is resolved to a class or interface H (5.4.3.1). Any of the exceptions pertaining to class or interface resolution can be thrown.

If resolution of H succeeds, but H is not declared in the same
run-time package as M, an IncompatibleClassChangeError is thrown.

Otherwise, if H lacks a NestMembers attribute (4.7.29), or if, where M has name N, there exists no entry in the classes array of the NestMembers attribute of H that refers to a class or interface with name N, an IncompatibleClassChangeError is thrown.

Otherwise, H is the nest host of M.

As taken from here: http://cr.openjdk.java.net/~dlsmith/nestmates.html

Signed-off-by: Talia McCormick Talia.McCormick@ibm.com

taliamccormick · 2018-02-20T19:37:58Z

@tajila

tajila · 2018-02-20T19:49:04Z

runtime/vm/visible.c

+		}
+
+		/* If any problem occurred in nest host verification, mask 0th bit */
+		if (J9_VISIBILITY_ALLOWED != result) {


do you still need this if you are returning the J9_VISIBILITY_NEST* error code?

I need some way of identifying which class - destClass or sourceClass - causes the nest verification error. I thought that the best way to do so was flag the 0 bit. A quick overview of the other options that I saw and my reasoning is below. Am open to other ideas/suggestions - having a non-functional non-null pointer may not be a good idea

Options:

Separate error codes for nest host errors in destClass & sourceClass

Changing function headers to pass a boolean/flag/other parameter to indicate nest host errors in either destClass or sourceClass

Flagging the 0th bit on the address

Putting a non-address value on the nest host (eg. -1)

Making error messages non-dependent on which classes have issues

Issues:

On a conceptual level, nest loading errors in either class would raise the same type of error, but nothing explicitly wrong with it (other than longer if-statements).

Creates excessive changes to existing code.

Equivalent to this method, but would require searching for a class if later changes to the code want to access the nest host.

Risks that later changes to the code try to use the address to access nest host without masking the 0 bit; may cause confusion later.

Need to know which class has nest loading issue, in order to raise accurate errors.

As discussed yesterday, new/current approach is:

Create two sets of visibility error codes; one set for nest visibility/verification issues in the source class and one set for nest visibility/verification issues in the destination class

Shift the value of these visibility error codes such that they can be separated from the non-nest related error codes with a single comparison and that the source class codes and destination class codes can be separated from eachother via a single bitwise comparison.

eg.

Visibility check result Value Hex value Last eight bits

J9_VISIBILITY_ALLOWED 1 1 0000 0001

J9_VISIBILITY_NON_MODULE_ACCESS_ERROR 0 0 0

J9_VISIBILITY_MODULE_READ_ACCESS_ERROR -1 FFFF FFFF 1111 1111

J9_VISIBILITY_MODULE_PACKAGE_EXPORT_ERROR -2 FFFF FFFE 1111 1110

J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR_SOURCE_CLASS -17 FFFF FFFF 1110 1111

J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR_TARGET_CLASS -18 FFFF FFEF 1110 1110

J9_VISIBILITY_NEST_HOST_DIFFERENT_PACKAGE_ERROR_SOURCE_CLASS -19 FFFF FFED 1110 1101

J9_VISIBILITY_NEST_HOST_DIFFERENT_PACKAGE_ERROR_TARGET_CLASS -20 FFFF FFEC 1110 1100

J9_VISIBILITY_NEST_MEMBER_NOT_CLAIMED_ERROR_SOURCE_CLASS -21 FFFF FFEB 1110 1011

J9_VISIBILITY_NEST_MEMBER_NOT_CLAIMED_ERROR_TARGET_CLASS -22 FFFF FFEA 1110 1010

Then we can detect a nest-related error via checking the range or a single bitwise comparison on the 5th- 8th last bits

if ((J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR_SOURCE_CLASS >= errorCode) && (J9_VISIBILITY_NEST_MEMBER_NOT_CLAIMED_ERROR_TARGET_CLASS <= errorCode)) { ... }

#define J9_VISIBILITY_NEST_CODE 240 // 0000 ... 0000 1111 0000 #define J9_VISIBILITY_NEST_MASK 224 // 0000 ... 0000 1110 0000 if (J9_VISIBILITY_NEST_CODE == (J9_VISIBILITY_NEST_MASK & errorCode)) { ... }

And we can also use the parity bit to detect if the source class or destination class is causing the error/exception

if (0 == errorCode % 2) { ... /* target class */ } if (1 == errorCode % 2) { ... /* destination class */ }

tajila · 2018-02-20T19:49:51Z

runtime/nls/j9vm/j9vm.nls

+# Second argument is class name
+# Third argument is nest host name length
+# Fourth argument is nest host name
+J9NLS_VM_NEST_HOST_FAILED_TO_LOAD=Class %2$.*1$s must be able to load its nest host %4$.*3$s.


do these messages matchup with the ones OpenJDK is using?

Some of them have different messages. The ones in which I add/directly use existing NLS messages, I will update to correspond with the ones OpenJDK is using.

What should I do for the cases in which existing error messages are different? By which I mean, the cases I did not add/change, eg. NoClassDefFound for testing the class loader case

I ran my updated set of test cases (currently found here: #1201) against both. For each test that was supposed to raise an error/exception, I printed: test case name + error/exception + cause of error/exception.

Relevant test cases for this PR are:

testDifferentClassLoadersWithAccess

testDifferentPackagesWithAccess

testNestMemberNotVerifiedWithAccess

testNesthostNotClaimedWithAccess

Ours:

testDifferentClassLoadersWithAccess java.lang.NoClassDefFoundError: FieldAccess$Inner java.lang.ClassNotFoundException

testDifferentPackagesWithAccess java.lang.IllegalAccessError: Class FieldAccess illegally accessing "package private" member of class pkg/FieldAccess$Inner null

testNestMemberNotVerifiedWithAccess java.lang.VerifyError: Class FieldAccess$Inner must be claimed by its nest host . null

testNesthostNotClaimedWithAccess java.lang.IllegalAccessError: Class FieldAccess illegally accessing "private" member of class FieldAccess$Inner null

OpenJDK:

testDifferentClassLoadersWithAccess java.lang.NoClassDefFoundError: FieldAccess$Inner java.lang.ClassNotFoundException: FieldAccess$Inner

testDifferentPackagesWithAccess java.lang.IllegalAccessError: failed to access class pkg.FieldAccess$Inner from class FieldAccess null

testNestMemberNotVerifiedWithAccess java.lang.IllegalAccessError: Type FieldAccess$Inner is not a nest member of FieldAccess: current type is not listed as a nest member null

testNesthostNotClaimedWithAccess java.lang.IllegalAccessError: tried to access field FieldAccess$Inner.f from class FieldAccess null

On a side note, how close do/should the parser errors be? I've copied the same output for those cases below and they are similar but not exactly the same.

This:

testMultipleNestMembersAttributes java.lang.ClassFormatError: JVMCFRE143 Multiple nest mates attributes; class=MultipleNestMembersAttributes, offset=166 null

testMultipleNestHostAttributes java.lang.ClassFormatError: JVMCFRE143 Multiple nest mates attributes; class=MultipleNestHostAttributes, offset=282 null

testMultipleNestAttributes java.lang.ClassFormatError: JVMCFRE143 Multiple nest mates attributes; class=MultipleNestAttributes, offset=188 null

OpenJDK:

testMultipleNestMembersAttributes java.lang.ClassFormatError: Multiple NestMembers attributes in class file MultipleNestMembersAttributes null

testMultipleNestHostAttributes java.lang.ClassFormatError: Multiple NestHost attributes in class file MultipleNestHostAttributes null

testMultipleNestAttributes java.lang.ClassFormatError: Conflicting NestHost and NestMembers attributes in class file MultipleNestAttributes null

We probably want to have a more specific error msg for these. Having more detail in the error message will make it easier to debug, but that doesn't have to be done in this PR

We don't want to copy the error messages. They should contain the same kind of information but shouldn't be copied from anywhere.

tajila · 2018-02-21T19:43:39Z

runtime/nls/j9vm/j9vm.nls

+# Second argument is class name
+# Third argument is nest host name length
+# Fourth argument is nest host name
+J9NLS_VM_NEST_HOST_FAILED_TO_LOAD=Class %2$.*1$s must be able to load its nest host %4$.*3$s.


We probably want to have a more specific error msg for these. Having more detail in the error message will make it easier to debug, but that doesn't have to be done in this PR

tajila · 2018-02-21T19:44:48Z

runtime/vm/lookupmethod.c

+		J9UTF8 *nestHostNameUTF = J9ROMCLASS_NESTHOSTNAME(romClass);
+
+		if ((J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR_SOURCE_CLASS == errorType)
+				|| (J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR_DEST_CLASS == errorType)){


needs space ){

also it is more clear like this:

if ((J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR_SOURCE_CLASS == errorType) || (J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR_DEST_CLASS == errorType) ) {

same for the rest.

tajila · 2018-02-21T20:05:38Z

runtime/vm/lookupmethod.c

+	 */
+	if ((J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR_SOURCE_CLASS <= errorType)
+			&& (J9_VISIBILITY_NEST_MEMBER_NOT_CLAIMED_ERROR_DEST_CLASS >= errorType)) {
+		if (0 == errorType % 2) {


a bit check might be less expensive then a modulo

tajila · 2018-02-21T20:07:18Z

runtime/vm/lookupmethod.c

+			&& (J9_VISIBILITY_NEST_MEMBER_NOT_CLAIMED_ERROR_DEST_CLASS >= errorType)) {
+		if (0 == errorType % 2) {
+			unverifiedNestMemberClass = targetClass;
+		} else if (1 == ((UDATA)targetClass->nestHost & 1)) {


do you still need to check this? if errorType is an odd value is the error always sender?

tajila · 2018-02-21T20:15:00Z

runtime/vm/visible.c

+		UDATA classLoadingFlags = J9_FINDCLASS_FLAG_THROW_ON_FAIL;
+		nestHost = vmThread->javaVM->internalVMFunctions->internalFindClassUTF8(vmThread, J9UTF8_DATA(nestHostName), J9UTF8_LENGTH(nestHostName), clazz->classLoader, classLoadingFlags);
+
+		/* Nest host must successfully loaded by the same classloader in the same package & verify the nest member */


Nest host must be successfully loaded by

tajila · 2018-02-21T20:22:48Z

runtime/vm/visible.c

+		/* If any problem occurred in nest host verification, error code was set
+		 * assuming that the nest host is being loaded for the source class; if
+		 * not, add a -1 offset to change to dest class error code */
+		if (TRUE != isSourceClass) {


Will this decrement result even if it is J9_VISIBILITY_ALLOWED

Missed that - thanks

tajila · 2018-02-22T18:20:19Z

runtime/vm/visible.c

+		/* If any problem occurred in nest host verification, error code was set
+		 * assuming that the nest host is being loaded for the source class; if
+		 * not, add a -1 offset to change to dest class error code */
+		if ((J9_VISIBILITY_ALLOWED == result) && (TRUE != isSourceClass)) {


should this be J9_VISIBILITY_ALLOWED != result

tajila · 2018-02-22T18:20:55Z

runtime/vm/lookupmethod.c

+		J9UTF8 *nestHostNameUTF = J9ROMCLASS_NESTHOSTNAME(romClass);
+
+		if ((J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR_SOURCE_CLASS == errorType)
+				|| (J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR_DEST_CLASS == errorType)


it looks like you have an extra tab here.

same below

tajila · 2018-02-22T18:21:56Z

runtime/vm/resolvesupport.cpp

@@ -678,8 +678,8 @@ resolveStaticFieldRefInto(J9VMThread *vmStruct, J9Method *method, J9ConstantPool
 						} else {
 							errorMsg = illegalAccessMessage(vmStruct, -1, classFromCP, targetClass, checkResult);
 						}
-						setCurrentExceptionUTF(vmStruct, J9VMCONSTANTPOOL_JAVALANGILLEGALACCESSERROR, errorMsg);
-						j9mem_free_memory(errorMsg);
+					setCurrentExceptionUTF(vmStruct, J9VMCONSTANTPOOL_JAVALANGILLEGALACCESSERROR, errorMsg);


this should also be tabbed

tajila · 2018-02-26T16:49:24Z

runtime/vm/lookupmethod.c

+	/* Values of error codes for nestmates visibility checks on source class is
+	 * odd and on destination/target class is even
+	 */
+	if ((J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR_SOURCE_CLASS <= errorType)


should be:

if ((J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR_SOURCE_CLASS <= errorType) && (J9_VISIBILITY_NEST_MEMBER_NOT_CLAIMED_ERROR_DEST_CLASS >= errorType) ) {

This approach feels very hacky. It will be easy for someone to add a new error message that doesn't follow this pattern without realizing it.

A switch statement will be more explicit and less fragile to future changes:

switch(errorType) { case J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR_SOURCE_CLASS: case J9_VISIBILITY_NEST_HOST_DIFFERENT_PACKAGE_ERROR_SOURCE_CLASS: case J9_VISIBILITY_NEST_MEMBER_NOT_CLAIMED_ERROR_SOURCE_CLASS: unverifiedNestMemberClass = targetClass; break; case J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR_DEST_CLASS case J9_VISIBILITY_NEST_HOST_DIFFERENT_PACKAGE_ERROR_DEST_CLASS case J9_VISIBILITY_NEST_MEMBER_NOT_CLAIMED_ERROR_DEST_CLASS unverifiedNestMemberClass = senderClass; break; default: // Assert unreachable here is good when possible. In this case, would still need to handle other error cases.

DanHeidinga · 2018-02-28T04:06:29Z

runtime/nls/j9vm/j9vm.nls

@@ -1729,7 +1729,7 @@ J9NLS_VM_NEST_HOST_HAS_DIFFERENT_PACKAGE.user_response=Load class and nest host
 # Second argument is class name
 # Third argument is nest host name length
 # Fourth argument is nest host name
-J9NLS_VM_NEST_MEMBER_NOT_CLAIMED_BY_NEST_HOST=Class %2$.*1$s must be claimed by its nest host %4$.*1$s.
+J9NLS_VM_NEST_MEMBER_NOT_CLAIMED_BY_NEST_HOST=Type %2$.*1$s is not a nest member of %4$.*3$s: current type is not listed as a nest member


Note the warning at the top of the file:

# New messages MUST be added at the end of this file. # DO NOT delete messages from this file, as it will change their indices. # If you wish to remove a message, delete its text, but leave the key in place

Typically we don't remove messages or add/remove format specifiers. In this case, it should be OK as the NestMates haven't been translated or shipped yet.

DanHeidinga · 2018-02-28T04:07:17Z

runtime/nls/j9vm/j9vm.nls

+# Second argument is class name
+# Third argument is nest host name length
+# Fourth argument is nest host name
+J9NLS_VM_NEST_HOST_FAILED_TO_LOAD=Class %2$.*1$s must be able to load its nest host %4$.*3$s.


We don't want to copy the error messages. They should contain the same kind of information but shouldn't be copied from anywhere.

DanHeidinga · 2018-02-28T05:42:48Z

runtime/vm/lookupmethod.c

+	/* Values of error codes for nestmates visibility checks on source class is
+	 * odd and on destination/target class is even
+	 */
+	if ((J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR_SOURCE_CLASS <= errorType)


This approach feels very hacky. It will be easy for someone to add a new error message that doesn't follow this pattern without realizing it.

A switch statement will be more explicit and less fragile to future changes:

switch(errorType) { case J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR_SOURCE_CLASS: case J9_VISIBILITY_NEST_HOST_DIFFERENT_PACKAGE_ERROR_SOURCE_CLASS: case J9_VISIBILITY_NEST_MEMBER_NOT_CLAIMED_ERROR_SOURCE_CLASS: unverifiedNestMemberClass = targetClass; break; case J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR_DEST_CLASS case J9_VISIBILITY_NEST_HOST_DIFFERENT_PACKAGE_ERROR_DEST_CLASS case J9_VISIBILITY_NEST_MEMBER_NOT_CLAIMED_ERROR_DEST_CLASS unverifiedNestMemberClass = senderClass; break; default: // Assert unreachable here is good when possible. In this case, would still need to handle other error cases.

DanHeidinga · 2018-02-28T05:45:56Z

runtime/vm/lookupmethod.c

+		J9UTF8 *nestMemberNameUTF = J9ROMCLASS_CLASSNAME(romClass);
+		J9UTF8 *nestHostNameUTF = J9ROMCLASS_NESTHOSTNAME(romClass);
+
+		if ((J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR_SOURCE_CLASS == errorType)


This is another opportunity to use a switch. It may or may not be cleaner here.

DanHeidinga · 2018-02-28T05:50:43Z

runtime/vm/visible.c

@@ -29,6 +29,10 @@
 #include "util_api.h"
 #include "vm_internal.h"

+#if defined(J9VM_OPT_VALHALLA_NESTMATES)
+static VMINLINE loadAndVerifyNestHost(J9VMThread *vmThread, J9Class *clazz, BOOLEAN isSourceClass);


What's the return type for this function?

DanHeidinga · 2018-02-28T05:55:48Z

runtime/vm/visible.c

@@ -142,3 +155,55 @@ checkVisibility(J9VMThread *currentThread, J9Class* sourceClass, J9Class* destCl

 	return result;
 }
+
+#if defined(J9VM_OPT_VALHALLA_NESTMATES)
+static VMINLINE UDATA loadAndVerifyNestHost(J9VMThread *vmThread, J9Class *clazz, BOOLEAN isSourceClass) {


This should only be called once per class in non-failing cases as the nestHost will either be itself or filled in by the first call. I don't think it needs to be VMINLINEd.

For classes without a nest attribute, we can set the nestHost to themselves in createramclass.cpp and never call this function. (TODO: confirm we do that already)

DanHeidinga · 2018-02-28T05:57:02Z

runtime/vm/visible.c

+		nestHost = clazz;
+	} else {
+		UDATA classLoadingFlags = J9_FINDCLASS_FLAG_THROW_ON_FAIL;
+		nestHost = vmThread->javaVM->internalVMFunctions->internalFindClassUTF8(vmThread, J9UTF8_DATA(nestHostName), J9UTF8_LENGTH(nestHostName), clazz->classLoader, classLoadingFlags);


This doesn't need to call through the internalVMFunctions table as we're in the VM. It can call internalFindClassUTF8 directly

DanHeidinga · 2018-02-28T06:01:53Z

runtime/vm/visible.c

+		/* If any problem occurred in nest host verification, error code was set
+		 * assuming that the nest host is being loaded for the source class; if
+		 * not, add a -1 offset to change to dest class error code */
+		if ((J9_VISIBILITY_ALLOWED != result) && (TRUE != isSourceClass)) {


Not a fan of this approach. It assumes no one will ever renumber the constants and if they do, it will result in subtly wrong error messages.

I would rather see a new out parameter to indicate source / dest or a more obvious way of mapping between the two. A table maybe?

DanHeidinga · 2018-03-07T18:25:51Z

runtime/vm/lookupmethod.c

@@ -193,6 +193,7 @@ processMethod(J9VMThread * currentThread, UDATA lookupOptions, J9Method * method
 					} else {
 							*exception = J9VMCONSTANTPOOL_JAVALANGILLEGALACCESSERROR;
 							*exceptionClass = methodClass;
+							*errorType = J9_VISIBILITY_NON_MODULE_ACCESS_ERROR;


This is a really good find. Since this is a bug fix, can it be split into a separate PR?

That PR can address the other exit points from this method and ensure they set errorType appropriately as well.

Created a separate PR here: #1383
Other exit points look like they handle it correctly; this was the odd one out.

DanHeidinga · 2018-03-07T18:27:13Z

runtime/vm/lookupmethod.c

+	 */
+	if ((J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR == errorType)
+			|| (J9_VISIBILITY_NEST_HOST_DIFFERENT_PACKAGE_ERROR == errorType)
+			|| (J9_VISIBILITY_NEST_MEMBER_NOT_CLAIMED_ERROR == errorType)) {


formatting nitpick: the closing ) { should be on its own line at the same indent level as the if. Makes it much easier to visually track the start / end of the if block.

DanHeidinga · 2018-03-07T18:28:02Z

runtime/vm/lookupmethod.c

+		J9UTF8 *nestMemberNameUTF = NULL;
+		J9UTF8 *nestHostNameUTF = NULL;
+
+		if (NULL == senderClass->nestHost) {


This needs a comment to describe why we can check in this order and know if the issue was with the sender or target.

DanHeidinga · 2018-03-07T18:29:39Z

runtime/vm/lookupmethod.c

+
+		if (bufLen > 0) {
+			buf = j9mem_allocate_memory(bufLen, OMRMEM_CATEGORY_VM);
+			if (NULL != buf) {


Since it uses a goto anyway, code can be written as:

if (NULL == buf) { goto allocationFailures; } j9str_printf(PORTLIB, buf, bufLen, errorMsg, J9UTF8_LENGTH(nestMemberNameUTF), J9UTF8_DATA(nestMemberNameUTF), J9UTF8_LENGTH(nestHostNameUTF), J9UTF8_DATA(nestHostNameUTF));

This isn't something that needs to be changed. Just another way to write this code.

DanHeidinga · 2018-03-07T18:36:57Z

runtime/vm/visible.c

-					&& (sourceClass->nestHost != destClass->nestHost)
+				/* loadAndVerifyNestHost returns an error if setting nest host field fails */
+				if (NULL == destClass->nestHost) {
+					result = loadAndVerifyNestHost(currentThread, destClass);


if we add a goto, we can avoid the repeated (J9_VISIBILITY_ALLOWED == result) check in the common path.

Something like:

if (NULL == destClass->nestHost) { result = loadAndVerifyNestHost(currentThread, destClass); if ((J9_VISIBILITY_ALLOWED != result) { goto _exit; } } if (NULL == sourceClass->nestHost) { .... }

DanHeidinga · 2018-03-07T18:39:44Z

runtime/vm/visible.c

+	if (NULL == nestHostName) {
+		nestHost = clazz;
+	} else {
+		UDATA classLoadingFlags = J9_FINDCLASS_FLAG_THROW_ON_FAIL;


TODO: check the code to see if we need to worry about this at JIT compile time. If it's a compile time resolve, we hopefully aren't doing access already.

This is called by checkVisibility, which is called by resolveClassRef during jit compile time resolves.

This code needs to check if this is a compile time resolve and if it is, replace J9_FINDCLASS_FLAG_THROW_ON_FAIL with J9_FINDCLASS_FLAG_EXISTING_ONLY.

checkVisiblity <- call by resolvefield.cpp code. Needs to also respect J9_RESOLVE_FLAG_NO_THROW_ON_FAIL to indicate not to throw.

if ((resolveFlags & J9_RESOLVE_FLAG_NO_THROW_ON_FAIL) || (resolveFlags & COMPILE_TIME) || .... AOT.... || ... J9_FINDCLASS_FLAG_EXISTING_ONLY ... ) { classLoadingFlags = J9_FINDCLASS_FLAG_EXISTING_ONLY; }

Or make the callers pass in BOOLEAN canRunJavaCode and use that to pick the EXISTING_ONLY or not flags. Some callers may already know this.

DanHeidinga · 2018-03-07T18:42:13Z

runtime/vm/visible.c

+				}
+			}
+			if (!verified) {
+				result = J9_VISIBILITY_NEST_MEMBER_NOT_CLAIMED_ERROR;


This can avoid the verified boolean if result = J9_VISIBILITY_NEST_MEMBER_NOT_CLAIMED_ERROR; is set before the loop and then set to J9_VISIBILITY_ALLOWED if we find the nest member.

tajila · 2018-03-21T20:08:35Z

runtime/vm/lookupmethod.c

+	if ((J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR == errorType)
+			|| (J9_VISIBILITY_NEST_HOST_DIFFERENT_PACKAGE_ERROR == errorType)
+			|| (J9_VISIBILITY_NEST_MEMBER_NOT_CLAIMED_ERROR == errorType)
+		) {


needs to be shifted to the left by one tab

tajila · 2018-03-21T20:30:12Z

runtime/vm/visible.c

+				if (NULL == sourceClass->nestHost) {
+					result = loadAndVerifyNestHost(currentThread, sourceClass, lookupOptions);
+				}
+				if (J9_VISIBILITY_ALLOWED != result) {


why isn't this in the same scope has the (NULL == sourceClass->nestHost) check above

tajila · 2018-03-21T20:31:15Z

runtime/vm/visible.c

@@ -183,3 +213,52 @@ checkVisibility(J9VMThread *currentThread, J9Class* sourceClass, J9Class* destCl

 	return result;
 }
+
+#if defined(J9VM_OPT_VALHALLA_NESTMATES)
+static UDATA loadAndVerifyNestHost(J9VMThread *vmThread, J9Class *clazz, UDATA options) {


the { at the end should be on a separate line

tajila · 2018-03-21T20:34:27Z

runtime/vm/visible.c

+		nestHost = clazz;
+	} else {
+		UDATA classLoadingFlags = 0;
+		if ((options & J9_LOOK_NO_THROW ) != J9_LOOK_NO_THROW) {


use J9_ARE_NO_BITS_SET(...) macro for clarity

DanHeidinga · 2018-03-26T18:21:04Z

runtime/nls/j9vm/j9vm.nls

+J9NLS_VM_NEST_HOST_FAILED_TO_LOAD=Class %2$.*1$s must be able to load its nest host %4$.*3$s.
+# START NON-TRANSLATABLE
+J9NLS_VM_NEST_HOST_FAILED_TO_LOAD.explanation=Class and nest host class were loaded with different class loaders.
+J9NLS_VM_NEST_HOST_FAILED_TO_LOAD.system_action=The JVM will throw a java/lang/VerifyError


java/lang/VerifyError -> ????? This is an IllegalAccessError now isn't it? The check doesn't occur during class load anymore...

DanHeidinga · 2018-03-26T18:35:29Z

runtime/vm/visible.c

+					result = loadAndVerifyNestHost(currentThread, sourceClass, lookupOptions);
+					if (J9_VISIBILITY_ALLOWED != result) {
+						goto _exit;
+					}


We need to update sourceClass to be the current class here again. Can you add:

sourceClass = J9_CURRENT_CLASS(sourceClass);

here?

In FSD mode, we may have created a new J9Class for sourceClass if a redefinition occurred and it may have a stale J9Class pointer.

DanHeidinga · 2018-03-26T18:35:48Z

runtime/vm/visible.c

+					result = loadAndVerifyNestHost(currentThread, destClass, lookupOptions);
+					if (J9_VISIBILITY_ALLOWED != result) {
+						goto _exit;
+					}


Same for destClass here. It also needs to be reloaded.

DanHeidinga · 2018-03-27T14:00:57Z

runtime/vm/visible.c

@@ -158,12 +158,16 @@ checkVisibility(J9VMThread *currentThread, J9Class* sourceClass, J9Class* destCl
 				 */
 				if (NULL == sourceClass->nestHost) {
 					result = loadAndVerifyNestHost(currentThread, sourceClass, lookupOptions);
+					sourceClass = J9_CURRENT_CLASS(sourceClass);


Small nitpick - these only need to be reloaded if the visibility succeeds so this could move after the if

DanHeidinga · 2018-03-27T14:11:28Z

runtime/vm/visible.c

+					if (J9_VISIBILITY_ALLOWED != result) {
+						goto _exit;
+						sourceClass = J9_CURRENT_CLASS(sourceClass);
+						destClass = J9_CURRENT_CLASS(destClass);


Shouldn't these be after the if statement? Otherwise they're dead code following the goto

DanHeidinga · 2018-03-27T14:27:54Z

The code in checkVisibility is missing some tracepoints for the module and nestmate failing cases. A separate PR can be used to add the tracepoints.

DanHeidinga · 2018-04-05T19:31:31Z

Jenkins test sanity plinux jdk9

DanHeidinga · 2018-04-06T15:45:30Z

Recording that the build passed: https://ci.eclipse.org/openj9/job/PullRequest-Sanity-JDK9-linux_ppc-64_cmprssptrs_le-OpenJ9/202/

@taliamccormick Can you squash into logical sets of commits and we can merge this?

Earlier nest mates spec draft loaded a class's nest host during class loading. More recent nest mates spec puts off host loading until required for an access check. This commit moves the nest host loading and verification to the visibility check. The spec draft for checking visibility states: To determine whether a class or interface C belongs to the same nest as D (that is, whether C and D are nestmates), the following steps are performed: 1. If C and D are the same class or interface, they belong to the same nest. 2. Otherwise, the nest host of D, H, is determined. If an exception is thrown, the nest membership test fails for the same reason. 3. Otherwise, the nest host of C, H', is determined. If an exception is thrown, the nest membership test fails for the same reason. 4. Otherwise, C and D belong to the same nest if H and H' are the same class or interface. To determine the nest host of a class M, the following steps are performed: 1. If M lacks a NestHost attribute (4.7.28), M is its own nest host. 2. Otherwise, where i is the host_class_index item of M's NestHost attribute, the symbolic reference at index i of M's run-time constant pool is resolved to a class or interface H (5.4.3.1). Any of the exceptions pertaining to class or interface resolution can be thrown. 3. If resolution of H succeeds, but H is not declared in the same run-time package as M, an IncompatibleClassChangeError is thrown. 4. Otherwise, if H lacks a NestMembers attribute (4.7.29), or if, where M has name N, there exists no entry in the classes array of the NestMembers attribute of H that refers to a class or interface with name N, an IncompatibleClassChangeError is thrown. 5. Otherwise, H is the nest host of M. As taken from here: http://cr.openjdk.java.net/~dlsmith/nestmates.html Signed-off-by: Talia McCormick <Talia.McCormick@ibm.com>

Earlier nest mates spec draft loaded a class's nest host during class loading. More recent nest mates spec puts off host loading until required for an access check. This commit removes the nest host loading and verification from class loading and class verification respectively. Signed-off-by: Talia McCormick <Talia.McCormick@ibm.com>

taliamccormick · 2018-04-06T18:58:41Z

@DanHeidinga Squashed

taliamccormick mentioned this pull request Feb 20, 2018

Updates valhalla nestmates test to match spec update #1201

Merged

tajila reviewed Feb 20, 2018

View reviewed changes

pshipton added comp:vm project:valhalla Used to track Project Valhalla related work labels Feb 20, 2018

taliamccormick force-pushed the UpdateNestmatesLinking branch 2 times, most recently from 1fdcb78 to 4487047 Compare February 21, 2018 16:31

tajila reviewed Feb 21, 2018

View reviewed changes

taliamccormick force-pushed the UpdateNestmatesLinking branch 2 times, most recently from 98e86cc to eec3940 Compare February 22, 2018 15:29

tajila reviewed Feb 22, 2018

View reviewed changes

taliamccormick force-pushed the UpdateNestmatesLinking branch 3 times, most recently from f9c19ff to 5c0df86 Compare February 26, 2018 15:46

tajila reviewed Feb 26, 2018

View reviewed changes

DanHeidinga reviewed Feb 28, 2018

View reviewed changes

taliamccormick force-pushed the UpdateNestmatesLinking branch 3 times, most recently from 0fa1703 to a88c8e2 Compare March 2, 2018 20:41

taliamccormick force-pushed the UpdateNestmatesLinking branch 2 times, most recently from 014e853 to 6a882d3 Compare March 6, 2018 22:43

DanHeidinga reviewed Mar 7, 2018

View reviewed changes

taliamccormick mentioned this pull request Mar 8, 2018

Refactor access check for processing methods #1383

Merged

taliamccormick force-pushed the UpdateNestmatesLinking branch 3 times, most recently from eb71eab to 502e251 Compare March 14, 2018 20:22

taliamccormick mentioned this pull request Mar 21, 2018

[Nestmates] Updates java.lang.Class API for new nestmates spec draft (class loading) depends on #1224 #1501

Closed

tajila approved these changes Mar 21, 2018

View reviewed changes

taliamccormick force-pushed the UpdateNestmatesLinking branch from 502e251 to 772bc68 Compare March 21, 2018 22:21

taliamccormick force-pushed the UpdateNestmatesLinking branch from 772bc68 to e264d3c Compare March 21, 2018 22:24

taliamccormick changed the title ~~Moves nestmates host loading to visibility check~~ [Nest mates] Moves nestmates host loading to visibility check Mar 26, 2018

DanHeidinga reviewed Mar 26, 2018

View reviewed changes

DanHeidinga reviewed Mar 27, 2018

View reviewed changes

DanHeidinga approved these changes Apr 5, 2018

View reviewed changes

DanHeidinga self-assigned this Apr 5, 2018

Talia McCormick added 2 commits April 6, 2018 14:52

taliamccormick force-pushed the UpdateNestmatesLinking branch from 859a2df to 6d6bc09 Compare April 6, 2018 18:57

DanHeidinga merged commit 5727d76 into eclipse-openj9:master Apr 9, 2018

Visibility check result	Value	Hex value	Last eight bits
J9_VISIBILITY_ALLOWED	1	1	0000 0001
J9_VISIBILITY_NON_MODULE_ACCESS_ERROR	0	0	0
J9_VISIBILITY_MODULE_READ_ACCESS_ERROR	-1	FFFF FFFF	1111 1111
J9_VISIBILITY_MODULE_PACKAGE_EXPORT_ERROR	-2	FFFF FFFE	1111 1110
J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR_SOURCE_CLASS	-17	FFFF FFFF	1110 1111
J9_VISIBILITY_NEST_HOST_LOADING_FAILURE_ERROR_TARGET_CLASS	-18	FFFF FFEF	1110 1110
J9_VISIBILITY_NEST_HOST_DIFFERENT_PACKAGE_ERROR_SOURCE_CLASS	-19	FFFF FFED	1110 1101
J9_VISIBILITY_NEST_HOST_DIFFERENT_PACKAGE_ERROR_TARGET_CLASS	-20	FFFF FFEC	1110 1100
J9_VISIBILITY_NEST_MEMBER_NOT_CLAIMED_ERROR_SOURCE_CLASS	-21	FFFF FFEB	1110 1011
J9_VISIBILITY_NEST_MEMBER_NOT_CLAIMED_ERROR_TARGET_CLASS	-22	FFFF FFEA	1110 1010

[Nest mates] Moves nestmates host loading to visibility check #1224

[Nest mates] Moves nestmates host loading to visibility check #1224

Conversation

taliamccormick commented Feb 20, 2018

taliamccormick commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanHeidinga commented Mar 27, 2018

DanHeidinga commented Apr 5, 2018

DanHeidinga commented Apr 6, 2018

taliamccormick commented Apr 6, 2018