Page MenuHomePhabricator

[SystemZ/ZOS] Additions to the build system.
ClosedPublic

Authored by Kai on Jul 15 2020, 5:10 AM.

Details

Summary

This change extend the CMake files with the necessary additions to build for z/OS.

Diff Detail

Event Timeline

Kai created this revision.Jul 15 2020, 5:10 AM
hubert.reinterpretcast added inline comments.
llvm/CMakeLists.txt
402

Is it actually true that the resulting code is not position-independent? I thought that all XPLINK code is DLL-enabled. The outcome where the resulting code is position-independent (without the need to specify additional options) should be expressed differently from what is done here.

llvm/cmake/modules/AddLLVM.cmake
252

The various "strip" binder options are not the default, so there is a TODO here to enable the removal of the unneeded portions of the input to the linking process (which should help in terms of binary size).

llvm/cmake/modules/GetHostTriple.cmake
17–19

I think CMAKE_SYSTEM_NAME (used above for z/OS) is right: The "host" of the resulting build is the target of this CMake build. @daltenty, if you agree, I think a small patch would make sense (likely to create a merge conflict with this one though)...

llvm/cmake/modules/HandleLLVMOptions.cmake
1040

What is the nature of this limitation? "Not supported" messages tend to confuse, because someone wanting to solve the issue (other than not using whatever is "unsupported") needs some rationale in order to make progress.

Indeed, the text of the message speaks to a limitation to the implementation strategy used by the build and does not directly say much about there being a limitation in achieving the outcome that the option is intended to produce.

daltenty added inline comments.Jul 16 2020, 1:02 PM
llvm/cmake/modules/GetHostTriple.cmake
17–19

I agree that is probably more desirable in the long run, though it does create a behavior difference between platforms which rely on "config.guess" (which won't respect the CMAKE_SYSTEM_NAME and will always use the host) and those that don't (i.e. AIX / z/OS).

Kai added inline comments.Aug 11 2020, 5:22 AM
llvm/CMakeLists.txt
402

You're right. This is not required.

llvm/cmake/modules/HandleLLVMOptions.cmake
1040

I agree that this is text is unclear. I'll deal later with this.

Kai updated this revision to Diff 284674.Aug 11 2020, 5:24 AM
  • Position-independent code needs not to be disabled
  • Removed message stating that static symbol export is not supported
llvm/cmake/modules/AddLLVM.cmake
252

@Kai, can we at least add a TODO comment to set up linker garbage collection for a z/OS toolchain?

llvm/cmake/modules/GetHostTriple.cmake
17–19

@Kai, are you okay with making a drive-by fix for AIX in this patch to keep the usage consistent?

Kai added inline comments.Aug 12 2020, 5:51 AM
llvm/cmake/modules/AddLLVM.cmake
252

For sure.

llvm/cmake/modules/GetHostTriple.cmake
17–19

Yes, no problem. What should I add?

llvm/cmake/modules/GetHostTriple.cmake
17–19

Yes, no problem. What should I add?

llvm/cmake/modules/GetHostTriple.cmake
17–19

I am suggesting that we use CMAKE_SYSTEM_NAME for both the AIX and z/OS queries (i.e., we should be consistent between the two here). The CMAKE_HOST_SYSTEM_NAME does not always reflect the host system for a build using the compiler resulting from the current build.

@daltenty pointed out that there is a logic error for some other platforms (and that using CMAKE_HOST_SYSTEM_NAME is consistent with that logic error). I do not believe that adopting the logic error for AIX and z/OS is necessarily better.

Kai updated this revision to Diff 285609.Aug 14 2020, 2:55 AM
  • Added a TODO comment to the linker strip section
  • Both z/OS and AIX now use CMAKE_SYSTEM_NAME to determine the host system
This revision is now accepted and ready to land.Aug 14 2020, 9:22 AM
This revision was automatically updated to reflect the committed changes.