Page MenuHomePhabricator

[AIX][cmake] Adjust management of `-G` for linking
ClosedPublic

Authored by hubert.reinterpretcast on Oct 23 2020, 6:45 AM.

Details

Summary

The change in 0ba98433971f changed the behaviour of the build when using an XL build compiler because -G is not a pure linker option: it also implies -shared. This was accounted for in the base CMake configuration, so an analysis of the change from 0ba98433971f in relation to a build using Clang (where -shared is introduced by CMake) would not identify the issue. This patch resolves this particular issue by adding -shared alongside -Wl,-G.

At the same time, the investigation reveals that several aspects of the various build configurations are not operating in the manner originally intended.

The other issue related to the -G linker option in the build is that the removal of it (to avoid unnecessary use of run-time linking) is not effective for the build using the Clang compiler. This patch addresses this by adjusting the regular expressions used to remove the broadly-applied -G.

Finally, the issue of specifying the export list with -Wl, instead of a compiler option is flagged with a FIXME comment.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 23 2020, 6:45 AM
Herald added a subscriber: mgorny. · View Herald Transcript
hubert.reinterpretcast requested review of this revision.Oct 23 2020, 6:45 AM
amyk accepted this revision.Oct 23 2020, 9:35 AM

Thanks for investigating this Hubert. Overall makes sense and LGTM, as long as there are no other concerns from anyone else.

This revision is now accepted and ready to land.Oct 23 2020, 9:35 AM
daltenty accepted this revision.Oct 23 2020, 10:54 AM

LGTM, thanks!

llvm/CMakeLists.txt
939

An aside: we should consider if this is worth mentioning to upstream CMake.

llvm/cmake/modules/AddLLVM.cmake
94

Just an aside note, very recent version of CMake on AIX now write their own export lists. When this gets fixed, it would also be good to check the interactions with that.

This revision was landed with ongoing or failed builds.Oct 23 2020, 11:32 AM
This revision was automatically updated to reflect the committed changes.
llvm/CMakeLists.txt
939

@daltenty, this is building on your prior work to reduce the run-time linking exposure. It seems upstream CMake does understand the need on AIX for -shared -Wl,-G for non-XL and -G for XL when the intent is to use run-time linking. That is, out-of-the-box, I think CMake works if we were happy with the run-time linking. It seems the ability for a user to communicate the run-time linking intent in terms of CMake may be where there is a gap.

daltenty added inline comments.Oct 23 2020, 12:54 PM
llvm/CMakeLists.txt
939

The runtime linking aspect was fixed in CMake 3.16: https://gitlab.kitware.com/cmake/cmake/-/issues/19163 . After that version -G / runtime linking is dropped from the default options.

I'm more referring to the fact that CMAKE_MODULE_LINKER_FLAGS seems like it should probably contain -shared out-of-the-box

llvm/CMakeLists.txt
939

I'm not sure CMAKE_MODULE_LINKER_FLAGS is or is not the right level in CMake. For the LLVM build, I think putting it in CMAKE_MODULE_LINKER_FLAGS is consistent with a more manually-tuned build (but actually only when using an XL compiler at this time). Since the builds on AIX are still in flux, I left the -shared from https://gitlab.kitware.com/cmake/cmake/-/blob/5988a4deea67ed122b30c2c0437f70e074273789/Modules/Compiler/GNU.cmake#L36 alone for the builds using a Clang build compiler.

For general CMake, I think the lack of -qmkshrobj in a similar position in the base configuration when using an XL build compiler is a good starting point for the discussion.

llvm/cmake/modules/AddLLVM.cmake
94

We need to check both that the LLVM mechanism can drive/override the CMake management of export lists and that the CMake mechanism is respected by the compiler's linker invocation. For GCC on AIX, I think CMake might need to avoid using -shared.