Throw NoClassDefFoundError instead of TypeNotPresentException #610

babsingh · 2017-11-16T19:52:25Z

Change 1:

When the VM is resolving a MethodType, NoClassDefFoundError should be thrown instead of TypeNotPresentException.

Change 2:

Renamed test_indyn_ldc_invalid_type to test_indyn_ldc_non_existent_class_methodtype. Updated test to check for NoClassDefFoundError instead of TypeNotPresentException.

Signed-off-by: Babneet Singh [email protected]

babsingh · 2017-11-16T23:01:22Z

@DanHeidinga
According to the below Javadoc for MethodType.fromMethodDescriptorString, TypeNotPresentException must be thrown for JCL usage. But, NoClassDefFoundError must be thrown by VM during the resolve stage.

Should I create two versions of MethodType.fromMethodDescriptorString? One for VM usage that throws NoClassDefFoundError, and one for JCL usage that throws TypeNotPresentException.

MethodType.fromMethodDescriptorString Javadoc: https://docs.oracle.com/javase/9/docs/api/java/lang/invoke/MethodType.html#fromMethodDescriptorString-java.lang.String-java.lang.ClassLoader-

DanHeidinga · 2017-11-17T03:37:19Z

Duplicating code isn't a great answer. What about modifying the resolve MT path to convert the TNPE to a NCDFE?

babsingh · 2017-11-21T16:27:20Z

Code has been updated - modified resolve MT path.
Peer Review - @JasonFengJ9

JasonFengJ9 · 2017-11-21T18:52:21Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandle.java

+	 * instead of TypeNotPresentException during the VM resolve stage.
+	 *
+	 * @param methodDescriptor - the method descriptor string
+	 * @param loader - the ClassLoader to be used or null for System ClassLoader


The loader passed in here is from VMAccess.getClassloader() --> clazz.getClassLoaderImpl() which returns the actual bootstrap classloader, not null.

updated description.

JasonFengJ9 · 2017-11-21T18:53:23Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandle.java

