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 Q types in lambda signature generation #16815

Merged
merged 1 commit into from
Mar 13, 2023
Merged

Fix Q types in lambda signature generation #16815

merged 1 commit into from
Mar 13, 2023

Conversation

ehrenjulzert
Copy link

@ehrenjulzert ehrenjulzert commented Mar 2, 2023 •
edited
Loading

Correctly generate method signatures for lambda functions that take Q type arguments

For #13182 (fixes error in WithFieldAccessorTest)

@ehrenjulzert
Copy link
Author

@hangshao0

hangshao0
hangshao0 approved these changes Mar 2, 2023
View reviewed changes
Copy link
Contributor

@hangshao0 hangshao0 left a comment

Choose a reason for hiding this comment

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

LGTM.

@hangshao0 hangshao0 requested a review from tajila March 2, 2023 20:31
@hangshao0 hangshao0 added comp:vm project:valhalla Used to track Project Valhalla related work labels Mar 2, 2023
@tajila
Copy link
Contributor

tajila commented Mar 2, 2023 •
edited
Loading

LGTM, the PR title should be updated to something that reflects "Correctly generate method signatures for lambda functions that take Q type arguments". Likewise with the commit message

@tajila
Copy link
Contributor

tajila commented Mar 2, 2023

jenkins test compile win jdk8

@tajila
Copy link
Contributor

tajila commented Mar 2, 2023

Jenkins test sanity plinuxvalst jdknext

@tajila
Copy link
Contributor

tajila commented Mar 2, 2023

Jenkins test sanity,extended xlinuxval jdknext

@ehrenjulzert ehrenjulzert changed the title Make jtreg valhalla WithFieldAccessorTest pass Fix Q types in lambda signature generation Mar 2, 2023
@ehrenjulzert ehrenjulzert force-pushed the debugging_WithFieldAccessorTest_fix branch from bd9a5d0 to 606a5cb Compare March 2, 2023 21:33
@ehrenjulzert
Copy link
Author

