Split zOS system linkage call dependencies #1307

NigelYiboYu · 2018-02-28T20:55:00Z

Split the zOS system linkage call dependencies by attaching the
pre-dependencies to the BASR call instruction, and attaching the
post-dependencies to the function call descriptor instruction.

The C XPLINK return will need a NOP after the BASR for entry to valid
instructions. If they are separated by spills, returning from C code can
cause exceptions. Having a post-dependency group after the NOP ensures
that no such spill can happen.

This pull request also contains miscellaneous zOS system linkage code simplifications

1. Simplify NOP and call descriptor generation for zOS system linkage.

2. Use the insetPad() API to insert NOP after the BASR call in system
linkage.

3. Remove the oneliner createCallDescriptor() function.  Do the same thing inline.

4. genCallNOPAndDescriptor() does not have to be virtual. Remove the virtual
keyword.

Signed-off-by: Nigel Yu yunigel@ca.ibm.com

NigelYiboYu · 2018-03-02T22:56:36Z

@fjeremic Tested and ready for review.
Travis CI seems to be failing with gzip problems.

fjeremic · 2018-03-02T23:08:28Z

runtime/compiler/trj9/z/codegen/J9S390SystemLinkage.cpp

@@ -395,24 +395,35 @@ TR::J9S390zOSSystemLinkage::generateInstructionsForCall(TR::Node * callNode, TR:
         }
      }

-   gcPoint = generateRRInstruction(codeGen, TR::InstOpCode::BASR, callNode, systemReturnAddressRegister, systemEntryPointRegister, deps);
+   // NOP padding is needed because return from C function will skip the XPLink eyecatcher and
+   // always return to a point that's 4 bytes after the return address.


This is incorrect. It is 2 bytes after the call instruction BASR (which is itself 2 bytes). SO 4 bytes after the call instruction address, not the return address. The NOP we generate is 2 bytes in length. Can you fix this comment?

Currently, the JIT generates this for the JNI:

0d 76 BASR GPR7,GPR6 47 00 00 51 NOP

The BASR is 2 bytes. And the XPLINK return address in GPR7 is loaded by BASR, and should be pointing at the NOP, which is really a 4-byte branch on condition instruction that does not jump.

As an example, this is what xlc produces when I compile C++ with XPLINK option:

207 15650ZOS V2.3 z/OS XL C++ SimpleCall.cpp: Java_com_jni_c... 02/20/18 12:28:59 5 208 209 OFFSET OBJECT CODE LINE# FILE# P S E U D O A S S E M B L Y L I S T I N G 210 211 000124 987F 480C 000029 | LM r7,r15,2060(r4) 212 000128 4140 4080 000029 | LA r4,128(,r4) 213 00012C 47F0 7004 000029 | B 4(,r7)

The B 4(,7) instruction is 0x47f07004. It's branch on condition instruction with an unconditional jump mask 0xF. The offset is 4, in bytes.

But a few lines below we generate a 2 byte NOP:

//In 64 bit XPLINK, the caller returns at RetAddr+2, so add 2 byte nop cursor = new (trHeapMemory()) TR::S390NOPInstruction(TR::InstOpCode::NOP, 2, callNode, codeGen);

https://github.com/eclipse/openj9/pull/1307/files#diff-bd656156fc34aa9cbcf33d25dba26a0eR417

So one of these comments has to be incorrect. Looking at the documentation for XPLINK [1] it is indeed 4 bytes in length. We have 0x470XYYYY where X represents the call type which should be 0 as we're generating a BASR and YYYY should technically point to the entry point but this is not an XPLINK frame but a private JIT linkage frame so it too should be 0.

[1] https://www.ibm.com/support/knowledgecenter/en/SSLTBW_2.3.0/com.ibm.zos.v2r3.ceev100/cee1v283.htm

64bit XPLINK uses 2-byte NOP; and 31-bit XPLINK uses 4-byte NOP.
See TR::J9S390zOSSystemLinkage::genCallNOPAndDescriptor()

64bit XPLINK uses 2-byte NOP; and 31-bit XPLINK uses 4-byte NOP.

So if XPLINK 64-bit uses a 2 byte NOP and we branch "to a point that's 4 bytes after the return address" wouldn't we branch in the middle of an instruction?

Sorry, I wasn't clear about this.

The B 4(,7) example I showed you was 31-bit C++ epilogue in XPLINK.
64-bit C++ XPLINK return has a 2-byte offset:

