This is an archive of the discontinued LLVM Phabricator instance.

[AIX] Make sure we use export lists for plugins
ClosedPublic

Authored by daltenty on Dec 3 2019, 10:54 AM.

Details

Summary

Besides just generating and consuming the lists, this includes:

  • Calling nm with the right options in extract_symbols.py. Such as not demangling C++ names, which AIX nm does by default, and accepting both 32/64-bit names.
  • Not having nm sort the list of symbols or we may run in to memory issues on debug builds, as nm calls a 32-bit sort.
  • Defaulting to having LLVM_EXPORT_SYMBOLS_FOR_PLUGINS on for AIX
  • CMake versions prior to 3.16 set the -brtl linker flag globally on AIX. Clear it out early on so we don't run into failures. We will set it as needed.

Diff Detail

Event Timeline

daltenty created this revision.Dec 3 2019, 10:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 3 2019, 10:54 AM
hubert.reinterpretcast added inline comments.
llvm/cmake/modules/HandleLLVMOptions.cmake
990

Does the "fact" to note apply to AIX and MSVC equally?

992

Add: "except when the LLVM libraries are set up for dynamic linking (due to incompatibility) in the initial configuration".

994

I'm not too sure about this. Are differing defaults based on other options passed on the initial configuration a good CMake design?

daltenty planned changes to this revision.Dec 23 2019, 12:44 PM
daltenty updated this revision to Diff 235197.Dec 23 2019, 7:47 PM
daltenty marked 3 inline comments as done.
  • Reword comment
  • Use CMAKE_DEPENDENT_OPTION instead to set LLVM_EXPORT_SYMBOLS_FOR_PLUGINS defaults on AIX
daltenty added inline comments.Dec 23 2019, 7:48 PM
llvm/cmake/modules/HandleLLVMOptions.cmake
994

This is done in some other places in HandleLLVMOptions. That said, I believe using CMakeDependentOption to hide the option on platforms where we set it internally maybe be a better solution.

llvm/CMakeLists.txt
882

The removal of -brtl from CMAKE_EXE_LINKER_FLAGS leads to test failures (with SIGILL) for various loadable-module tests such as llvm/test/Feature/load_module.ll.

llvm/cmake/modules/HandleLLVMOptions.cmake
989–991

It helps me read this easier if "when" is inserted before "using MSVC".

990

If I understand correctly, the "initial configuration" aspect has now been corrected.

Suggestion:
On AIX we don't show this option, and we enable it by default except when the LLVM libraries are set up for dynamic linking (due to incompatibility).

992

With MSVC, note that [ ... ].

995

This seems misnamed, as this should just be the default for AIX.

daltenty planned changes to this revision.Jan 6 2020, 6:24 AM
daltenty marked 5 inline comments as done.Jan 14 2020, 2:21 PM
daltenty updated this revision to Diff 238101.Jan 14 2020, 2:21 PM
  • Leave -G on for modules and brtl for executables that load them
  • Update comment
  • Rename variable
llvm/CMakeLists.txt
878

Do we risk variance in the configuration here if we leave the CMake default -brtl in CMAKE_EXE_LINKER_FLAGS when the version is less than 3.16? If we want that -brtl, then I'm also not sure I understand where we are adding it for CMake >= 3.16.

880

Is this safe? The string might require replacing -Wl,-brtl, with -Wl, because of having additional options passed through to the linker via the same -Wl. This also does not handle -brtl in other positions in such a list. Please consider for all lines.

882

No need to change CMAKE_SHARED_LIBRARY_CREATE_CXX_FLAGS? Also, I'm wondering if it is more appropriate to hook in at CMAKE_<LANG>_CREATE_SHARED_LIBRARY.

882

This would not handle -G in all positions and would not properly handle -G passed via -Wl. Please consider for all lines.

daltenty planned changes to this revision.Jan 20 2020, 1:52 PM
daltenty marked 2 inline comments as done.Jan 30 2020, 7:45 AM
daltenty updated this revision to Diff 241457.Jan 30 2020, 7:47 AM
  • Leave -G on for modules and brtl for executables that load them
  • Use REGEX replace to filter flags
  • Guard flags and make sure to add brtl if needed
daltenty marked 2 inline comments as done.Jan 30 2020, 8:04 AM
daltenty added inline comments.
llvm/CMakeLists.txt
880