Just pushed the commit message change (I hope it didn't stop the tests from running)

@tajila
Copy link
Contributor

tajila commented Mar 3, 2023

win -> https://openj9-jenkins.osuosl.org/view/Pull%20Requests/job/PullRequest-OpenJ9/3424/
standard testing -> https://openj9-jenkins.osuosl.org/view/Pull%20Requests/job/PullRequest-OpenJ9/3425/
vt tests -> https://openj9-jenkins.osuosl.org/view/Pull%20Requests/job/PullRequest-OpenJ9/3426/

@tajila
Copy link
Contributor

tajila commented Mar 3, 2023

Jenkins test sanity plinuxvalst jdknext

@tajila
Copy link
Contributor

tajila commented Mar 7, 2023

@ehrenjulzert Seems to be compile failures, can you rebase?

@ehrenjulzert ehrenjulzert force-pushed the debugging_WithFieldAccessorTest_fix branch from 606a5cb to abeb6ea Compare March 7, 2023 21:36
@ehrenjulzert
Copy link
Author

Okay, I rebased it

@tajila
Copy link
Contributor

tajila commented Mar 7, 2023

Jenkins test sanity plinuxvalst jdknext

@tajila
Copy link
Contributor

tajila commented Mar 8, 2023

@ehrenjulzert some build failures

@ehrenjulzert
Copy link
Author

When I rebase to master I also get compile issues (looks like openj9-openjdk-jdk.valuetypes is too out of date to support the extra boolean parameter that was added to the String.newStringUTF8NoRepl call in #16828). Should we just wait until openj9-openjdk-jdk.valuetypes is updated?

@hangshao0
Copy link
Contributor

@JasonFengJ9 ^^

@JasonFengJ9
Copy link
Member

openj9-openjdk-jdk.valuetypes has jdk-21+11 updated around Feb. 23rd.

There is a merging conflict to prevent a newer update because openjdk valhalla only has jdk-21+5 on Feb. 2th, and doesn't work w/ latest openjdk head stream yet.

For the new API String.newStringUTF8NoRepl(), I would suggest following Access change to continue using the earlier API until the update is available.

		/*[IF (JAVA_SPEC_VERSION < 21) | INLINE-TYPES]*/ 
		return String.newStringUTF8NoRepl(bytes, offset, length);
		/*[ELSE] JAVA_SPEC_VERSION < 21 */
		return String.newStringUTF8NoRepl(bytes, offset, length, true);
		/*[ENDIF] JAVA_SPEC_VERSION < 21 */

@ehrenjulzert
Copy link
Author

Should I put those changes in this PR or a new one?

@JasonFengJ9
Copy link
Member

Should I put those changes in this PR or a new one?

To pass a PR build, the change is required in this PR.

@ehrenjulzert
Copy link
Author

Alright, I was thinking of putting them in another PR and then rebasing when that got merged, but I suppose that would be overly complicated.

@ehrenjulzert ehrenjulzert force-pushed the debugging_WithFieldAccessorTest_fix branch from abeb6ea to 346a242 Compare March 8, 2023 23:03
@ehrenjulzert
Copy link
Author

Ok I pushed the fix in 346a242

@tajila
Copy link
Contributor

tajila commented Mar 9, 2023

Jenkins test sanity plinuxvalst jdknext

@tajila
Copy link
Contributor

tajila commented Mar 10, 2023

Jenkins test sanity,extended alinux64val jdknext

@tajila
Copy link
Contributor

tajila commented Mar 10, 2023

Jenkins test sanity,extended xlinuxval jdknext

@tajila
Copy link
Contributor

tajila commented Mar 10, 2023

Some more compile errors

00:25  /home/jenkins/workspace/Build_JDKnext_x86-64_linux_valhalla_Personal/build/linux-x86_64-server-release/support/j9jcl/java.base/share/classes/java/lang/Class.java:4360: warning: [cast] redundant cast to Method
14:00:25  					return (Method) reflectionFactory.copyMethod(method);
14:00:25  					       ^
14:00:25  /home/jenkins/workspace/Build_JDKnext_x86-64_linux_valhalla_Personal/build/linux-x86_64-server-release/support/j9jcl/java.base/share/classes/java/lang/Class.java:4401: warning: [cast] redundant cast to Method
14:00:25  		return (Method) reflectionFactory.copyMethod(method);
14:00:25  		       ^
14:00:25  /home/jenkins/workspace/Build_JDKnext_x86-64_linux_valhalla_Personal/build/linux-x86_64-server-release/support/j9jcl/java.base/share/classes/java/lang/Class.java:4419: warning: [cast] redundant cast to Field
14:00:25  				return (Field) reflectionFactory.copyField(field);
14:00:25  				       ^
14:00:25  /home/jenkins/workspace/Build_JDKnext_x86-64_linux_valhalla_Personal/build/linux-x86_64-server-release/support/j9jcl/java.base/share/classes/java/lang/Class.java:4453: warning: [cast] redundant cast to Field
14:00:25  		return (Field) reflectionFactory.copyField(field);
14:00:25  		       ^
14:00:25  /home/jenkins/workspace/Build_JDKnext_x86-64_linux_valhalla_Personal/build/linux-x86_64-server-release/support/j9jcl/java.base/share/classes/java/lang/Access.java:569: error: method does not override or implement a method from a supertype
14:00:25  	@Override
14:00:25  	^

@ehrenjulzert

@tajila
Copy link
Contributor

tajila commented Mar 10, 2023

@JasonFengJ9 is this a jdknext issue?

@JasonFengJ9
Copy link
Member

is this a jdknext issue?

Yeah, valhalla extension doesn't have latest openjdk JavaLangAccess.getLoaderNameID(ClassLoader loader).

openj9/jcl/src/java.base/share/classes/java/lang/Access.java

Line 700 in bfe3610

@Override

@ehrenjulzert this PR need following change as well.

/*[IF (JAVA_SPEC_VERSION >= 21) && !INLINE-TYPES]*/
	@Override
	public String getLoaderNameID(ClassLoader loader) {
....
	}
/*[ENDIF] JAVA_SPEC_VERSION >= 21 */

Fix Q types in lambda signature generation
e8ffceb 
Correctly generate method signatures for lambda
functions that take Q type arguments

For eclipse-openj9#13182 (fixes error in WithFieldAccessorTest)

Signed-off-by: Ehren Julien-Neitzert <[email protected]>
@ehrenjulzert ehrenjulzert force-pushed the debugging_WithFieldAccessorTest_fix branch from 346a242 to e8ffceb Compare March 10, 2023 22:46
@ehrenjulzert
Copy link
Author

Pushed the fix in e8ffceb

@tajila
Copy link
Contributor

tajila commented Mar 12, 2023

Jenkins test sanity,extended xlinuxval jdknext

@tajila
Copy link
Contributor

tajila commented Mar 12, 2023

jenkins test compile win jdk8

@tajila tajila merged commit c50acf4 into eclipse-openj9:master Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hangshao0 hangshao0 hangshao0 approved these changes

@tajila tajila Awaiting requested review from tajila

Assignees
No one assigned
Labels
comp:vm project:valhalla Used to track Project Valhalla related work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ehrenjulzert @tajila @hangshao0 @JasonFengJ9