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

Assert dstObj in arraycopyEval is not a dataAddrPtr in X/Z #20480

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

rmnattas
Copy link
Contributor

@rmnattas rmnattas commented Nov 1, 2024

For OffHeap, the arraycopy transformations uses the correct base object as the dstObj node, instead of the dataAddrPtr load.
To ensure correctness this PR adds an asserts that checks that the dstObj is not a dataAddrPtr.

Valid:
n60n        arraycopy  java/lang/System.arraycopy(Ljava/lang/Object;ILjava/lang/Object;II)V[#405  unresolved notAccessed static Method] [flags 0x400 0x0 ] (Unsigned forwardArrayCopy noArra>
n58n          aload  <parm 0 [Ljava/lang/Object;>[#403  Parm]                          
n59n          aload  <parm 1 [Ljava/lang/Object;>[#404  Parm]                            
n54n          aloadi  <contiguousArrayDataAddrFieldSymbol>[#352  final Shadow +8] [flags 0x20604 0x0 ] (internalPtr )  [    0x78f0fb0e00a0] bci=[-1,0,20] rc=1 vc=33 vn=- li=- udi=- nc=1 fl>
n55n            aload  <parm 0 [Ljava/lang/Object;>[#403  Parm]                
n56n          aloadi  <contiguousArrayDataAddrFieldSymbol>[#352  final Shadow +8] [flags 0x20604 0x0 ] (internalPtr )  [    0x78f0fb0e0140] bci=[-1,2,20] rc=1 vc=33 vn=- li=- udi=- nc=1 fl>
n57n            aload  <parm 1 [Ljava/lang/Object;>[#404  Parm]                         
n53n          lconst 8 (highWordZero X!=0 X>=0 cannotOverflow )             

Not-Valid:
n60n        arraycopy  java/lang/System.arraycopy(Ljava/lang/Object;ILjava/lang/Object;II)V[#405  unresolved notAccessed static Method] [flags 0x400 0x0 ] (Unsigned forwardArrayCopy noArra>
n58n          aload  <parm 0 [Ljava/lang/Object;>[#403  Parm]       
n62n          aloadi  <contiguousArrayDataAddrFieldSymbol>[#352  final Shadow +8] [flags 0x20604 0x0 ] (internalPtr )  [    0x78f0fb0e00a0] bci=[-1,0,20] rc=1 vc=33 vn=- li=- udi=- nc=1 fl>                   
n59n            aload  <parm 1 [Ljava/lang/Object;>[#404  Parm]                            
n54n          aloadi  <contiguousArrayDataAddrFieldSymbol>[#352  final Shadow +8] [flags 0x20604 0x0 ] (internalPtr )  [    0x78f0fb0e00a0] bci=[-1,0,20] rc=1 vc=33 vn=- li=- udi=- nc=1 fl>
n55n            aload  <parm 0 [Ljava/lang/Object;>[#403  Parm]                
n56n          aloadi  <contiguousArrayDataAddrFieldSymbol>[#352  final Shadow +8] [flags 0x20604 0x0 ] (internalPtr )  [    0x78f0fb0e0140] bci=[-1,2,20] rc=1 vc=33 vn=- li=- udi=- nc=1 fl>
n57n            aload  <parm 1 [Ljava/lang/Object;>[#404  Parm]                         
n53n          lconst 8 (highWordZero X!=0 X>=0 cannotOverflow )      

Reason being the writeBarrier code assumes the dstObj node being the base object and uses that for card-dirtying address calculation.

Following: #20264
P/AArch64 in OMR: eclipse-omr/omr#7500

@rmnattas rmnattas marked this pull request as ready for review December 11, 2024 14:06
@rmnattas rmnattas changed the title Add not dataAddrPtr assert in arraycopyEval for dstObj Assert dstObj in arraycopyEval is not a dataAddrPtr in X/Z Dec 11, 2024
@rmnattas
Copy link
Contributor Author

Both Openj9/OMR are ready for review and merge, no need for coordination.

Copy link
Contributor

@zl-wang zl-wang left a comment

Choose a reason for hiding this comment

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

LGTM

@zl-wang
Copy link
Contributor

zl-wang commented Dec 11, 2024

Jenkins test sanity.functional xlinux,zlinux jdk21

@zl-wang
Copy link
Contributor

zl-wang commented Dec 11, 2024

hmm ... zlinux JLM testcase failed. @rmnattas please check it out!

@rmnattas
Copy link
Contributor Author

rmnattas commented Dec 11, 2024

Hmm.. output matches expected, will confirm failure reason.

[2024-12-11T19:29:39.763Z] PASSED: testToString
[2024-12-11T19:29:39.763Z] FAILED: testThreadMXBeanProxy
[2024-12-11T19:29:39.763Z] java.lang.AssertionError: expected:<"main" prio=5 Id=2 RUNNABLE
[2024-12-11T19:29:39.763Z] > but was:<"main" prio=5 Id=2 RUNNABLE
[2024-12-11T19:29:39.763Z] >
[2024-12-11T19:29:39.764Z] 	at org.testng.AssertJUnit.fail(AssertJUnit.java:59)
[2024-12-11T19:29:39.764Z] 	at org.testng.AssertJUnit.failNotEquals(AssertJUnit.java:364)
[2024-12-11T19:29:39.764Z] 	at org.testng.AssertJUnit.assertEquals(AssertJUnit.java:80)
[2024-12-11T19:29:39.764Z] 	at org.testng.AssertJUnit.assertEquals(AssertJUnit.java:88)
[2024-12-11T19:29:39.764Z] 	at org.openj9.test.java.lang.management.TestManagementFactory.testThreadMXBeanProxy(TestManagementFactory.java:1335)
[2024-12-11T19:29:39.764Z] 	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
[2024-12-11T19:29:39.764Z] 	at java.base/java.lang.reflect.Method.invoke(Method.java:586)

@rmnattas
Copy link
Contributor Author

Seem like a known issue, #18920

@zl-wang zl-wang merged commit 5d9f84b into eclipse-openj9:master Dec 13, 2024
5 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants