Fix and enable a large subset of tests on the z/OS OMR CI #4733

fjeremic · 2020-01-17T20:28:59Z

In this PR as a sequence of commits we fix a whole host of issues to get most of the tests currently ran by ctest working on z/OS. We can use ctest on z/OS as well, however at the moment the thread extended test is hanging and is preventing us from making the switch. Several other tests have failures which need to be investigated. However we are well on our way.

The following table represents the list of tests currently running on other platforms and the before/after work once this PR is merged.

Test name	Runs before this PR	Runs after this PR	Test status	Reason/Comment
gcexample		✔️	✔️
python "run_tests.py"		✔️	✔️
conditionals		✔️	✔️
isSupportedType		✔️	✔️
iterfib		✔️	✔️
nestedloop		✔️	✔️
pow2		✔️	✔️
simple		✔️	✔️
worklist		✔️	✔️
omralgotest	✔️	✔️	✔️
TestBytes		✔️	✔️
TestIntrusiveList		✔️	✔️
TestTypeTraits		✔️	✔️
omrporttest		❌	❌	#4747 (excluded via ctest)
omrrastest	✔️	✔️	✔️
omrsubscribertest	✔️	✔️	✔️
omrtraceoptiontest	✔️	✔️	✔️
omrsigtest	✔️	✔️	✔️
omrthreadextendedtest		❌	❌	#4746 (excluded via ctest)
omrthreadtest 1		✔️	✔️	Pthread issues?
omrthreadtest 2		✔️	✔️	Pthread issues?
omrthreadtest 3		✔️	✔️	Pthread issues?
omrutiltest	✔️	✔️	✔️
omrvmtest	✔️	✔️	✔️
omrgctest		✔️	✔️
jitbuildertest		✔️	✔️
compilertest		❌	❌	#4719 (excluded via ctest)
triltest		✔️	✔️
comptest		✔️	✔️

~~The end goal will be to get all tests functionally running on z/OS and have 100% parity with other platforms running via ctest.~~ Goal has been achieved. We are currently running all checkmarked tests above via ctest on z/OS!

The recursive case was incorrectly placed under a define when it should be always on by default. We also take this opportunity to migrate some statics which are only used within the function into local statics. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

The cursor currently needs to be specified to `genCallNOPAndDescriptor` to ensure it is generated after the call instruction. Currently because of the way the `TR::S390Imm2Instruction` constructor works it ends up generating the call descriptor at the start of the instruction stream if a `NULL` preceeding instruction is specified. This will be fixed in outside the scope of this change. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

We currently lack full trampoline support in OMR. To get JitBuilder callouts to native functions working we must ensure we can encode the distance from the JIT method body to the native with a `BRASL` instruction which only has a signed 32-bit range. The only way to currently achieve this is to allocate all code cache memory below the bar. Once JitBuilder is hooked up to the port library we should be able to safely use the port library APIs to accomplish this. See eclipse-omr#4719 for more details to references of how this is done in OpenJ9. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

The current Tril lexer transition table assumes ASCII so it only creates 128 entries for the transitions. This will not work on z/OS because the native encoding is EBCDIC, so the table size needs to be bumped up to 256 to handle all EBCDIC characters so we have proper transitions. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

The z/OS system linkage uses the `saveArguments` function to offload arguments to the stack if needed. This function traverses the parameter list looking for arguments which are part of the linkage (linkage arguments) and if needed it stores them to the stack for use in the method. This is currently not done on z/OS for Doubles. What ends up happening is that the linkage register index (lri) is -1 for Double parameters and as such we never store them off to the stack. Subsequently the method body when referencing such parameters tries to load them from the stack and we get back garbage. To fix this we simply enable the logic to mark Double linkage argument registers with proper indices on z/OS. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

On z/OS and Linux on Z we also do not currently support calls, so skip the MinimalTest on those platforms, including other ones which do not support calls. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

The shutdown sequence for the compilation controller was never called. On platforms such as z/OS this is problematic because JitBuilder and Tril initialize and shutdown the JIT many times, each time leaking resources in the compilation controller. One such resource is the thread local TLS `OMR::compilation` variable which is typically accessed through `TR::comp()`. This TLS variable was not being properly freed with every shutdown, eventually resulting in inability to allocate the TLS variable which causes crashes later on. The JIT compiler currently assumes `TR::comp()` is always non-NULL in places where it should be available, thus we add fatal asserts if we are for some reason unable to allocate this variable. These asserts would have made investigating this problem trivial. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

