[AIX] Make sure we use export lists for plugins

[daltenty]

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.

[hubert.reinterpretcast]

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]

• Reword comment
• Use CMAKE_DEPENDENT_OPTION instead to set LLVM_EXPORT_SYMBOLS_FOR_PLUGINS defaults on AIX

[daltenty]

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.

[hubert.reinterpretcast]

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]

• Leave -G on for modules and brtl for executables that load them
• Update comment
• Rename variable

[daltenty]

• Rebase

[hubert.reinterpretcast]

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]

• 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]

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.

[hubert.reinterpretcast]

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]

• Check for -Wl, form of passing -G

[stevewan]

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]

• 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]

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.

[hubert.reinterpretcast]

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.

[hubert.reinterpretcast]

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.

[hubert.reinterpretcast]

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]

• Move comment block
• Remove flags on executable linking as well

[hubert.reinterpretcast]

LGTM. Thanks!

llvm/CMakeLists.txt

set(CMAKE_MODULE_LINKER_FLAGS
     "${CMAKE_MODULE_LINKER_FLAGS} -Wl,-lto_library -Wl,${DARWIN_LTO_LIBRARY}")
 endif()
 
 # Build with _XOPEN_SOURCE on AIX, as stray macros in _ALL_SOURCE mode tend to
 # break things. In this case we need to enable the large-file API as well.
 if (UNIX AND ${CMAKE_SYSTEM_NAME} MATCHES "AIX")
           add_definitions("-D_XOPEN_SOURCE=700")
           add_definitions("-D_LARGE_FILE_API")
+
+  # CMake versions less than 3.16 set default linker flags to include -brtl, as
+  # well as setting -G when building libraries, so clear them out. Note we only
+  # try to clear the form that CMake will set as part of its initial
+  # 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
+      "${CMAKE_SHARED_LIBRARY_CREATE_C_FLAGS}")
+    string(REGEX REPLACE "(^|[ \t]+)(-Wl,)?-G([ \t]+|$)" "" CMAKE_SHARED_LIBRARY_CREATE_CXX_FLAGS
+      "${CMAKE_SHARED_LIBRARY_CREATE_CXX_FLAGS}")
+    string(REGEX REPLACE "(^|[ \t]+)(-Wl,)?-G([ \t]+|$)" "" CMAKE_SHARED_LIBRARY_CREATE_ASM_FLAGS
+      "${CMAKE_SHARED_LIBRARY_CREATE_ASM_FLAGS}")
+  endif()
+
+  # Modules should be built with -G, so we can use runtime linking with
+  # plugins.
+  string(APPEND CMAKE_MODULE_LINKER_FLAGS " -G")
+
+  # Also set the correct flags for building shared libraries.
+  string(APPEND CMAKE_SHARED_LINKER_FLAGS " -shared")
 endif()
 
 # Build with _FILE_OFFSET_BITS=64 on Solaris to match g++ >= 9.
 if (UNIX AND ${CMAKE_SYSTEM_NAME} MATCHES "SunOS")
           add_definitions("-D_FILE_OFFSET_BITS=64")
 endif()
 
 # Work around a broken bfd ld behavior. When linking a binary with a

llvm/cmake/modules/AddLLVM.cmake

if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
     add_custom_command(OUTPUT ${native_export_file}
       COMMAND sed -e "s/^/_/" < ${export_file} > ${native_export_file}
       DEPENDS ${export_file}
       VERBATIM
       COMMENT "Creating export file for ${target_name}")
     set_property(TARGET ${target_name} APPEND_STRING PROPERTY
                  LINK_FLAGS " -Wl,-exported_symbols_list,\"${CMAKE_CURRENT_BINARY_DIR}/${native_export_file}\"")
   elseif(${CMAKE_SYSTEM_NAME} MATCHES "AIX")
