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.

Sign up for GitHub

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

Jump to bottom

Fix reflect ConstantPool bootstrapping issues #18169

Merged
merged 1 commit into from
Sep 21, 2023
Merged

Fix reflect ConstantPool bootstrapping issues #18169

merged 1 commit into from
Sep 21, 2023

Conversation

tajila
Copy link
Contributor

@tajila tajila commented Sep 20, 2023 •
edited
Loading

We currently perform explicit native registration for jdk.internal.reflect.ConstantPool. This occurs after JCL init in VM startup. It is possible that ConstantPool will be used in JCL init before the natives are registered, this will result in signal 218 since we have public JNI stubs as a catch all for these capabilities.

I propose the following:

Step 1) (this PR)

  • introduce a registerNative call in ConstantPool.

Step 2)

  • update the JCL code to use the registerNative() in the static initializer of the class, thereby ensuring that the natives are registered before the class is used.

Step 3)

  • remove the explicit register native call in JVM initialization

With these steps we dont need to do any build coordination between openj9 and the extension repos.

@tajila
Copy link
Contributor Author

tajila commented Sep 20, 2023

jenkins compile win jdk11

@tajila
Copy link
Contributor Author

tajila commented Sep 20, 2023

jenkins test sanity alinux64 jdk21

@JasonFengJ9
Copy link
Member

With these steps we dont need to do any build coordination between openj9 and the extension repos.

Assuming the proposed extension changes are in extension repo openj9 branch, there is no acceptance build or manual promotion required like OMR contents.
I think it is efficient to make changes in both OpenJ9 and extension repos, and merge them at the same time (very small time windows to break some PR/personal builds) unless @keithc-ca or @pshipton disagree.

@pshipton
Copy link
Member

I'm happy to go either way. We can merge it all at once, or stage it like Tobi suggests. Staging it makes it easier to test & merge version by version.

@keithc-ca
Copy link
Contributor

the proposed extension changes

Which changes are those?

keithc-ca
keithc-ca requested changes Sep 20, 2023
View reviewed changes
runtime/jcl/common/sun_reflect_ConstantPool.c Outdated Show resolved Hide resolved
runtime/jcl/common/sun_reflect_ConstantPool.c Outdated Show resolved Hide resolved
runtime/jcl/common/sun_reflect_ConstantPool.c
Comment on lines +931 to +934
if (J9_ARE_ANY_BITS_SET(vm->extendedRuntimeFlags2, J9_EXTENDED_RUNTIME2_REFLECT_CONSTANTPOOL_REGISTER_NATIVES_CALLED)) {
goto _end;
}
vm->extendedRuntimeFlags2 |= J9_EXTENDED_RUNTIME2_REFLECT_CONSTANTPOOL_REGISTER_NATIVES_CALLED;
Copy link
Contributor

Choose a reason for hiding this comment

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

What ensures this is thread-safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JVM init is single threaded, so is class init.

@tajila
Fix reflect ConstantPool bootstrapping issues
4b61672 
We currently perform explicit native registration for
jdk.internal.reflect.ConstantPool. This occurs after JCL init in VM
startup. It is possible that ConstantPool will be used in JCL init before
the natives are registered, this will result in signal 218 since we have
public JNI stubs as a catch all for these capabilities.

I propose the following:

Step 1) (this PR)
- introduce a registerNative call in ConstantPool.

Step 2)
- update the JCL code to use the registerNative() in the static initializer
of the class, thereby ensuring that the natives are registered before the
class is used.

Step 3)
- remove the explicit register native call

With these steps we dont need to do any build coordination between openj9
and the extension repos.

Signed-off-by: tajila <atobia@ca.ibm.com>
tajila added a commit to tajila/openj9-openjdk-jdk that referenced this pull request Sep 20, 2023
@tajila
Register natives in ConstantPool init
b47823a 
Depends on eclipse-openj9/openj9#18169

