Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate z/OS to a CMake build system #3693

Merged
merged 44 commits into from
Jun 11, 2019

Conversation

fjeremic
Copy link
Contributor

@fjeremic fjeremic commented Mar 25, 2019

This PR adds support for building JitBuilder on z/OS. It has various fixes and some workarounds for issues either related to xlc toolchain not supporting C++11 or z/OS workarounds for unsupported APIs. Note we cannot actually run JitBuilder tests at the moment because the JIT needs to implement proper XPLINK linkage which we have somehow lost in the refactoring process.

This PR simply gets us into a state where we can build JitBuilder. It will enable us to hook up the build process to our PR testing to ensure developers are not checking in C++11 code which does not work on z/OS. In parallel I'll be working on adding proper XPLINK support back so that we can start executing tests as well.

The PR is broken up into many commits, each of which was a hurdle I had to overcome in the development process. Each commit description outlines why the change had to be made. I'll now start off with some self-reviews for major hacks I had to place in which require changes outside of my expertise for which I'll need help with.

Fixes: #3710

@fjeremic
Copy link
Contributor Author

Also requesting a review from @dnakamura.

compiler/env/FEBase_t.hpp Show resolved Hide resolved
compiler/env/FEBase_t.hpp Show resolved Hide resolved
compiler/env/TRMemory.hpp Outdated Show resolved Hide resolved
ddr/lib/ddr-blobgen/java/genBinaryBlob.cpp Outdated Show resolved Hide resolved
util/omrutil/CMakeLists.txt Show resolved Hide resolved
@fjeremic
Copy link
Contributor Author

@genie-omr build all

@fjeremic fjeremic force-pushed the jitbuilder-zos branch 2 times, most recently from 203d723 to 57f6100 Compare March 25, 2019 19:26
@charliegracie
Copy link
Contributor

I would also like to see working zOS builds on the OMR farm before moving too far forward with these changes since I can not verify they work in a normal zOS environment. Also I believe there are 1 or 2 PRs that should be merged before this. I think there is a PR to resolve the return values after TR_UNIMPLEMENTED as a minimum

@fjeremic
Copy link
Contributor Author

I would also like to see working zOS builds on the OMR farm before moving too far forward with these changes since I can not verify they work in a normal zOS environment. Also I believe there are 1 or 2 PRs that should be merged before this. I think there is a PR to resolve the return values after TR_UNIMPLEMENTED as a minimum

I agree on both fronts. I'll work with the owners of the dependent PRs to move them along.

@fjeremic
Copy link
Contributor Author

fjeremic commented Apr 2, 2019

@genie-omr build all

@fjeremic
Copy link
Contributor Author

fjeremic commented Apr 3, 2019

Removed the workaround due to #3956 being merged. I've verified everything works as expected now.

@genie-omr build all

@mstoodle
Copy link
Contributor

mstoodle commented Apr 4, 2019

@fjeremic thanks for all the effort here!

Like @charliegracie, I would like to wait until the z/OS machines are configured so that we can actually activate the compiler/jitbuilder builds as part of merging these changes. As it stands, I believe we've only verified (at this point) that these changes do not affect any of the non-compiler componentry building on z/OS.

I guess that means getting an appropriate version of cmake installed and operational on our z/OS machines so that these changes can be activated to actually build the jitbuilder library?

@fjeremic
Copy link
Contributor Author

fjeremic commented Apr 4, 2019

I guess that means getting an appropriate version of cmake installed and operational on our z/OS machines so that these changes can be activated to actually build the jitbuilder library?

Yes, I agree. We've actually open sourced the z/OS support for CMake 3.5.1 [1]. I'm currently working on bootstrapping cmake on our OMR z/OS build machines. Currently running into some infrastructure issues with /tmp so I'm working with IT to resolve those. I'll update once I have CMake working on the farm.

This PR will receive another commit which enables CMake tests on the build farm.

[1] https://github.com/fjeremic/CMake/

@fjeremic
Copy link
Contributor Author

fjeremic commented Apr 9, 2019

Just a short update on this. I've been working with IT behind the scenes to install various tooling we'll need to get these builds going on the Eclipse OMR z/OS machines hooked up for PR testing. I've now got this PR building completely on the PR machines. The Jenkins user is now setup for CMake builds as well. Next step will be the addition of a new commit which will enable CMake z/OS builds. I'll be working on this in the next day or two.

@fjeremic
Copy link
Contributor Author

fjeremic commented Apr 24, 2019

So I ended up going down a deep rabbit hole to properly implement XPLINK linkage on z/OS so we can actually run the tests that we build here. With the change in #3779 along with the one here we can now from end-to-end build and run all the JitBuilder tests.

Before going back to polish off the system linkage changes further I'll take pause now and get back to working on this PR to hook it up to the Jenkins builds. Once #3779 is merged this PR should follow which will enable the actual testing.