+    set(native_export_file "${export_file}")
     set_property(TARGET ${target_name} APPEND_STRING PROPERTY
                  LINK_FLAGS " -Wl,-bE:${export_file}")
   elseif(LLVM_HAVE_LINK_VERSION_SCRIPT)
     # Gold and BFD ld require a version script rather than a plain list.
     set(native_export_file "${target_name}.exports")
     # FIXME: Don't write the "local:" line on OpenBSD.
     # in the export file, also add a linker script to version LLVM symbols (form: LLVM_N.M)
     add_custom_command(OUTPUT ${native_export_file}
if(NOT LLVM_NO_DEAD_STRIP)
       endif()
     else() #LLVM_NO_DEAD_STRIP
       if(${CMAKE_SYSTEM_NAME} MATCHES "AIX")
         set_property(TARGET ${target_name} APPEND_STRING PROPERTY
                      LINK_FLAGS " -Wl,-bnogc")
       endif()
     endif()
   endif()
+
+  if(ARG_SUPPORT_PLUGINS AND ${CMAKE_SYSTEM_NAME} MATCHES "AIX")
+    set_property(TARGET ${target_name} APPEND_STRING PROPERTY
+                 LINK_FLAGS " -Wl,-brtl")
+  endif()
 endfunction(add_link_opts)
 
 # Set each output directory according to ${CMAKE_CONFIGURATION_TYPES}.
 # Note: Don't set variables CMAKE_*_OUTPUT_DIRECTORY any more,
 # or a certain builder, for eaxample, msbuild.exe, would be confused.
 function(set_output_directory target)
   cmake_parse_arguments(ARG "" "BINARY_DIR;LIBRARY_DIR" "" ${ARGN})
 

llvm/cmake/modules/HandleLLVMOptions.cmake

 # This CMake module is responsible for interpreting the user defined LLVM_
 # options and executing the appropriate CMake commands to realize the users'
 # selections.
 
 # This is commonly needed so make sure it's defined before we include anything
 # else.
 string(TOUPPER "${CMAKE_BUILD_TYPE}" uppercase_CMAKE_BUILD_TYPE)
 
 include(CheckCompilerVersion)
 include(HandleLLVMStdlib)
 include(CheckCCompilerFlag)
 include(CheckCXXCompilerFlag)
 include(CheckSymbolExists)
+include(CMakeDependentOption)
 
 if(CMAKE_LINKER MATCHES "lld-link" OR (WIN32 AND LLVM_USE_LINKER STREQUAL "lld") OR LLVM_ENABLE_LLD)
   set(LINKER_IS_LLD_LINK TRUE)
 else()
   set(LINKER_IS_LLD_LINK FALSE)
 endif()
 
 set(LLVM_ENABLE_LTO OFF CACHE STRING "Build LLVM with LTO. May be specified as Thin or Full to use a particular kind of LTO")
elseif(uppercase_LLVM_ENABLE_LTO STREQUAL "FULL")
   endif()
 elseif(LLVM_ENABLE_LTO)
   append("-flto" CMAKE_CXX_FLAGS CMAKE_C_FLAGS)
   if(NOT LINKER_IS_LLD_LINK)
     append("-flto" CMAKE_EXE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)
   endif()
 endif()
 
+# Set an AIX default for LLVM_EXPORT_SYMBOLS_FOR_PLUGINS based on whether we are
+# doing dynamic linking (see below).
+set(LLVM_EXPORT_SYMBOLS_FOR_PLUGINS_AIX_default OFF)
+if (NOT (BUILD_SHARED_LIBS OR LLVM_LINK_LLVM_DYLIB))
+  set(LLVM_EXPORT_SYMBOLS_FOR_PLUGINS_AIX_default ON)
+endif()
+
 # This option makes utils/extract_symbols.py be used to determine the list of
-# symbols to export from LLVM tools. This is necessary when using MSVC if you
-# want to allow plugins, though note that the plugin has to explicitly link
-# against (exactly one) tool so we can't unilaterally turn on
+# symbols to export from LLVM tools. This is necessary when on AIX or when using
+# MSVC if you want to allow plugins. 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). With MSVC, note that the plugin has to
+# explicitly link against (exactly one) tool so we can't unilaterally turn on
 # LLVM_ENABLE_PLUGINS when it's enabled.
-option(LLVM_EXPORT_SYMBOLS_FOR_PLUGINS "Export symbols from LLVM tools so that plugins can import them" OFF)
+CMAKE_DEPENDENT_OPTION(LLVM_EXPORT_SYMBOLS_FOR_PLUGINS
+       "Export symbols from LLVM tools so that plugins can import them" OFF
+       "NOT ${CMAKE_SYSTEM_NAME} MATCHES AIX" ${LLVM_EXPORT_SYMBOLS_FOR_PLUGINS_AIX_default})
 if(BUILD_SHARED_LIBS AND LLVM_EXPORT_SYMBOLS_FOR_PLUGINS)
   message(FATAL_ERROR "BUILD_SHARED_LIBS not compatible with LLVM_EXPORT_SYMBOLS_FOR_PLUGINS")
 endif()
 if(LLVM_LINK_LLVM_DYLIB AND LLVM_EXPORT_SYMBOLS_FOR_PLUGINS)
   message(FATAL_ERROR "LLVM_LINK_LLVM_DYLIB not compatible with LLVM_EXPORT_SYMBOLS_FOR_PLUGINS")
 endif()
 
 # By default we should enable LLVM_ENABLE_IDE only for multi-configuration
 # generators. This option disables optional build system features that make IDEs
 # less usable.
 set(LLVM_ENABLE_IDE_default OFF)
 if (CMAKE_CONFIGURATION_TYPES)

llvm/utils/extract_symbols.py

def dumpbin_get_symbols(lib):
     for line in process.stdout:
         # Look for external symbols that are defined in some section
         match = re.match("^.+SECT.+External\s+\|\s+(\S+).*$", line)
         if match:
             yield match.group(1)
     process.wait()
 
 def nm_get_symbols(lib):
+    if sys.platform.startswith('aix'):
+        process = subprocess.Popen(['nm','-P','-Xany','-C','-p',lib], bufsize=1,
+                                   stdout=subprocess.PIPE, stdin=subprocess.PIPE,
+                                   universal_newlines=True)
+    else:
         process = subprocess.Popen(['nm','-P',lib], bufsize=1,
                                    stdout=subprocess.PIPE, stdin=subprocess.PIPE,
                                    universal_newlines=True)
     process.stdin.close()
     for line in process.stdout:
         # Look for external symbols that are defined in some section
         match = re.match("^(\S+)\s+[BDGRSTVW]\s+\S+\s+\S+$", line)
         if match:
             yield match.group(1)
     process.wait()