Add full support for calls on z/OS and enable corresponding tests #4907

fjeremic · 2020-03-06T21:25:35Z

This PR contains a series of commits which enables full support for JIT <-> native calls on z/OS. The change appears rather large due to the amount of test code that was enhanced to validate the support. There was also other issues encountered during development which were fixed. Please see the commit descriptions for details on what each change does and why.

The "meat" of the support can be found in the two commits titled:

"Create the XPLINK Function Descriptor for OMR compiled bodies on z/OS"
"Use method symbol address to get the entry point in Tril and JitBuilder"

These two commits give us the support and are not very large changes all in all.

Issue: #4719

fjeremic · 2020-03-06T21:27:05Z

Leaving this in draft mode as I still need to validate that I haven't broken OpenJ9 on z/OS, which I most certainly have, but the fix will be simple as soon as I test it. In the mean while this should be ready for review. I'll proceed and self-review the change to note the most important design decisions which we can discuss here.

@genie-omr build all

fjeremic · 2020-03-06T21:29:06Z

compiler/control/CompileMethod.cpp

@@ -379,7 +379,7 @@ compileMethodFromDetails(
         // not ready yet...
         //OMR::MethodMetaDataPOD *metaData = fe.createMethodMetaData(&compiler);

-         startPC = compiler.cg()->getCodeStart();
+         startPC = (uint8_t*)compiler.getMethodSymbol()->getMethodAddress();


This was the main change in thinking here. See the commit body (ab6d6a4) for a full description of why we made this change.

To be explicit this is a design decision which I'd like @mstoodle and @aviansie-ben to comment on, and perhaps @Leonardo2718 as well. It is unfortunate we don't have a public API, because in that case getCodeStart would likely be a private API internal to the compiler, and we would only provide one public API to extract the "pointer to a function" we compiled with OMR.

Do we agree with this approach? If not, any suggestions on an alternative?

This approach seems fine to me.

I agree that having a single public API to extract the function pointer would be ideal. But I don't think that's a problem that needs to be solved here. The approach you've taken is reasonable given what we currently have, IMHO.

fjeremic · 2020-03-06T21:29:58Z

compiler/z/codegen/SystemLinkagezOS.cpp

+          genLoadAddressConstant(cg(), callNode, reinterpret_cast<uintptrj_t>(fd), epReg);
+          generateRSInstruction(cg(), TR::InstOpCode::getLoadMultipleOpCode(), callNode, aeReg, epReg, generateS390MemoryReference(epReg, 0, cg()));
+
+          TR::Instruction* callInstr = new (cg()->trHeapMemory()) TR::S390RILInstruction(TR::InstOpCode::BRASL, callNode, raReg, fd->func, callSymRef, cg());
+          callInstr->setDependencyConditions(preDeps);


Loads the function's address from the function descriptor and dispatches the call.

fjeremic · 2020-03-06T21:30:39Z

jitbuilder/control/Jit.cpp

-#if defined(J9ZOS390)
-   struct FunctionDescriptor
-   {
-      uint64_t environment;
-      void* func;
-   };
-
-   FunctionDescriptor* fd = new FunctionDescriptor();
-   fd->environment = 0;
-   fd->func = *entry;
-
-   *entry = (void*) fd;
-#elif defined(AIXPPC)


Finally! This hack is now removed and we don't need to do anything special for z/OS. Once AIX gets support we can also remove the AIX piece as well.

fjeremic · 2020-03-06T21:31:31Z

jitbuilder/release/cpp/samples/Mandelbrot.cpp

+
+   // The file we generate here is in the PPM image format which has a simple header describing how to interpret the
+   // buffer. This string needs to be encoded as ASCII in the file generated, however on platforms such as z/OS whose
+   // native encoding is not ASCII this is problematic.
+   //
+   // TODO (#4901): Use the port library to output the header bytes for PPM image file format


Linking this PR with #4901 so GitHub notes the link for future whenever we fix this.

fjeremic · 2020-03-06T21:32:22Z

Also suggesting @aviansie-ben for review as well as I cannot tag him on the GitHub interface.

fjeremic · 2020-03-06T21:42:04Z

compiler/codegen/OMRCodeGenPhase.cpp

-     if (comp->getCurrentMethod() == NULL)
-        {
-        comp->getMethodSymbol()->setMethodAddress(cg->getBinaryBufferStart());
-        }


This piece of code was alarming to me. Does anyone know why this exists? And why is it guarded by a getCurrentMethod null check? When would this value ever be NULL?

mstoodle · 2020-03-06T21:44:46Z

compiler/codegen/OMRCodeGenPhase.cpp

+         }
+      cg->getAheadOfTimeCompile()->dumpRelocationData();
+      traceMsg(comp, "</relocatableDataCG>\n");
+      }


