Fix calculation of XPLINK call descriptor offset #4154
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Although this change is in OMR I have only been able to produce this bug via OpenJ9 since OMR does not support calling non-XPLINK targets. I'm attaching the test case suite to this comment below. It adapts the JNI tests found in [1] as a standalone since we need to compile the said tests with non-XPLINK linkage. In the ZIP file attached you may find both ASCII and EBCDIC logs of a sample of 4 method compilations which have different alignments modulo 8 for an exhaustive confirmation that the XPLINK offset descriptor is now encoded correctly. I'll copy/paste the relevant snippets here for the reviewer's convenience: nopLocationIs0Modulo8.trace.ascii: nopLocationIs2Modulo8.trace.ascii: nopLocationIs4Modulo8.trace.ascii: nopLocationIs6Modulo8.trace.ascii (note the float parameter descriptor is properly encoded): [1] https://github.com/eclipse/openj9/tree/master/runtime/tests/jniarg |
joransiu
reviewed
Jul 24, 2019
The call descriptor offset in the NOP following a call within an XPLINK frame was not being correctly calculated. The patch location was correct, however the offset encoded was from the patch location, which is incorrect. As per the documentation linked in the source code the value we wish to encoding is a (positive) signed offset in doublewords from the doubleword at or preceding the return point (NOP) to the XPLINK call descriptor for this signature. This is not something we can encode with the existing relocations, and due to the doubleword alignment that is required it is not something that we should generalize either since we don't envision other uses for such a strict API. Instead we create a private relocation in the XPLINK linkage class and handle the relocation there. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
fjeremic
force-pushed
the
fix-nop-call-descriptor
branch
from
d0f0416
to
6810584
Compare
…location failsafe Fail the compilation if we are unable to encode the distance to the XPLINKCallDescriptorSnippet. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
joransiu
approved these changes
Jul 24, 2019
|
@genie-omr build zlinux,zos |
Use the already calculated offsetToCallDescriptor instead of recalculating it again. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
fjeremic
force-pushed
the
fix-nop-call-descriptor
branch
from
6810584
to
6c0ad4f
Compare
|
Commits well structured, only affects compiler on Z platform so Z testing is sufficient, code has been reviewed by two developers with Z background. Since @fjeremic can't merge it himself, I'll do it. Thanks, everyone! |
fjeremic
added a commit
to fjeremic/openj9
that referenced
this pull request
Sep 10, 2019
The fatal bug fixed in eclipse-omr/omr#4154 was not caught during automated testing because there currently do not exist JNI tests which are built with non-XPLINK (system) linkage. To prevent such obvious bugs from escaping into production we reuse the jniargtests and compile them with system linkage on z/OS 31-bit. We modify the existing Java test so it can run against custom native libraries, then we build two native libraries on z/OS, one with XPLINK which is already done today, and one with system linkage which will exercise callouts to non-XPLINK natives. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The call descriptor offset in the NOP following a call within an XPLINK
frame was not being correctly calculated. The patch location was
correct, however the offset encoded was from the patch location, which
is incorrect.
As per the documentation linked in the source code the value we wish to
encoding is a (positive) signed offset in doublewords from the
doubleword at or preceding the return point (NOP) to the XPLINK call
descriptor for this signature.
This is not something we can encoding with the existing locations, and
due to the doubleword alignment that is required it is not something
that we should generalize either since we don't envision other uses for
such a strict API. Instead we create a private relocation in the XPLINK
linkage class and handle the relocation there.
Signed-off-by: Filip Jeremic fjeremic@ca.ibm.com