Page MenuHomePhabricator

[AIX] Make sure we use export lists for plugins
Changes PlannedPublic

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.

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
930

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

932

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

934

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
934

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
842

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
929–936

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

930

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).

932

With MSVC, note that [ ... ].

935

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

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

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.

840

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.

842

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.

842

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.Mon, Jan 20, 1:52 PM