any chance you could split these changes into a separate commit? they interfere with appreciating the setup you've made here for a much better solution to the FD issue.

The reason this formatting change was made is because the indentation in it's current state is very confusing [1], and at first sight one may think the:

if (comp->getCurrentMethod() == NULL) { comp->getMethodSymbol()->setMethodAddress(cg->getBinaryBufferStart()); }

piece of code is guarded by the if (cg->getAheadOfTimeCompile() && check, which is what I first thought, but upon a more careful inspection the indentation was hiding this is not the case.

If you wish to view that commit without whitespace changes GitHub can help here:
ab6d6a4?w=1

[1] https://github.com/eclipse/omr/blob/31db81f3d84a431736ac7a9ff1b3980158fae258/compiler/codegen/OMRCodeGenPhase.cpp#L182-L202

On second thought, I think I agree. For history of commits it would be beneficial to split up formatting from actual changes.

I've split up the actual change into 6e902d5, where it belongs, and the formatting change into 27349e8. Hope that helps!

fjeremic · 2020-03-06T23:34:48Z

@genie-omr build all

fjeremic · 2020-03-09T18:08:27Z

Just remembered my comment [1] to myself to revert some tests we disabled because of the lack of support for callouts. Now that we have them we can re-enable the tests.

[1] #4719 (comment)

fjeremic · 2020-03-09T18:08:36Z

@genie-omr build all

fjeremic · 2020-03-10T19:55:13Z

Leaving this in draft mode as I still need to validate that I haven't broken OpenJ9 on z/OS, which I most certainly have, but the fix will be simple as soon as I test it.

Much to my surprise I was wrong. OpenJ9 works fine with this change. The reason is because generateInstructionsForCall is a virtual method [1] and OpenJ9 overrides this in it's JNI linkage, which inherits the XPLINK system linkage on z/OS [2].

I've validated OpenJ9 works perfectly fine with this change, hence I'm marking this PR as ready for review/merging.

[1] https://github.com/eclipse/omr/blob/b03105e81b1b020900afbb2871cbd45aaaf8048c/compiler/z/codegen/SystemLinkage.hpp#L118-L120
[2] https://github.com/eclipse/openj9/blob/e8215634090734f30ba084a83089b58dbe579a3b/runtime/compiler/z/codegen/J9SystemLinkagezOS.cpp#L65-L218

In this change we rewrite the Call sample with a few enhancements to introduce all call types: - JIT to JIT - JIT to native - JIT to computed native - Native to JIT The test computation is still the same, i.e. we still compute a double sum, however we do it in several different ways so as to test calls both to and from JIT/native to ensure all paths are working as expected. This is particularly useful for platforms which have special handling of function pointers, such as AIX and z/OS. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

Now that we have full support for calls on z/OS we can safely enable all Tril tests which were previously disabled. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

OMR nor any downstream projects support 31-bit XPLINK to OS linkage, and the support is also not present in the rest of the compiler. This code block is slated for deprecation as we don't anticipate it being used. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

The path for loading double arguments for outgoing calls was disabled within the `J9_PROJECT_SPECIFIC` ifdef, and as such double arguments were not being loaded for outgoing calls. We fix this by moving the case label outside of the ifdef. In addition there is no need to check for `getNumIntegerArgumentRegisters` before incrementing the GPR index, similarly to the integer cases since the `getIntegerArgumentRegister` API will already return the default value if the register index does not participate in the calling convention. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

fjeremic · 2020-03-13T16:43:17Z

I had to rebase to absorb the changes in #4821 to use the new SKIP_ON_<ARCH> helpers in LinkageTest.cpp and MinimalTest.cpp. Rest of the change is as before.

@genie-omr build all

Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

fjeremic · 2020-03-20T17:05:06Z

I had to rebase again to fix conflicts introduced by removal of uintptrj_t in #4931. Launching builds again:

@genie-omr build all

Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

fjeremic · 2020-03-20T18:46:09Z

@genie-omr build all

fjeremic · 2020-03-23T21:22:19Z

@mstoodle @0xdaryl @Leonardo2718 any takers for a committer for this one?

fjeremic · 2020-03-25T15:38:18Z

As per comment by @mstoodle (#4964 (comment)) I've added the patch from #4909 to this PR in 2e7226c so we have test coverage. Launching builds again.

@genie-omr build all

fjeremic · 2020-03-25T15:57:35Z

Looks like there are issues on the following builds:
https://ci.eclipse.org/omr/job/PullRequest-linux_x86-64/2027/
https://ci.eclipse.org/omr/job/PullRequest-osx_x86-64/1581/

I'm going to back out 2e7226c from this PR and reopen #4909 for further investigation.

Leonardo2718

I'm not done reviewing this yet, but here are some comments I have so far. Mostly minor stuff that doesn't necessarily have to be addressed here. I just wanted to write down my thoughts so far so I don't forget.

compiler/z/codegen/OMRLinkage.cpp

fvtest/compilertriltest/LinkageTest.cpp

Leonardo2718

LGTM 👍

@fjeremic I'm fine with the changes as they are. I did have a few questions but they are minor enough they don't need to be addressed here. I'll let you decide what to do with them.

fvtest/compilertriltest/LinkageTest.cpp

Leonardo2718 · 2020-03-30T19:51:30Z

fvtest/compilertriltest/LinkageTest.cpp

+       TypeToString<TypeParam>::type, // args
+       TypeToString<TypeParam>::prefix, // return
+       TypeToString<TypeParam>::prefix, //call
+       reinterpret_cast<uintmax_t>(static_cast<TypeParam(*)(TypeParam)>(callee_entry_point)), // address


Hm.... I'm not completely sure if that inner static_cast is needed.

As I recall, that cast is needed to work around a bug in MSVC (I think) involving incorrect handling of pointers to template functions. Technically, callee_entry_point is not a template function. However, it's type is template type dependent. So, I'm not sure if it triggers the same bug.

You're right. All the builds passed. Fixed in 11c8008

If it worked in this one location, I would think that it should also work for every other test that uses callee_entry_point.

Right. I've force pushed the fix to cover all uses in fa96327

Leonardo2718 · 2020-03-30T20:16:26Z

fvtest/compilertriltest/LinkageTest.cpp

+}
+
+template <typename T>
+T (*getJitCompiledMethodReturning1stArgument())(T) {


Man, gotta love that function signature 😝

Leonardo2718 · 2020-03-30T21:02:41Z

compiler/control/CompileMethod.cpp

@@ -379,7 +379,7 @@ compileMethodFromDetails(
         // not ready yet...
         //OMR::MethodMetaDataPOD *metaData = fe.createMethodMetaData(&compiler);

-         startPC = compiler.cg()->getCodeStart();
+         startPC = (uint8_t*)compiler.getMethodSymbol()->getMethodAddress();


This approach seems fine to me.

I agree that having a single public API to extract the function pointer would be ideal. But I don't think that's a problem that needs to be solved here. The approach you've taken is reasonable given what we currently have, IMHO.

fjeremic · 2020-03-30T21:18:01Z

@Leonardo2718 thanks for the review. I still want to address your earlier points. I think there is no better time do implement such work than now. I'll have fixes for those in the next day. Apologies for the slow progress.

Leonardo2718 · 2020-03-31T15:13:39Z

Apologies for the slow progress.

Well, if we're going to apologize for slow progress, I should apologies for taking so long to review. 😅

Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

fjeremic · 2020-03-31T16:44:16Z

@genie-omr build all

fjeremic · 2020-03-31T17:25:34Z

All review comments addressed. Please see each review for a reference to a commit which addresses the review concern. @Leonardo2718 do you want to be a committer for this one?

Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

These helper macros will skip tests based on architecture and operating system combinations. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

Leonardo2718

LGTM 👍

Leonardo2718 · 2020-03-31T19:23:57Z

@genie-omr build all

I want to merge this tomorrow by EOD. If anyone still wants to review this change, please speak up before then.

Leonardo2718 · 2020-04-02T14:11:53Z

Times up! Merging now.

fjeremic added comp:compiler comp:test comp:jitbuilder comp:tril ci os:zos arch:z labels Mar 6, 2020

fjeremic commented Mar 6, 2020

View reviewed changes

fjeremic requested review from Leonardo2718, mstoodle and 0xdaryl March 6, 2020 21:31

fjeremic commented Mar 6, 2020

View reviewed changes

mstoodle reviewed Mar 6, 2020

View reviewed changes

fjeremic mentioned this pull request Mar 6, 2020

Crash in JitBuilder when calling a method via the TR::MethodBuilder *calleeMB API #4909

Closed

fjeremic force-pushed the jitbuilder-native branch from 907ae0c to 7dc9546 Compare March 6, 2020 23:33

fjeremic force-pushed the jitbuilder-native branch from 9677c26 to b14ad25 Compare March 9, 2020 18:07

fjeremic marked this pull request as ready for review March 10, 2020 19:55

fjeremic requested review from charliegracie, rwy7 and youngar as code owners March 10, 2020 19:55

fjeremic added 4 commits March 13, 2020 11:53

Enable Tril tests with calls on z/OS

7b3ecf2

Now that we have full support for calls on z/OS we can safely enable all Tril tests which were previously disabled. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

fjeremic requested a review from mstoodle March 13, 2020 16:43

Replace uintptrj_t with uintptr_t

68fafb1

Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

Add missing OMRPORT_ACCESS_FROM_OMRPORT in LinkageTest

0bd553a

Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

mstoodle mentioned this pull request Mar 23, 2020

Prepare compiled MethodBuilder for use in later Call() #4964

Merged

fjeremic force-pushed the jitbuilder-native branch from 2e7226c to 0bd553a Compare March 25, 2020 15:59

Leonardo2718 reviewed Mar 26, 2020

View reviewed changes

compiler/z/codegen/OMRLinkage.cpp Outdated Show resolved Hide resolved

fvtest/compilertriltest/LinkageTest.cpp Outdated Show resolved Hide resolved

Leonardo2718 approved these changes Mar 30, 2020

View reviewed changes

Simplify if/else to ternary in OMRLinkage.cpp

f5f00a4

Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

fjeremic force-pushed the jitbuilder-native branch 2 times, most recently from 6ea5f84 to 13a5b43 Compare March 31, 2020 16:43

fjeremic requested a review from Leonardo2718 March 31, 2020 17:25

Leonardo2718 self-assigned this Mar 31, 2020

fjeremic added 2 commits March 31, 2020 14:44

Remove redundant static_cast in LinkageTest.cpp

fa96327

Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

Add SKIP_ON_S390_LINUX macros and use them in LinkageTest.cpp

cf60a96

These helper macros will skip tests based on architecture and operating system combinations. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

fjeremic force-pushed the jitbuilder-native branch from 13a5b43 to cf60a96 Compare March 31, 2020 18:44

Leonardo2718 approved these changes Mar 31, 2020

View reviewed changes

Leonardo2718 merged commit 3240dfd into eclipse-omr:master Apr 2, 2020

fjeremic deleted the jitbuilder-native branch April 2, 2020 14:17

Add full support for calls on z/OS and enable corresponding tests #4907

Add full support for calls on z/OS and enable corresponding tests #4907

Conversation

fjeremic commented Mar 6, 2020

fjeremic commented Mar 6, 2020

Choose a reason for hiding this comment

fjeremic Mar 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjeremic commented Mar 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjeremic Mar 6, 2020 • edited Loading

Choose a reason for hiding this comment

fjeremic Mar 6, 2020 • edited Loading

Choose a reason for hiding this comment

fjeremic commented Mar 6, 2020

fjeremic commented Mar 9, 2020

fjeremic commented Mar 9, 2020

fjeremic commented Mar 10, 2020

fjeremic commented Mar 13, 2020

fjeremic commented Mar 20, 2020

fjeremic commented Mar 20, 2020

fjeremic commented Mar 23, 2020

fjeremic commented Mar 25, 2020

fjeremic commented Mar 25, 2020

Leonardo2718 left a comment

Choose a reason for hiding this comment

Leonardo2718 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjeremic commented Mar 30, 2020

Leonardo2718 commented Mar 31, 2020

fjeremic commented Mar 31, 2020

fjeremic commented Mar 31, 2020 • edited Loading

Leonardo2718 left a comment

Choose a reason for hiding this comment

Leonardo2718 commented Mar 31, 2020

Leonardo2718 commented Apr 2, 2020

fjeremic Mar 6, 2020 •

edited

Loading

fjeremic Mar 6, 2020 •

edited

Loading

fjeremic Mar 6, 2020 •

edited

Loading

fjeremic commented Mar 31, 2020 •

edited

Loading