Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw NoClassDefFoundError: MH.resolveInvokeDynamic(...) #20

Merged
merged 1 commit into from
Sep 28, 2017

Conversation

babsingh
Copy link
Contributor

@babsingh babsingh commented Sep 14, 2017

If ConstantPool.getClassAt(...) returns null while resolving bootstrap
method handles, then we should throw NoClassDefFoundError, and the
cause of NoClassDefFoundError should be ClassNotFoundException. The
message of NoClassDefFoundError and ClassNotFoundException should
contain the name of the class, which wasn't found.

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

@babsingh babsingh changed the title Throw NoClassDefFoundError: MH.resolveInvokeDynamic(...) WIP: Throw NoClassDefFoundError: MH.resolveInvokeDynamic(...) Sep 14, 2017
@genie-openj9
Copy link

Can one of the admins verify this patch?

2 similar comments
@genie-openj9
Copy link

Can one of the admins verify this patch?

@genie-openj9
Copy link

Can one of the admins verify this patch?

@babsingh babsingh changed the title WIP: Throw NoClassDefFoundError: MH.resolveInvokeDynamic(...) Throw NoClassDefFoundError: MH.resolveInvokeDynamic(...) Sep 19, 2017
@babsingh babsingh force-pushed the jcl_npe_to_ncdfe branch 4 times, most recently from a724832 to 5961bb5 Compare September 19, 2017 22:19
@babsingh babsingh changed the title Throw NoClassDefFoundError: MH.resolveInvokeDynamic(...) WIP: Throw NoClassDefFoundError: MH.resolveInvokeDynamic(...) Sep 20, 2017
@babsingh
Copy link
Contributor Author

@AdamBrousseau the builds are failing without any errors. same issue noticed for #19.

@DanHeidinga
Copy link
Member

@babsingh Did you force push at some point? The builds get confused when the sha they expect isn't in the repo anymore

@babsingh
Copy link
Contributor Author

@DanHeidinga yes. how to fix this issue?

@DanHeidinga
Copy link
Member

I've restarted the build on travis. Let's see if it passes now

@babsingh
Copy link
Contributor Author

babsingh commented Sep 22, 2017

@DanHeidinga the build still failed.

Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some doc updates required, otherwise looks good

* Retrieve the class name of the constant pool class element located at the specified
* index in clazz's constant pool. Then, throw a NoClassDefFoundError with the cause
* set as ClassNotFoundException. The message of NoClassDefFoundError and
* ClassNotFoundException contains the name of the class, which couldn't be found.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add javadoc style comment for the arguments / return types?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@@ -459,6 +459,41 @@ Java_sun_reflect_ConstantPool_getFieldAtIfLoaded0(JNIEnv *env, jobject unusedObj
}

jobject JNICALL
Java_java_lang_invoke_MethodHandle_getClassNameAt(JNIEnv *env, jobject unusedObject, jobject constantPoolOop, jint cpIndex)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please a function comment that describes the arguments, the return types and any invariants.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

* set as ClassNotFoundException. The message of NoClassDefFoundError and
* ClassNotFoundException contains the name of the class, which couldn't be found.
*/
private static Throwable noClassDefFoundError(Class<?> clazz, int index) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its totally a nitpick, but can you call this throwNoClassDefFoundError(...)? so it will match the naming convention in the j.l.i. package for these kinds of methods

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@babsingh babsingh force-pushed the jcl_npe_to_ncdfe branch 2 times, most recently from a51643d to 121b44d Compare September 25, 2017 17:28
@babsingh babsingh changed the title WIP: Throw NoClassDefFoundError: MH.resolveInvokeDynamic(...) Throw NoClassDefFoundError: MH.resolveInvokeDynamic(...) Sep 25, 2017
* Get the class name from a constant pool class element, which is located at the
* specified index in clazz's constant pool.
*/
private static final native String getClassNameAt(Class<?> clazz, int index);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies - I missed a naming convention issue. All the other MH-constantPool related methods have a CP prefix - i.e.: getCPClassNameAt

see:

Java_java_lang_invoke_MethodHandle_getCPTypeAt
Java_java_lang_invoke_MethodHandle_getCPMethodTypeAt
Java_java_lang_invoke_MethodHandle_getCPMethodHandleAt

And Javadoc the method as well please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed name to getCPClassNameAt, added Java doc for getCPClassNameAt, and resolved merge conflicts that arose from merging #19

If ConstantPool.getClassAt(...) returns null while resolving bootstrap
method handles, then we should throw NoClassDefFoundError, and the
cause of NoClassDefFoundError should be ClassNotFoundException. The
message of NoClassDefFoundError and ClassNotFoundException should
contain the name of the class, which wasn't found.

Signed-off-by: Babneet Singh <[email protected]>
@DanHeidinga
Copy link
Member

Jenkins test sanity

@DanHeidinga
Copy link
Member

This is ready to merge once the sanity builds pass.

@babsingh
Copy link
Contributor Author

@DanHeidinga Sanity Tests have failed due to the following error:
tr.source/trj9/optimizer/J9Optimizer.cpp:85:45: error: ‘IfVoluntaryOSR’ is not a member of ‘OMR’
Should I rebase to fix this issue?

@pshipton
Copy link
Member

jenkins test sanity

@pshipton
Copy link
Member

tr.source/trj9/optimizer/J9Optimizer.cpp:85:45: error: ‘IfVoluntaryOSR’ is not a member of ‘OMR’

this was a temporarily problem as a PR was merged before the required OMR content had promoted.

@pshipton pshipton merged commit b5f3cc5 into eclipse-openj9:master Sep 28, 2017
@babsingh babsingh deleted the jcl_npe_to_ncdfe branch September 28, 2017 03:43
amicic pushed a commit to amicic/openj9 that referenced this pull request Jan 16, 2018
Disable Windows OpenJ9 Java 8 builds temporarily
tajila pushed a commit to tajila/openj9 that referenced this pull request Jul 11, 2019
* Updated JVMImageHeader and fixed reading from file

* Formatting fixup

* Moved the header's buffer to the stack

Signed-off-by: Brady Jessup <[email protected]>
hzongaro pushed a commit to hzongaro/openj9 that referenced this pull request Mar 31, 2020
…e-queries

Move value type query methods to ClassEnv
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants