Update JNI Target relocation record to have offset to the reloLocation. #14421
Conversation
Normally the relocation records are generated at binary encoding, when the offset to patch relative to the start of the method is known. However, sometimes a relocation record has to be generated before. This commit fixes some such locations that did not use the TR::BeforeBinaryEncodingExternalRelocation. Signed-off-by: Irwin D'Souza <[email protected]>
|
@jdmpapin do you mind reviewing this and the associated OMR PR? |
|
Mixed it up with the OMR PR... |
|
Jenkins test sanity+aot all jdk17 depends eclipse/omr#6332 |
|
Worth noting all tests passted except for aarch64, which failed because of Will force push the build break fix. As per https://openj9-jenkins.osuosl.org/job/PullRequest-OpenJ9/1574/ only the builds passed, I guess the CI was too unstable and didn't actually test anything... |
On x86, the JNI Target relocation puts the incorrect address when generating a Runtime Assumption. However, the address that the relocation infra itself needs to patch is correct. Thus, the relo record has to hold not just the location to patch but also the location to register the assumption against. This is the same on most platforms, but different on x86. Signed-off-by: Irwin D'Souza <[email protected]>
|
Jenkins test sanity+aot all jdk17 depends eclipse/omr#6332 |
1 similar comment
|
Jenkins test sanity+aot all jdk17 depends eclipse/omr#6332 |
|
Ok well jenkins is too unstable to launch the PR tests so I guess we're on standby till then.. |
|
Jenkins test sanity.functional+aot all jdk17 depends eclipse/omr#6332 |
|
Jenkins test sanity.functional+aot all jdk17 depends eclipse/omr#6332 |
|
The failing tests are due to
1 should be fixed now and the test in 2 has been disabled. |
|
Jenkins test sanity.functional+aot all jdk17 depends eclipse/omr#6332 launching again just for sanity purposes. |
|
aix failures are due to:
x86 failure is due to:
|
|
The other aix failure is in which happens on the second run. Considering the method at the top of the stack is a native method, maybe there's something wrong with the relocation on power. |
|
DirectToJNI is disabled on P as the query [1] is not overridden. All of my changes on P are related to the following methods: These only run when generating directToJNI code. I verified that the relocation isn't generated by running a unit test @jdmpapin has written. As such, the failing test can't be related to my changes. FWIW: ... |
|
@jdmpapin do you mind merging this if everything looks good to you? |
|
All observed test failures are unrelated just as @dsouzai described above. Re-launching tests for a coordinated merge (both to include jdk8,jdk11 and to ensure that the test results will be recent) Jenkins test sanity all jdk8,jdk11,jdk17 depends eclipse/omr#6332 |
|
Should we be adding this fix to the Java 18 release? |
|
We could. It's a very low likelihood issue in practice (which explains why it's only been reported now) but at the same time, it's 100% reproducible in Devin's unit test. What kind of changes are generally expected to be cherry-picked into a release? If it's any kind of bug fix then I guess this would fall into that category. |
|
We want to deliver fixes that may impact users and are of low risk for breaking something else. If it's low risk we could just put it in, even if not sure of the impact on users. |
|
Sometimes we want changes to stew for a while in the head stream before backporting, to ensure they aren't going to cause additional issues. If this is one of those things then given "very low likelihood in practice" we shouldn't backport if it needs to stew longer than it already has. |
|
Ok I'll open a set of PRs for openj9-omr/openj9 0.31 shortly. EDIT: This was in response to #14421 (comment) |
Sorry, I didn't seet his when I posted my earlier comment (and I just got the email notification for it for some reason). I mean the PR involves changing the raw relocation data stored to the SCC for these directToJNI relocations, which had to be done across all platforms, so while the change is relatively straightforward, it does have a big scope. Additionally there is a workaround for the problem, namely The combination of these two facts leads me to personally believe we don't need to double deliver. However, if others feel differently I have no problem opening the necessary PRs. Thoughts @mstoodle @DanHeidinga ? Also I just realized I should have also changed the JITserver minor version -_- |
eclipse-openj9#14421 changed the layout of the relocation data. As such, the JITServer minor version has to be updated. Signed-off-by: Irwin D'Souza <[email protected]>
|
Agree, we haven't seen it in practice until now so I think it can wait for 0.32 . Can/has Devin's test case be(en) added to our tests? |
In theory I suppose it can, but because it involves compiling (using a platforms specific native compiler) the native code as a library, it isn't as straightforward as a test that's purely java code. We'd need to get the help of the test team to best determine how to get this test added. |
On x86, the JNI Target relocation puts the incorrect address when
generating a Runtime Assumption. However, the address that the
relocation infra itself needs to patch is correct. Thus, the relo record
has to hold not just the location to patch but also the location to
register the assumption against. This is the same on most platforms, but
different on x86.
Depends on eclipse/omr#6326Depends on eclipse/omr#6331Depends on eclipse/omr#6332
Fixes #14300