193 0000F4 Start of Epilog 194 0000F4 E370 4818 0004 000029 | LG r7,2072(,r4) 195 0000FA 4140 4100 000029 | LA r4,256(,r4) 196 0000FE 47F0 7002 000029 | B 2(,r7)

So in our JIT'ed system call, the padding should be 2-byte for 64bit and 4-byte for 31-bit.

Ok, we need to properly document this as it is non-obvious. Could you amend the comment with some of the details regarding 31-bit vs. 64-bit XPLINK and what is expected as well as what the NOP is really supposed to represent (call type, call descriptor, etc.)?

That is true. @NigelYiboYu Padding is 2 Bytes for 64-Bit zOS and 4 Bytes for 31-bit zOS.
https://github.com/eclipse/openj9/blob/26f8c7edb1fe37253d001243c0ce14473334d07f/runtime/compiler/trj9/z/codegen/J9S390CHelperLinkage.cpp#L348

Updated comments in SystemLinkage and CHelperLinkage classes.

fjeremic · 2018-03-02T23:09:19Z

runtime/compiler/trj9/z/codegen/J9S390SystemLinkage.cpp

   if (TR::Compiler->target.is64Bit())
      {
      //In 64 bit XPLINK, the caller returns at RetAddr+2, so add 2 byte nop
-      TR::Instruction * cursor = new (trHeapMemory()) TR::S390NOPInstruction(TR::InstOpCode::NOP, 2, callNode, codeGen);
+      cursor = new (trHeapMemory()) TR::S390NOPInstruction(TR::InstOpCode::NOP, 2, callNode, codeGen);


Can you use the OMR::Z::CodeGenerator::insertPad API here instead?

OK. I'll try insertPad API.

I refactored and simplified the BASR padding function in system linkage class to use insertPad() API. I'm currently running tests.

NigelYiboYu · 2018-03-05T15:57:33Z

Re-testing my changes. Marking this as WIP.

fjeremic · 2018-03-06T16:23:23Z

runtime/compiler/trj9/z/codegen/J9S390SystemLinkage.cpp

+   //In 31 bit XPLINK, the caller returns at RetAddr+4, so add 4 byte nop with last two bytes
+   // as signed offset in doublewords at or preceding NOP
+   uint32_t padSize = TR::Compiler->target.is64Bit() ? 2 : 4;
+   cursor = nop = new (trHeapMemory()) TR::S390NOPInstruction(TR::InstOpCode::NOP, padSize, node, codeGen);


Can we use the insertPad API here? Also spaces between // and the comment.

NigelYiboYu · 2018-03-08T16:36:38Z

Re-tested my changes after using insertPad(). Test passed. @fjeremic

Split the zOS system linkage call dependencies by attaching the pre-dependencies to the BASR call instruction, and attaching the post-dependencies to the function call descriptor instruction. The C XPLINK return will need a NOP after the BASR for entry to valid instructions. If they are separated by spills, returning from C code can cause exceptions. Having a post-dependency group after the NOP ensures that no such spill can happen. Signed-off-by: Nigel Yu <yunigel@ca.ibm.com>

1. Simplify NOP and call descriptor generation for zOS system linkage. 2. Use the insetPad() API to insert NOP after the BASR call in system linkage. 3. Remove createCallDescriptor() function and do the same thing inline. 4. genCallNOPAndDescriptor() does not have to be virtual. Remove the virtual keyword. Signed-off-by: Nigel Yu <yunigel@ca.ibm.com>

fjeremic · 2018-04-02T20:47:44Z

Jenkins test all

fjeremic · 2018-04-03T18:14:32Z

Failures seem infrastructure issues cloning the repos. The Linux s390 passed though which is all that should matter for this change.

NigelYiboYu force-pushed the system-link-break-deps branch 3 times, most recently from b599808 to 76ae8ba Compare February 28, 2018 21:02

NigelYiboYu closed this Mar 2, 2018

NigelYiboYu reopened this Mar 2, 2018

NigelYiboYu changed the title ~~WIP: Split zOS system linkage call dependencies~~ Split zOS system linkage call dependencies Mar 2, 2018

NigelYiboYu force-pushed the system-link-break-deps branch from 76ae8ba to c46185e Compare March 2, 2018 22:54

fjeremic added the comp:jit label Mar 2, 2018

fjeremic self-assigned this Mar 2, 2018

fjeremic reviewed Mar 2, 2018

View reviewed changes

NigelYiboYu force-pushed the system-link-break-deps branch from c46185e to 23f218f Compare March 5, 2018 15:55

NigelYiboYu changed the title ~~Split zOS system linkage call dependencies~~ WIP: Split zOS system linkage call dependencies Mar 5, 2018

NigelYiboYu force-pushed the system-link-break-deps branch 2 times, most recently from bed4bd7 to 5f51f3a Compare March 5, 2018 18:22

NigelYiboYu changed the title ~~WIP: Split zOS system linkage call dependencies~~ Split zOS system linkage call dependencies Mar 6, 2018

fjeremic reviewed Mar 6, 2018

View reviewed changes

NigelYiboYu force-pushed the system-link-break-deps branch from 5f51f3a to 3210262 Compare March 6, 2018 18:28

NigelYiboYu changed the title ~~Split zOS system linkage call dependencies~~ WIP: Split zOS system linkage call dependencies Mar 6, 2018

NigelYiboYu force-pushed the system-link-break-deps branch from 3210262 to 488f09e Compare March 7, 2018 23:27

NigelYiboYu changed the title ~~WIP: Split zOS system linkage call dependencies~~ Split zOS system linkage call dependencies Mar 7, 2018

NigelYiboYu added 2 commits March 25, 2018 18:06

NigelYiboYu force-pushed the system-link-break-deps branch from 488f09e to cc5dc68 Compare March 25, 2018 22:07

fjeremic approved these changes Apr 2, 2018

View reviewed changes

fjeremic merged commit 45c7cee into eclipse-openj9:master Apr 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split zOS system linkage call dependencies #1307

Split zOS system linkage call dependencies #1307

NigelYiboYu commented Feb 28, 2018 •

edited

Loading

NigelYiboYu commented Mar 2, 2018

fjeremic Mar 2, 2018 •

edited

Loading

NigelYiboYu Mar 2, 2018 •

edited

Loading

fjeremic Mar 3, 2018 •

edited

Loading

NigelYiboYu Mar 4, 2018 •

edited

Loading

fjeremic Mar 5, 2018 •

edited

Loading

NigelYiboYu Mar 5, 2018 •

edited

Loading

fjeremic Mar 5, 2018

r30shah Mar 5, 2018

NigelYiboYu Mar 5, 2018

fjeremic Mar 2, 2018

NigelYiboYu Mar 2, 2018

NigelYiboYu Mar 5, 2018

NigelYiboYu commented Mar 5, 2018

fjeremic Mar 6, 2018

NigelYiboYu commented Mar 8, 2018

fjeremic commented Apr 2, 2018

fjeremic commented Apr 3, 2018

Split zOS system linkage call dependencies #1307

Split zOS system linkage call dependencies #1307

Conversation

NigelYiboYu commented Feb 28, 2018 • edited Loading

NigelYiboYu commented Mar 2, 2018

fjeremic Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

NigelYiboYu Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

fjeremic Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

NigelYiboYu Mar 4, 2018 • edited Loading

Choose a reason for hiding this comment

fjeremic Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

NigelYiboYu Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

fjeremic Mar 5, 2018

Choose a reason for hiding this comment

r30shah Mar 5, 2018

Choose a reason for hiding this comment

NigelYiboYu Mar 5, 2018

Choose a reason for hiding this comment

fjeremic Mar 2, 2018

Choose a reason for hiding this comment

NigelYiboYu Mar 2, 2018

Choose a reason for hiding this comment

NigelYiboYu Mar 5, 2018

Choose a reason for hiding this comment

NigelYiboYu commented Mar 5, 2018

fjeremic Mar 6, 2018

Choose a reason for hiding this comment

NigelYiboYu commented Mar 8, 2018

fjeremic commented Apr 2, 2018

fjeremic commented Apr 3, 2018

NigelYiboYu commented Feb 28, 2018 •

edited

Loading

fjeremic Mar 2, 2018 •

edited

Loading

NigelYiboYu Mar 2, 2018 •

edited

Loading

fjeremic Mar 3, 2018 •

edited

Loading

NigelYiboYu Mar 4, 2018 •

edited

Loading

fjeremic Mar 5, 2018 •

edited

Loading

NigelYiboYu Mar 5, 2018 •

edited

Loading