+	 * @throws NoClassDefFoundError - if a named type cannot be found
+	 */
+	private static final MethodType vmResolveFromMethodDescriptorString(String methodDescriptor, ClassLoader loader) throws Throwable {
+		MethodType type = null;


Initializing a variable to null here is not necessary.

JasonFengJ9 · 2017-11-21T18:54:20Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandle.java

+	private static final MethodType vmResolveFromMethodDescriptorString(String methodDescriptor, ClassLoader loader) throws Throwable {
+		MethodType type = null;
+		try {
+			type = MethodType.fromMethodDescriptorString(methodDescriptor, loader);


Probably just return MethodType.fromMethodDescriptorString(methodDescriptor, loader);.

JasonFengJ9 · 2017-11-21T19:00:45Z

test/Jsr292/src/com/ibm/j9/jsr292/indyn/IndyTest.java

 		}
+		Assert.fail("NoClassDefFoundError not thrown when non-existent class is used to create MethodType via LDC");


This Assert.fail can be moved after MethodType mtToString = com.ibm.j9.jsr292.indyn.GenIndyn.ldc_invalid_type();, and return within catch block can be removed.

JasonFengJ9 · 2017-11-21T19:01:34Z

test/Jsr292/src/com/ibm/j9/jsr292/indyn/IndyTest.java

-
-		boolean typeNotPresentExceptionThrown = false;
-
+	public void test_indyn_ldc_non_existent_class_methodtype() {
 		try {
 			MethodType mtToString = com.ibm.j9.jsr292.indyn.GenIndyn.ldc_invalid_type();


mtToString is not used, can it be removed?

we should keep it since it tells what is being returned.

JasonFengJ9 · 2017-11-21T20:48:39Z

runtime/vm/callin.cpp

@@ -883,6 +883,24 @@ sendFromMethodDescriptorString(J9VMThread *currentThread, J9UTF8 *descriptor, J9
 				currentThread->returnValue2 = (UDATA)J9VMJAVALANGINVOKEMETHODTYPE_FROMMETHODDESCRIPTORSTRINGAPPENDARG_METHOD(vm);
 			}
 			c_cInterpreter(currentThread);
+			if (VM_VMHelpers::exceptionPending(currentThread)) {
+				j9object_t currentException = currentThread->currentException;
+				J9Class *exceptionClass = J9VMJAVALANGTYPENOTPRESENTEXCEPTION(vm);


Using name exceptionClassTNPE offers better clarity. In addition, to achieve putting a rvalue on the left when comparing for equality later, pls define it as J9Class * const exceptionClassTNPE.

done - changed to exceptionClassTNPE and added const.

JasonFengJ9 · 2017-11-21T20:50:31Z

runtime/vm/callin.cpp

+				 */
+				if (exceptionClass == J9OBJECT_CLAZZ(currentThread, currentException)) {
+					j9object_t cause = J9VMJAVALANGTHROWABLE_CAUSE(currentThread, currentException);
+					J9Class *causeClass = J9VMJAVALANGCLASSNOTFOUNDEXCEPTION(vm);


Similarly, using name causeClassThrowable offers better clarity. In addition, pls define it as J9Class * const causeClassThrowable.

done - changed to causeClassCNFE and added const.

DanHeidinga · 2017-11-22T03:47:55Z

runtime/vm/callin.cpp

@@ -883,6 +883,24 @@ sendFromMethodDescriptorString(J9VMThread *currentThread, J9UTF8 *descriptor, J9
 				currentThread->returnValue2 = (UDATA)J9VMJAVALANGINVOKEMETHODTYPE_FROMMETHODDESCRIPTORSTRINGAPPENDARG_METHOD(vm);
 			}
 			c_cInterpreter(currentThread);
+			if (VM_VMHelpers::exceptionPending(currentThread)) {


Rather than duplicating this code natively, wouldn't it be cleaner to call vmResolveFromMethodDescriptorString() directly and let the java code convert the TNPE to a CNFE?

It will be better in terms of avoiding duplicate code. But, sendFromMethodDescriptorString calls MT.fromMethodDescriptorString and MT.fromMethodDescriptorStringAppendArg. So, the definition of MH.vmResolveFromMethodDescriptorString will change.

private static final MethodType vmResolveFromMethodDescriptorString(String methodDescriptor, ClassLoader loader, Class<?> appendArgumentType) throws Throwable { try { if (null == appendArgumentType) { return MethodType.fromMethodDescriptorString(methodDescriptor, loader); } else { return MethodType.fromMethodDescriptorStringAppendArg(methodDescriptor, loader, appendArgumentType); } } catch (TypeNotPresentException e) { throw throwNoClassDefFoundError(e); } }

Currently, MT.fromMethodDescriptorStringAppendArg is a private method. It will change to a public method since it will be used in MethodHandle.java.

MH resolve code paths will become slightly longer due to this change. @DanHeidinga do you approve the above changes?

It cannot become a public method as that changes the public api for a class in the java.lang.invoke.* namespace.

It can become package or default access which will allow it to be called anywhere in the package without becoming API.

For the MH resolve, this shouldn't be an issue as the resolve occurs once per item.

DanHeidinga

Is this change to the exception being thrown the expected behaviour in both Java 8 & 9?

DanHeidinga · 2017-11-23T15:44:50Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodType.java

+		try {
+			if (null == appendArgumentType) {
+				return MethodType.fromMethodDescriptorString(methodDescriptor, loader);
+			} else {


Nitpick: an if that terminates with a return or a throw doesn't need an else block after it.

babsingh · 2017-11-23T16:06:05Z

This is a JVM requirement for both Java 8 and Java 9 during resolution.

If the Java Virtual Machine ever attempts to load a class C during verification (§5.4.1) or resolution (§5.4.3) (but not initialization (§5.5)), and the class loader that is used to initiate loading of C throws an instance of ClassNotFoundException, then the Java Virtual Machine must throw an instance of NoClassDefFoundError whose cause is the instance of ClassNotFoundException.

DanHeidinga · 2017-11-24T04:24:08Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodType.java

+		}
+	}
+
+	/**


Can you fix the formatting for the javadoc on this and the above method? The formatting is inconsistent between the two.

When the VM is resolving a MethodType, NoClassDefFoundError should be thrown instead of TypeNotPresentException. Signed-off-by: Babneet Singh <[email protected]>

Renamed test_indyn_ldc_invalid_type to test_indyn_ldc_non_existent_class_methodtype. Updated test to check for NoClassDefFoundError instead of TypeNotPresentException. Signed-off-by: Babneet Singh <[email protected]>

DanHeidinga · 2017-11-24T14:37:13Z

Jenkins test sanity plinux

babsingh force-pushed the fix_noclassdeferror branch from 2ebd50e to c9379d7 Compare November 16, 2017 21:20

babsingh changed the title ~~Throw NoClassDefFoundError instead of TypeNotPresentException~~ WIP: Throw NoClassDefFoundError instead of TypeNotPresentException Nov 17, 2017

pshipton added the comp:vm label Nov 17, 2017

babsingh force-pushed the fix_noclassdeferror branch 4 times, most recently from 0be2182 to f4aea60 Compare November 21, 2017 00:04

babsingh changed the title ~~WIP: Throw NoClassDefFoundError instead of TypeNotPresentException~~ Throw NoClassDefFoundError instead of TypeNotPresentException Nov 21, 2017

JasonFengJ9 reviewed Nov 21, 2017

View reviewed changes

babsingh force-pushed the fix_noclassdeferror branch from f4aea60 to 76cd3ff Compare November 21, 2017 19:05

JasonFengJ9 reviewed Nov 21, 2017

View reviewed changes

babsingh force-pushed the fix_noclassdeferror branch 2 times, most recently from a730300 to 3f6b771 Compare November 21, 2017 21:45

DanHeidinga reviewed Nov 22, 2017

View reviewed changes

babsingh force-pushed the fix_noclassdeferror branch from 3f6b771 to 8f427bd Compare November 23, 2017 08:45

DanHeidinga reviewed Nov 23, 2017

View reviewed changes

babsingh force-pushed the fix_noclassdeferror branch from 8f427bd to 6516407 Compare November 23, 2017 15:56

DanHeidinga reviewed Nov 24, 2017

View reviewed changes

babsingh added 2 commits November 24, 2017 08:06

Throw NoClassDefFoundError instead of TypeNotPresentException

5a15df4

When the VM is resolving a MethodType, NoClassDefFoundError should be thrown instead of TypeNotPresentException. Signed-off-by: Babneet Singh <[email protected]>

Update test: IndyTest.test_indyn_ldc_invalid_type

08d5f60

Renamed test_indyn_ldc_invalid_type to test_indyn_ldc_non_existent_class_methodtype. Updated test to check for NoClassDefFoundError instead of TypeNotPresentException. Signed-off-by: Babneet Singh <[email protected]>

babsingh force-pushed the fix_noclassdeferror branch from 6516407 to 08d5f60 Compare November 24, 2017 13:08

DanHeidinga approved these changes Nov 24, 2017

View reviewed changes

DanHeidinga self-assigned this Nov 24, 2017

DanHeidinga merged commit 7ae77ca into eclipse-openj9:master Nov 24, 2017

babsingh deleted the fix_noclassdeferror branch August 17, 2020 02:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw NoClassDefFoundError instead of TypeNotPresentException #610

Throw NoClassDefFoundError instead of TypeNotPresentException #610

babsingh commented Nov 16, 2017 •

edited

Loading

babsingh commented Nov 16, 2017 •

edited

Loading

DanHeidinga commented Nov 17, 2017

babsingh commented Nov 21, 2017

JasonFengJ9 Nov 21, 2017

babsingh Nov 21, 2017

JasonFengJ9 Nov 21, 2017

babsingh Nov 21, 2017

JasonFengJ9 Nov 21, 2017

JasonFengJ9 Nov 21, 2017

babsingh Nov 21, 2017

JasonFengJ9 Nov 21, 2017

babsingh Nov 21, 2017

JasonFengJ9 Nov 21, 2017

babsingh Nov 21, 2017 •

edited

Loading

JasonFengJ9 Nov 21, 2017

babsingh Nov 21, 2017

DanHeidinga Nov 22, 2017

babsingh Nov 22, 2017 •

edited

Loading

DanHeidinga Nov 22, 2017

babsingh Nov 23, 2017

DanHeidinga left a comment

DanHeidinga Nov 23, 2017

babsingh Nov 23, 2017

babsingh commented Nov 23, 2017 •

edited

Loading

DanHeidinga Nov 24, 2017

babsingh Nov 24, 2017

DanHeidinga commented Nov 24, 2017

		}
		Assert.fail("NoClassDefFoundError not thrown when non-existent class is used to create MethodType via LDC");

+              		}
+              	}
+              	/**

Throw NoClassDefFoundError instead of TypeNotPresentException #610

Throw NoClassDefFoundError instead of TypeNotPresentException #610

Conversation

babsingh commented Nov 16, 2017 • edited Loading

babsingh commented Nov 16, 2017 • edited Loading

DanHeidinga commented Nov 17, 2017

babsingh commented Nov 21, 2017

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

babsingh Nov 21, 2017 • edited Loading

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

babsingh Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanHeidinga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

babsingh commented Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanHeidinga commented Nov 24, 2017

babsingh commented Nov 16, 2017 •

edited

Loading

babsingh commented Nov 16, 2017 •

edited

Loading

babsingh Nov 21, 2017 •

edited

Loading

babsingh Nov 22, 2017 •

edited

Loading

babsingh commented Nov 23, 2017 •

edited

Loading