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

Support for CP String patching #8460

Merged
merged 3 commits into from
Feb 8, 2020
Merged

Support for CP String patching #8460

merged 3 commits into from
Feb 8, 2020

Conversation

fengxue-IS
Copy link
Contributor

@fengxue-IS fengxue-IS commented Jan 30, 2020 •
edited
Loading

  • New J9ClassPatchMap structure to store patching info
  • Update defineClassCommon code to accept J9ClassPatchMap
  • Patch RAM constantpool before returing class object

Signed-off-by: Jack Lu Jack.S.Lu@ibm.com

babsingh
babsingh requested changes Jan 30, 2020
View reviewed changes
runtime/oti/j9nonbuilder.h Outdated Show resolved Hide resolved
@babsingh babsingh mentioned this pull request Jan 30, 2020
Merged
babsingh
babsingh requested changes Jan 30, 2020
View reviewed changes
Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

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

minor nitpicks

runtime/bcutil/defineclass.c Outdated Show resolved Hide resolved
runtime/jcl/common/jcldefine.c Outdated Show resolved Hide resolved
runtime/jcl/common/sun_misc_Unsafe.cpp Outdated Show resolved Hide resolved
runtime/jcl/common/sun_misc_Unsafe.cpp Outdated Show resolved Hide resolved
runtime/jcl/common/sun_misc_Unsafe.cpp Outdated Show resolved Hide resolved
runtime/jcl/common/sun_misc_Unsafe.cpp Outdated Show resolved Hide resolved
runtime/jcl/common/sun_misc_Unsafe.cpp Outdated Show resolved Hide resolved
@babsingh
Copy link
Contributor

babsingh commented Feb 3, 2020

@fengxue-IS i have verified that these changes work with JEP-370. can we remove the WIP and have the pull request merged?

@fengxue-IS fengxue-IS changed the title WIP: Support for CP String patching Support for CP String patching Feb 3, 2020
@fengxue-IS
Copy link
Contributor Author

@babsingh can you take another look please?

babsingh
babsingh approved these changes Feb 3, 2020
View reviewed changes
Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

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

lgtm. minor formatting change requested.

runtime/jcl/common/sun_misc_Unsafe.cpp Outdated Show resolved Hide resolved
@fengxue-IS fengxue-IS marked this pull request as ready for review February 3, 2020 23:50
@fengxue-IS
Copy link
Contributor Author

I've compiled this change locally and passed JEP370 tests (assert null not fixed yet on my local branch).

@babsingh could you run the testing with the lastest JEP 370 branch please, thanks

marking this as ready for formal review

This was referenced Feb 4, 2020
Merged
Merged
@babsingh babsingh mentioned this pull request Feb 5, 2020
Closed
@babsingh
Copy link
Contributor

babsingh commented Feb 5, 2020

@babsingh could you run the testing with the lastest JEP 370 branch please, thanks

These changes yield the correct behavior. No failures seen in JEP-370 testing. Refer to #8292 (comment) for more details.

@fjeremic
Copy link
Contributor

fjeremic commented Feb 5, 2020 •
edited
Loading

The commit messages contain no details on what this change does. It would be helpful for readers such has myself who don't know much about what is being done here to put a description of the change. It also helps future developers looking at the commit history.

@fengxue-IS
Copy link
Contributor Author

Added comments to the code/commit, added new null check after memory allocation.

This PR replaces #8369 , the old one will be kept as a WIP draft for future references.

@DanHeidinga can you please take a look
FYI @babsingh

DanHeidinga
DanHeidinga reviewed Feb 6, 2020
View reviewed changes
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.

This looks good over all. Some very minor touchups and it'll be ready to go (assuming the builds pass =)

runtime/bcutil/ConstantPoolMap.cpp Outdated Show resolved Hide resolved
runtime/bcutil/ConstantPoolMap.cpp Outdated Show resolved Hide resolved
runtime/jcl/common/sun_misc_Unsafe.cpp Outdated Show resolved Hide resolved
runtime/jcl/common/sun_misc_Unsafe.cpp Show resolved Hide resolved
@DanHeidinga DanHeidinga self-assigned this Feb 6, 2020
@DanHeidinga DanHeidinga added this to the Release 0.19 (Java 14) milestone Feb 6, 2020
@DanHeidinga
Copy link
Member

Jenkins test sanity zlinux jdk8,jdk14

@DanHeidinga
Copy link
Member

@fengxue-IS Can you record the build results / links before pushing new changes to this PR? I'll be on a plane shortly and likely won't see the results in time.

@pshipton
Copy link
Member

pshipton commented Feb 6, 2020

The builds passed https://ci.eclipse.org/openj9/job/PullRequest-OpenJ9/2621/

@fengxue-IS
Support CP String patching
573628b 
- New J9ClassPatchMap structure to store patching info
- Update defineClassCommon code to accept J9ClassPatchMap
- Patch RAM constantpool before returing class object

Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>
@fengxue-IS
Search RAM CP for identical String entries to patch
cd81472 
For each patch object, apply to all matching String entries in the constantpool

Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>
@fengxue-IS
Copy link
Contributor Author

Updated code as suggested by review comments.

DanHeidinga
DanHeidinga approved these changes Feb 7, 2020
View reviewed changes
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.

lgtm

@fengxue-IS
Add assert on CP patch type check
99754a9 
- Only allow String CP entries to be patched
- Remove redundant VMAccess call when setting OOM exception

Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>
@DanHeidinga
Copy link
Member

Jenkins test sanity zlinux jdk14

@DanHeidinga DanHeidinga merged commit 71b7b69 into eclipse-openj9:master Feb 8, 2020
babsingh added a commit to babsingh/openj9 that referenced this pull request Feb 10, 2020
@babsingh
New field _patchMap in ROMClassCreationContext is uninitialized
550e159 
New field _patchMap needs to be initialized in all
ROMClassCreationContext constructors since accessing the uninitialized
field will cause unexpected behavior.

Related: eclipse-openj9#8460

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@babsingh babsingh mentioned this pull request Feb 10, 2020
Merged
@fengxue-IS fengxue-IS mentioned this pull request Feb 13, 2020
Closed
11 tasks
@babsingh babsingh mentioned this pull request Jun 18, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@babsingh babsingh babsingh approved these changes

@DanHeidinga DanHeidinga DanHeidinga approved these changes

Assignees

@DanHeidinga DanHeidinga

Labels
comp:vm
Projects
None yet
Milestone
Release 0.19 (Java 14)
Development

Successfully merging this pull request may close these issues.
5 participants
@fengxue-IS @babsingh @fjeremic @DanHeidinga @pshipton