fjeremic and others added 12 commits June 4, 2019 16:08
When building omrsig on z/OS the build process fails because the import
library (libomrsig.x) does not live alongside the shared library
(libomrsig.so). Because of this the make process fails as we cannot
find the import library.

Signed-off-by: Filip Jeremic <[email protected]>
For platforms which require "extra" metadata for exported library
functions (Windows and z/OS) we need to call the helper routine to
generate such metadata so that we are able to load the symbols at
runtime.

Signed-off-by: Filip Jeremic <[email protected]>
Initializer lists of vectors do not work on z/OS due to compiler
restrictions.

Signed-off-by: Filip Jeremic <[email protected]>
Because `GTEST_HAS_DEATH_TEST` is not defined on z/OS neither is the
`ExitedWithCode` API, and hence we cannot make use of it.

Signed-off-by: Filip Jeremic <[email protected]>
This is causing AIX problems:

```
/third_party/gtest-1.8.0/include/gtest/internal/gtest-port.h", line
714.7: 1540-0130 (S) "::std::get" is not declared.
/third_party/gtest-1.8.0/include/gtest/internal/gtest-port.h", line
715.7: 1540-0130 (S) "::std::make_tuple" is not declared.
/third_party/gtest-1.8.0/include/gtest/internal/gtest-port.h", line
716.7: 1540-0130 (S) "::std::tuple" is not declared.
/third_party/gtest-1.8.0/include/gtest/internal/gtest-port.h", line
717.7: 1540-0130 (S) "::std::tuple_element" is not declared.
/third_party/gtest-1.8.0/include/gtest/internal/gtest-port.h", line
718.7: 1540-0130 (S) "::std::tuple_size" is not declared.
```

Signed-off-by: Filip Jeremic <[email protected]>
- Better identify z/OS specific config rather than xlc + zArch
- Allow ability to specify independently control EBCDIC usage in tools
  and runtime components

Signed-off-by: Devin Nakamura <[email protected]>
@fjeremic
Copy link
Contributor Author

fjeremic commented Jun 4, 2019

@genie-omr build all

@Leonardo2718
Copy link
Contributor

@keithc-ca Have your comments been addressed?

I intend to merge this PR today, so if anyone still wants to review this work, please do so ASAP.

@@ -34,3 +34,6 @@ set(OMR_THR_CUSTOM_SPIN_OPTIONS ON CACHE BOOL "")
set(OMR_THR_FORK_SUPPORT ON CACHE BOOL "")
set(OMR_THR_SPIN_WAKE_CONTROL ON CACHE BOOL "")
set(OMR_THR_THREE_TIER_LOCKING ON CACHE BOOL "")

set(OMR_USE_NATIVE_ENCODING ON CACHE BOOL "")
set(OMR_TOOLS_USE_NATIVE_ENCODING ON CACHE BOOL "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The native encoding properties still say ON despite the apparent agreement above.

cmake/modules/platform/os/zos.cmake Show resolved Hide resolved
cmake/modules/platform/toolcfg/xlc.cmake Show resolved Hide resolved
port/unix/omrfile.c Outdated Show resolved Hide resolved
port/unix/omrfiletext.c Outdated Show resolved Hide resolved
port/zos390/omrgetjobname.c Show resolved Hide resolved
port/zos390/omrosdump.c Outdated Show resolved Hide resolved
util/a2e/CMakeLists.txt Outdated Show resolved Hide resolved
@fjeremic
Copy link
Contributor Author

fjeremic commented Jun 7, 2019

@keithc-ca I've addressed all latest reviews.

@genie-omr build all

In addition instead of moving files in the CMake build we copy them
instead.

Signed-off-by: Filip Jeremic <[email protected]>
@Leonardo2718
Copy link
Contributor

@keithc-ca I believe your last concern has been addressed so I will merge this once the CI build completes.

@genie-omr build all

@keithc-ca
Copy link
Contributor

I didn't see any response to my concern about Travis.cmake.

@fjeremic
Copy link
Contributor Author

fjeremic commented Jun 7, 2019

I'm going to start resolving what I believe are addressed reviews on behalf of reviewers as there are so many threads on this PR and it's not easy to see what is active and what is not.

… default

There are a few changes needed to get OMR to be able to build when specifying `OMR_USE_NATIVE_ENCODING=OFF` however this is not the default OMR configuration. By default this variable is set to `ON` so that we use the native encoding of the platform. Projects consuming OMR are able to use non-native encodings by overriding this variable to `OFF`, however the onus is on the downstream projects which do this to correctly make use of the a2e library provided in OMR.

Signed-off-by: Filip Jeremic <[email protected]>
@fjeremic
Copy link
Contributor Author

@genie-omr build all

@Leonardo2718
Copy link
Contributor

The latest changes LGTM.

Given that these changes as a whole enable more extensive testing of OMR components, I think the change in behaviour compared to autotools is acceptable.

Since this PR is holding back other work, I am going to merge it today and will request that further changes and discussions happen in followup issues and PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable the use of -D_ISOC99_SOURCE in all of OMR for z/OS
7 participants