I've updated this to use REGEX to make it position independent.Replacing with -Wl, however is also unsafe because this will generate warnings which will fail a Werror build. We know that CMake will not pass multiple combined flags with -brtl itself.

I guess in theory a user could force this somehow, but that would mess up their build in other ways anyhow so I'm inclined to say that's user error.

882

Updated to to be position independent. As above, we know CMake will not pass this via -Wl, so there is no need to address that case here since we are only interested in filtering the CMake defaults.

llvm/CMakeLists.txt
882

I know we've been focused on the XL-based build, but the "GCC" path for CMake did pass -G via -Wl: https://gitlab.kitware.com/cmake/cmake/commit/3fb3157dae5427b6c86949ec1603986c57602658

We might as well fix the issue proactively if there is reason to suspect that we will switch to a different build compiler (such as Clang).

daltenty updated this revision to Diff 241732.Jan 31 2020, 7:46 AM
  • Check for -Wl, form of passing -G
daltenty marked an inline comment as done.Jan 31 2020, 7:46 AM
stevewan added inline comments.Feb 11 2020, 8:45 AM
llvm/CMakeLists.txt
880

Is it better to make the pattern (-Wl,)?-brtl here? So that it handles the case where -brtl doesn't come right after -Wl. Or if having multiple combined flags with -brtl is strictly prohibited, then we should make this more explicit by adding a comment here that says so.

daltenty updated this revision to Diff 245399.Feb 19 2020, 7:01 AM
daltenty marked an inline comment as done.
  • Update comment
llvm/CMakeLists.txt
880

The problem is that pattern may leave stray -Wl, which will cause a warning that will become an error with -Werrror builds. We can try to do some sophisticated argument parsing here, but I'd rather avoid it since it will be brittle and this is not intended to catch user specified options, but rather as a temporary workaround for CMake's own initial configuration. I'll add a comment that indicates such.

stevewan added inline comments.Feb 19 2020, 8:45 AM
llvm/CMakeLists.txt
880

What I meant was, for instance, if we have a string -Wl,-some_args,-brtl, our current matching pattern -Wl,-brtl would not do the trick. If we change the pattern to (-Wl)?,-brtl, however, things would still work as normal.

I agree that we shouldn't go too fancy here, and this follow on comment is mainly for clarification.

llvm/cmake/modules/HandleLLVMOptions.cmake
988

Comment block is in the wrong location now. Leave a comment here about the AIX default, perhaps with a forward reference like "(see below)".

994

The large comment block goes here.

llvm/CMakeLists.txt
878

I see that we're adding -brtl somewhere else now; however, I'm still under the impression that not removing -brtl here from CMAKE_EXE_LINKER_FLAGS would cause variance on something that—according to the comment here—is known to be dependent on the CMake version.

llvm/CMakeLists.txt
883

The following did not result in deleterious effects for my build using CMake 3.13.2:

--- a/llvm/CMakeLists.txt
+++ b/llvm/CMakeLists.txt
@@ -860,6 +860,7 @@ if (UNIX AND ${CMAKE_SYSTEM_NAME} MATCHES "AIX")
   # configuration, it is still possible the user may force it as part of a
   # compound option.
   if(CMAKE_VERSION VERSION_LESS 3.16)
+    string(REGEX REPLACE "(^|[ \t]+)-Wl,-brtl([ \t]+|$)" "" CMAKE_EXE_LINKER_FLAGS  "${CMAKE_EXE_LINKER_FLAGS}")
     string(REGEX REPLACE "(^|[ \t]+)-Wl,-brtl([ \t]+|$)" "" CMAKE_SHARED_LINKER_FLAGS  "${CMAKE_SHARED_LINKER_FLAGS}")
     string(REGEX REPLACE "(^|[ \t]+)-Wl,-brtl([ \t]+|$)" "" CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS}")
     string(REGEX REPLACE "(^|[ \t]+)(-Wl,)?-G([ \t]+|$)" "" CMAKE_SHARED_LIBRARY_CREATE_C_FLAGS
daltenty marked 3 inline comments as done.May 6 2020, 9:00 PM
daltenty updated this revision to Diff 262540.May 6 2020, 9:00 PM
  • Move comment block
  • Remove flags on executable linking as well
This revision is now accepted and ready to land.May 8 2020, 9:06 AM
This revision was automatically updated to reflect the committed changes.