Signed-off-by: tajila <atobia@ca.ibm.com>
@tajila tajila mentioned this pull request Sep 20, 2023
Merged
@keithc-ca
Copy link
Contributor

Step 3) remove the explicit register native call

Won't we end up in the same situation we have now?

@tajila
Copy link
Contributor Author

tajila commented Sep 20, 2023 •
edited
Loading

Won't we end up in the same situation we have now?

No, this refers to the explicit call we are doing in JVM initialization

@keithc-ca
Copy link
Contributor

No, this refers to the explicit call we are doing in JVM initialization

Are you referring to this call in stdinit.c?

@tajila
Copy link
Contributor Author

tajila commented Sep 20, 2023

Are you referring to this call in stdinit.c?

yes

tajila added a commit to tajila/openj9-openjdk-jdk that referenced this pull request Sep 20, 2023
@tajila
Register natives in ConstantPool init
8618d72 
Depends on eclipse-openj9/openj9#18169

Signed-off-by: tajila <atobia@ca.ibm.com>
tajila added a commit to tajila/openj9-openjdk-jdk that referenced this pull request Sep 20, 2023
@tajila
Register natives in ConstantPool init
bb21f10 
Depends on eclipse-openj9/openj9#18169

Signed-off-by: tajila <atobia@ca.ibm.com>
@tajila
Copy link
Contributor Author

tajila commented Sep 21, 2023

@keithc-ca any further comments?

@babsingh
Copy link
Contributor

babsingh commented Sep 21, 2023 •
edited
Loading

Old builds (all passed):

IMO, the first set of builds should still be good. @tajila Do you agree or recommend relaunching the builds?

@tajila
Copy link
Contributor Author

tajila commented Sep 21, 2023

Ill do a sanity compile since some source code was changed

@tajila
Copy link
Contributor Author

tajila commented Sep 21, 2023

jenkins compile alinux64 jdk11

@babsingh babsingh merged commit 1859981 into eclipse-openj9:master Sep 21, 2023
tajila added a commit to tajila/openj9-openjdk-jdk21 that referenced this pull request Sep 22, 2023
@tajila
Register natives in ConstantPool init
120c5c6 
Depends on eclipse-openj9/openj9#18169

Signed-off-by: tajila <atobia@ca.ibm.com>
@tajila tajila mentioned this pull request Sep 22, 2023
Merged
tajila added a commit to tajila/openj9-openjdk-jdk17 that referenced this pull request Sep 22, 2023
@tajila
Register natives in ConstantPool init
462c8c6 
Depends on eclipse-openj9/openj9#18169

Signed-off-by: tajila <atobia@ca.ibm.com>
@tajila tajila mentioned this pull request Sep 22, 2023
Merged
tajila added a commit to tajila/openj9-openjdk-jdk11 that referenced this pull request Sep 22, 2023
@tajila
Register natives in ConstantPool init
4cc6ad4 
Depends on eclipse-openj9/openj9#18169

Signed-off-by: tajila <atobia@ca.ibm.com>
@tajila tajila mentioned this pull request Sep 22, 2023
Merged
@tajila tajila mentioned this pull request Sep 25, 2023
Merged
tajila added a commit to tajila/openj9 that referenced this pull request Sep 25, 2023
@tajila
Remove redundant register native call
d23cb46 
Completes: eclipse-openj9#18169

Signed-off-by: Tobi Ajila <atobia@ca.ibm.com>
JasonFengJ9 pushed a commit to ibmruntimes/openj9-openjdk-jdk.valuetypes that referenced this pull request Oct 5, 2023
@tajila @JasonFengJ9
Register natives in ConstantPool init
1c08c0e 
Depends on eclipse-openj9/openj9#18169

Signed-off-by: tajila <atobia@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JasonFengJ9 JasonFengJ9 JasonFengJ9 approved these changes

@keithc-ca keithc-ca keithc-ca approved these changes

@babsingh babsingh babsingh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tajila @JasonFengJ9 @pshipton @keithc-ca @babsingh