The allocated code cache memory was not being freed up on z/OS causing memory leaks and eventual failure of tests which would not be able to allocate code cache memory. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

For consistency with all other header files in the compiler component. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

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

jitbuilder/control/Jit.cpp

fjeremic · 2020-01-17T21:03:58Z

Suggesting @mstoodle and @Leonardo2718 for reviews.

fjeremic · 2020-01-17T21:10:23Z

@genie-omr build all

This is a port of commit f816690 for z/OS. The same reasoning applies and will hopefully be fixed in eclipse-omr#4719. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

mstoodle

Mostly looks good to me. I opened a couple of issues to account for the things you found.

I am no z platform expert, so I'm hoping @joransiu will look over those linkage parts, in particular.

mstoodle · 2020-01-23T19:36:44Z

buildenv/jenkins/jobs/builds/Build-zos_390-64

@@ -42,7 +42,7 @@ pipeline {
                timestamps {
                    dir('build') {
                        echo "Sanity Test..."
-                        sh'''./omralgotest -avltest:fvtest/algotest/avltest.lst && ./omrvmtest && ./omrutiltest && ./omrsigtest && ./omrrastest && ./omrsubscribertest && ./omrtraceoptiontest'''
+                        sh'''ctest -V'''


not your mistake, but why doesn't this file have a copyright header?

Opened #4753 .

mstoodle · 2020-01-23T19:37:11Z

buildenv/jenkins/jobs/pull-requests/PullRequest-zos_390-64

@@ -28,7 +28,7 @@ pipeline {
                timestamps {
                    dir('build') {
                        echo "Sanity Test..."
-                        sh'''./omralgotest -avltest:fvtest/algotest/avltest.lst && ./omrvmtest && ./omrutiltest && ./omrsigtest && ./omrrastest && ./omrsubscribertest && ./omrtraceoptiontest'''
+                        sh'''ctest -V'''


same problem here: no copyright header

Opened #4753 .

compiler/ilgen/OMRMethodBuilder.hpp

jitbuilder/control/Jit.cpp

jitbuilder/release/cpp/samples/RecursiveFib.cpp

mstoodle

I have no concerns, but want a z expert other than Filip :) to review before it's merged.

fjeremic · 2020-01-23T21:08:58Z

@genie-omr build zos

fjeremic · 2020-01-23T23:15:56Z

fvtest/jitbuildertest/SystemLinkageTest.cpp

+#if defined(PPC)
+   // TODO (#4765): Fix the Power linkage and enable this test
+   // TODO (#4764): This is a poor man's SKIP_IF which is only available for Tril tests currently. We should find a
+   // nice home for this useful utility so we can use it in JitBuilder tests as well.
+   const ::testing::TestInfo* const test_info = ::testing::UnitTest::GetInstance()->current_test_info();
+   ::testing::Test::RecordProperty("skipped", "Known Bug");
+   std::cout << "Known Bug" << ": Skipping test: " << test_info->name() << "\n    " << "Power system linkage cannot handle stack arguments (see issue #4765)" << "\n";
+   SUCCEED() << "Power system linkage cannot handle stack arguments (see issue #4765)";
+#endif


This is another hack we are adding in, but I don't think this PR is the correct place to solve this. I've opened up two new issues to address the hack added here.

fjeremic · 2020-01-23T23:30:04Z

This issue seems to be turning into a goldmine of spawning other issues. To avoid scope creep I will stop here as the number of fixes is getting large now. The task at hand has been achieved. We have parity now on z/OS and are running almost all the tests via ctest :yay:

One final whirl:

@genie-omr build all

joransiu · 2020-01-24T17:57:38Z

compiler/z/codegen/SystemLinkagezOS.cpp

+               lri = numVRFArgs;
+               }
+
+            numVRFArgs++;


Reading the Vendor Interfaces, I think the same change for FPRs above to bump numGPRArgs should be applicable for VRs as well. Though it's not clear to me from the docs whether it would be numGPRArgs += 2 or 4.... as their prototype f29 suggests GPR would only hold 4 bytes worth of data for VRs.

I couldn't find an updated reference for XPLINK with vector registers. Do they even participate in linkage conventions? How does one pass a vector register argument? If you have any links that would be appreciated. I'll make the change though.

I was referencing: https://www-01.ibm.com/servers/resourcelink/svc00100.nsf/pages/zOSV2R3SA380688/$file/ceev100_v2r3.pdf around page 143/144, and the examples around page 911.

Fixed in a03a579

Ever consider the alignment requirement of passing Vector arguments, i.e. leaving more holes in GPR side? or, is there alignment requirement at all? I remembered I did all these on AIX side before, need to read the code in details to see why it failed now.

joransiu

The changes look good to me. Thanks for addressing all these issues @fjeremic. Just have one comment about bumping of GPRs for vector parameters as well to handle the following from the Vendor Interfaces doc.

Up to eight vector arguments are passed in VR24-31, and not passed in the argument area, although space is set aside for these arguments in the argument area."

Implement XPLINK linkage register to parameter mapping as described in the XPLINK documentation. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

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

On Linux on Z for example we can fit 4 integral and 4 floating point arguments into registers for linkage. This means that no arguments are ever passed through the stack. We want to test this path (it actually has a bug which is fixed in the next commit) so we double the number of arguments to the test to ensure some arguments are passed on the stack. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

Because the Z architecture is big-endian we need to spill the entire GPR contents for incoming arguments in the register to the stack, since our stack slots on 64-bit are 64-bit wide. The parameters used within the method are then properly initialized with offsets into the stack slot. For example for a 4-byte parameter, we will spill all 8 bytes on entry if needed and initialize the offset of the parameter with a +4 increment, so that the trees which end up using the parameter within the method will only access the clean bits of the argument. This also works for sub-32-bit types as well which more or less fixes the issues noted in eclipse-omr#3525. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

See eclipse-omr#4746. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

See eclipse-omr#4747. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

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

Currently due to a limitation of the z/OS CMake port we require to specify an explicit `LIBPATH` so that the various tests which dynamically load DLLs can find them. Once CMake is fixed to properly generate the dynamic libraries in the correct directories setting of the `LIBPATH` environment variable should be removed. See eclipse-omr#4761. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

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

fjeremic · 2020-01-24T18:21:43Z

@mstoodle do you want to be a committer for this one?

@genie-omr build all

mstoodle · 2020-01-24T19:17:41Z

Nice to see all those tests running and passing on z/OS ! Even some JitBuilder tests are now enabled and passing! Woohoo!

Once the appveyor tests run and pass, just for pedantry, I'll merge :) .

mstoodle · 2020-01-24T19:44:05Z

Thanks @fjeremic!

Now that eclipse-omr#4733 has been merged we have fixed the issue which was preventing us from running various sub-int32 tests because the incoming arguments were not being loaded correctly. We can now re-enable all the sub-int32 tests which were disabled as a result. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

fjeremic added 10 commits January 15, 2020 11:31

Skip MinimalTest on non-x86 platforms because of eclipse-omr#1645

19c0bd9

On z/OS and Linux on Z we also do not currently support calls, so skip the MinimalTest on those platforms, including other ones which do not support calls. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

Fix code cache cleanup in JitBuilder and TestCompiler on z/OS

4a3841c

The allocated code cache memory was not being freed up on z/OS causing memory leaks and eventual failure of tests which would not be able to allocate code cache memory. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

Move ThreadLocal.h to ThreadLocal.hpp

6254aed

For consistency with all other header files in the compiler component. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

Run the largest passing subset of tests on z/OS

4d51b47

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

fjeremic added comp:compiler comp:jitbuilder comp:tril ci os:zos arch:z labels Jan 17, 2020

fjeremic requested review from 0xdaryl, charliegracie, Leonardo2718 and mstoodle as code owners January 17, 2020 20:28

fjeremic commented Jan 17, 2020

View reviewed changes

jitbuilder/control/Jit.cpp Show resolved Hide resolved

fjeremic changed the title ~~Fix and enable a large subset of tests on the OMR CI~~ Fix and enable a large subset of tests on the z/OS OMR CI Jan 17, 2020

Disable Tril's IlGenTest on z/OS

7a3c45e

This is a port of commit f816690 for z/OS. The same reasoning applies and will hopefully be fixed in eclipse-omr#4719. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

fjeremic mentioned this pull request Jan 20, 2020

Add support for JitBuilder callouts to private and native functions on platforms with function descriptors #4719

Open

fjeremic requested review from rwy7 and youngar as code owners January 20, 2020 19:03

rwy7 approved these changes Jan 20, 2020

View reviewed changes

fjeremic force-pushed the jitbuilder-call branch from f7ec559 to da18f21 Compare January 22, 2020 18:14

mstoodle requested changes Jan 23, 2020

View reviewed changes

mstoodle approved these changes Jan 23, 2020

View reviewed changes

This was referenced Jan 23, 2020

SKIP_IF not available to JitBuilder tests #4764

Open

Power system linkage cannot handle stack arguments #4765

Closed

fjeremic commented Jan 23, 2020

View reviewed changes

fjeremic mentioned this pull request Jan 23, 2020

Power and Z code generators incorrectly spills sub-32bit integer arguments on big-endian platforms #3525

Closed

fjeremic force-pushed the jitbuilder-call branch from 96bbaf9 to 119ecc1 Compare January 23, 2020 23:29

joransiu reviewed Jan 24, 2020

View reviewed changes

joransiu approved these changes Jan 24, 2020

View reviewed changes

fjeremic added 9 commits January 24, 2020 13:20

Implement setParameterLinkageRegisterIndex on z/OS XPLINK

a03a579

Implement XPLINK linkage register to parameter mapping as described in the XPLINK documentation. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

Implement setParameterLinkageRegisterIndex on Linux

93ae438

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

Disable ThreadExtendedTest on z/OS

175dcf4

See eclipse-omr#4746. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

Disable porttest on z/OS

bf1e56d

See eclipse-omr#4747. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>

Enable testing via ctest on z/OS

06a3681

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

Skip SystemLinkageTest on Power because of eclipse-omr#4765

830e76d

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

fjeremic force-pushed the jitbuilder-call branch from 119ecc1 to 830e76d Compare January 24, 2020 18:21

mstoodle self-assigned this Jan 24, 2020

mstoodle merged commit 9d16f0d into eclipse-omr:master Jan 24, 2020

fjeremic mentioned this pull request Jan 24, 2020

JitBuilder samples should return a non-zero return code on incorrect results #4775

Open

fjeremic mentioned this pull request Feb 6, 2020

Fix and enable sub-int32 tests on Z #4811

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and enable a large subset of tests on the z/OS OMR CI #4733

Fix and enable a large subset of tests on the z/OS OMR CI #4733

fjeremic commented Jan 17, 2020 •

edited

Loading

fjeremic commented Jan 17, 2020

fjeremic commented Jan 17, 2020

mstoodle left a comment

mstoodle Jan 23, 2020

mstoodle Jan 23, 2020

mstoodle Jan 23, 2020

mstoodle Jan 23, 2020

mstoodle left a comment

fjeremic commented Jan 23, 2020

fjeremic Jan 23, 2020

fjeremic commented Jan 23, 2020

joransiu Jan 24, 2020 •

edited

Loading

fjeremic Jan 24, 2020

joransiu Jan 24, 2020

fjeremic Jan 24, 2020

zl-wang Jan 27, 2020

joransiu left a comment

fjeremic commented Jan 24, 2020

mstoodle commented Jan 24, 2020

mstoodle commented Jan 24, 2020

Fix and enable a large subset of tests on the z/OS OMR CI #4733

Fix and enable a large subset of tests on the z/OS OMR CI #4733

Conversation

fjeremic commented Jan 17, 2020 • edited Loading

fjeremic commented Jan 17, 2020

fjeremic commented Jan 17, 2020

mstoodle 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

mstoodle left a comment

Choose a reason for hiding this comment

fjeremic commented Jan 23, 2020

Choose a reason for hiding this comment

fjeremic commented Jan 23, 2020

joransiu Jan 24, 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

joransiu left a comment

Choose a reason for hiding this comment

fjeremic commented Jan 24, 2020

mstoodle commented Jan 24, 2020

mstoodle commented Jan 24, 2020

fjeremic commented Jan 17, 2020 •

edited

Loading

joransiu Jan 24, 2020 •

edited

Loading