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

Fix MemberName fields after Fast HCR and FSD #12799

Merged

Conversation

EricYangIBM
Copy link
Contributor

@EricYangIBM EricYangIBM commented May 31, 2021

fixMemberNames called after HCR to do a heap walk to fix all MemberName objects' vmtarget. MemberName->clazz and MemberName->vmindex have already been fixed by existing mechanisms.

  • If the object is a fieldID, set vmtarget to the field offset + modifiers. If it is a method/constructor ID, set vmtarget to the ID's method
  • Move needed MemberName macros to j9nonbuilder.h
  • Define struct consisting of J9VMThread * and void * to be passed into the callback function as it needs the current thread and the class hash table

Issue: #11528
Signed-off-by: Eric Yang eric.yang@ibm.com

runtime/util/hshelp.c Outdated Show resolved Hide resolved
runtime/util/hshelp.c Outdated Show resolved Hide resolved
runtime/util/hshelp.c Outdated Show resolved Hide resolved
runtime/util/hshelp.c Outdated Show resolved Hide resolved
runtime/util/hshelp.c Outdated Show resolved Hide resolved
runtime/util/hshelp.c Outdated Show resolved Hide resolved
runtime/util/hshelp.c Outdated Show resolved Hide resolved
@fengxue-IS fengxue-IS added the project:MH Used to track Method Handles related work label May 31, 2021
@EricYangIBM
Copy link
Contributor Author

Currently, vmtarget updates with the old IDs, even if they are outdated. Perhaps in the future we could add some handling to improve UX.

@EricYangIBM EricYangIBM changed the title [WIP] Fix MemberName fields after Fast HCR and FSD Fix MemberName fields after Fast HCR and FSD Jun 1, 2021
@EricYangIBM EricYangIBM marked this pull request as ready for review June 1, 2021 18:57
WIP of function to be called on each heap object

Signed-off-by: Eric Yang <eric.yang@ibm.com>
Need struct for the callback function to have access to the
currentThread
+ Other compile error fixes
@EricYangIBM
Copy link
Contributor Author

Changes build successfully.

Copy link
Contributor

@fengxue-IS fengxue-IS left a comment

Choose a reason for hiding this comment

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

minor formating suggestions, rest lgtm

runtime/util/hshelp.c Outdated Show resolved Hide resolved
runtime/util/hshelp.c Outdated Show resolved Hide resolved
runtime/util/hshelp.c Outdated Show resolved Hide resolved
runtime/util/hshelp.c Outdated Show resolved Hide resolved
runtime/util/hshelp.c Outdated Show resolved Hide resolved
runtime/util/hshelp.c Outdated Show resolved Hide resolved
runtime/util/hshelp.c Outdated Show resolved Hide resolved
runtime/util/hshelp.c Outdated Show resolved Hide resolved
@gacholio
Copy link
Contributor

gacholio commented Jun 2, 2021

I'm not sure this is doing what you expect - is the purpose of this to fix references from instance of MemberName to fields/methods of classes which have been redefined?

@EricYangIBM
Copy link
Contributor Author

EricYangIBM commented Jun 2, 2021

I'm not sure this is doing what you expect - is the purpose of this to fix references from instance of MemberName to fields/methods of classes which have been redefined?

It fixes the vmtarget field of every MemberName object using its already up to date vmindex field (updated by fixJNIRefs) and clazz field.

@gacholio
Copy link
Contributor

gacholio commented Jun 2, 2021

Also, please resolve any conversations that have been completed.

@EricYangIBM
Copy link
Contributor Author

@gacholio Done.
Also, may I ask why the intermediate UDATA cast when casting between pointers and ints? I followed existing code and it didn't have that cast. E.g.

J9OBJECT_U64_STORE(currentThread, membernameObject, vm->vmindexOffset, (U_64)vmindex);

@gacholio
Copy link
Contributor

gacholio commented Jun 2, 2021

It's not legal to cast between pointers and integers whose size does not match the size of a pointer. For the U64 case, this would fail to compile on some 32-bit platforms without the intermediate cast.

The line you cited is not casting to a pointer.

runtime/util/hshelp.c Outdated Show resolved Hide resolved
Move null hash table check to calling function
Split if statement
Add intermediate casts to UDATA
@gacholio
Copy link
Contributor

gacholio commented Jun 2, 2021

jenkins test sanity zlinux jdk8

@gacholio
Copy link
Contributor

gacholio commented Jun 2, 2021

jenkins test sanity winojdk292 jdk16 depends ibmruntimes/openj9-openjdk-jdk16#29

@gacholio
Copy link
Contributor

gacholio commented Jun 2, 2021

@babsingh Please triage the failures.

@babsingh
Copy link
Contributor

babsingh commented Jun 2, 2021

https://openj9-jenkins.osuosl.org/job/Test_openjdk16_j9_sanity.functional_x86-64_windows_ojdk292_Personal/3/

@gacholio In the above test build, SharedCPEntryInvokerTests test targets successfully pass: #11528 (comment). All failures are known issues. This PR is good to merge.

@gacholio gacholio merged commit 3288ea2 into eclipse-openj9:master Jun 3, 2021
@EricYangIBM
Copy link
Contributor Author

@gacholio I had not squashed the commits when you merged. The commits/commit messages I made are not the most meaningful. Do you want this to be fixed?

@gacholio
Copy link
Contributor

gacholio commented Jun 3, 2021

I think we may as well let it go this time (wish I had seen that before the merge). The only way to "correct" this would be to revert this and use a new PR with a single commit, which would just be even more churn.

@EricYangIBM EricYangIBM deleted the updateMemberNameAfterHCR branch June 3, 2021 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project:MH Used to track Method Handles related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants