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

Clear breakpts in class redef and fix HCR warning #19710

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

tajila
Copy link
Contributor

@tajila tajila commented Jun 14, 2024

The current implementation emits a warning message when a JVMTI operation suceeds.

Also, clear breakpoints unconditionally when a class is redefined.

@tajila
Copy link
Contributor Author

tajila commented Jun 14, 2024

jenkins test sanity.openjdk zlinux jdk22

@tajila
Copy link
Contributor Author

tajila commented Jun 14, 2024

jenkins test sanity,extended,sanity.openjdk zlinux jdk17

@tajila tajila requested a review from gacholio June 14, 2024 19:25
@tajila
Copy link
Contributor Author

tajila commented Jun 15, 2024

Failures:

FAILED test targets:
17:12:35  	jdk_lang_VarHandleTest_j9_0 - Test results: passed: 32; failed: 6 
17:12:35  		Failed test cases: 
17:12:35  			TEST: java/lang/invoke/VarHandles/VarHandleTestByteArrayAsDouble.java
17:12:35          TEST: java/lang/invoke/VarHandles/VarHandleTestByteArrayAsChar.java
17:12:35          TEST: java/lang/invoke/VarHandles/VarHandleTestByteArrayAsFloat.java
17:12:35          TEST: java/lang/invoke/VarHandles/VarHandleTestByteArrayAsLong.java
17:12:35          TEST: java/lang/invoke/VarHandles/VarHandleTestByteArrayAsInt.java
17:12:35          TEST: java/lang/invoke/VarHandles/VarHandleTestByteArrayAsShort.java

@tajila
Copy link
Contributor Author

tajila commented Jun 15, 2024

Another sanity.openjdk run with -XX:-EnableExtendedHCR https://openj9-jenkins.osuosl.org/job/Grinder/3668/

@pshipton
Copy link
Member

The VarHandles failures are this known issue #19693

@tajila
Copy link
Contributor Author

tajila commented Jun 17, 2024

@gacholio please review these changes

@pshipton
Copy link
Member

Pls fix the commit title to reflect the change to clear the breakpoints. The current title is misleading.

@tajila tajila changed the title Fix extended HCR message Clear all breakpoints in class redefinition Jun 17, 2024
@pshipton
Copy link
Member

I expected a title that would mention both changes.

@pshipton
Copy link
Member

With this fix, can the -XX:+EnableExtendedHCR option be removed from any of the tests where it was added by #19554 ?

@gacholio
Copy link
Contributor

If the playlists are shared between open and IBM builds, then we must retain the option as the default differs between the two builds.

@pshipton
Copy link
Member

If the playlists are shared between open and IBM builds, then we must retain the option as the default differs between the two builds.

Not sure about this logic. The IBM builds don't need any test change because -XX:+EnableExtendedHCR is enabled by default. The OpenJ9 builds may need to have this option added to certain tests, however the tests that need it were determined before the bug fix in hshelp.c, perhaps the option can be removed from some of the tests with this change.

  if (extensionsEnabled == 0) {
		if (JVMTI_ERROR_NONE != rc) {      <----- bug fix
			emitExtendedHCRWarning(currentThread);
		}
		return rc;
	}

@tajila tajila changed the title Clear all breakpoints in class redefinition Clear breakpts in class redef and fix HCR warning Jun 18, 2024
@tajila
Copy link
Contributor Author

tajila commented Jun 18, 2024

With this fix, can the -XX:+EnableExtendedHCR option be removed from any of the tests where it was added by #19554 ?

Yes

@gacholio
Copy link
Contributor

jenkins test sanity zlinux jdk21

@pshipton
Copy link
Member

It seems another update to the tests is expected, setting as draft until it's ready.

@pshipton pshipton marked this pull request as draft June 18, 2024 15:32
@gacholio
Copy link
Contributor

gacholio commented Jun 20, 2024

I don't understand what you're waiting for - by definition, only our internal tests will ever need this option, and every test which uses extended must have the option because the default is off.

@tajila tajila force-pushed the ehcr branch 12 times, most recently from b410034 to ed417db Compare June 25, 2024 17:28
@tajila
Copy link
Contributor Author

tajila commented Jun 25, 2024

jenkins test sanity,extended zlinux jdk21

The current implementwation emits a warning message when a JVMTI
operation suceeds.

Also, clear breakpoints unconditionally when a class is redefined as
required by the spec.

Fixes: eclipse-openj9#19691

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

tajila commented Jun 26, 2024

jenkins test sanity,extended zlinux jdk21

@tajila tajila marked this pull request as ready for review June 26, 2024 20:12
@tajila
Copy link
Contributor Author

tajila commented Jun 26, 2024

@pshipton this is ready to go

@pshipton pshipton merged commit bd47476 into eclipse-openj9:master Jun 26, 2024
6 checks passed
@pshipton
Copy link
Member

Pls create the PR for 0.46.

pshipton added a commit to pshipton/openj9 that referenced this pull request Jul 5, 2024
Related to eclipse-openj9#19710
Issue eclipse-openj9#19765

Signed-off-by: Peter Shipton <Peter_Shipton@ca.ibm.com>
pshipton added a commit to pshipton/openj9 that referenced this pull request Jul 5, 2024
Related to eclipse-openj9#19710
Issue eclipse-openj9#19765
Issue eclipse-openj9#19813

Signed-off-by: Peter Shipton <Peter_Shipton@ca.ibm.com>
pshipton added a commit to pshipton/openj9 that referenced this pull request Jul 5, 2024
Related to eclipse-openj9#19710
Issue eclipse-openj9#19765
Issue eclipse-openj9#19813

Signed-off-by: Peter Shipton <Peter_Shipton@ca.ibm.com>
pshipton added a commit to pshipton/openj9 that referenced this pull request Jul 5, 2024
Related to eclipse-openj9#19710
Issue eclipse-openj9#19765
Issue eclipse-openj9#19813

Signed-off-by: Peter Shipton <Peter_Shipton@ca.ibm.com>
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.

3 participants