[PATCH] D70972: [AIX] Make sure we use export lists for plugins

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/CMakeLists.txt:838
+
+  # 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.
----------------
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.


================
Comment at: llvm/CMakeLists.txt:840
+  # well as setting -G when building libraries, so clear them out.
+  string(REPLACE "-Wl,-brtl" "" CMAKE_SHARED_LINKER_FLAGS  "${CMAKE_SHARED_LINKER_FLAGS}")
+  string(REPLACE "-Wl,-brtl" "" CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS}")
----------------
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.


================
Comment at: llvm/CMakeLists.txt:842
+  string(REPLACE "-Wl,-brtl" "" CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS}")
+  string(REPLACE "-G " "" CMAKE_SHARED_LIBRARY_CREATE_C_FLAGS
+    "${CMAKE_SHARED_LIBRARY_CREATE_C_FLAGS}")
----------------
No need to change `CMAKE_SHARED_LIBRARY_CREATE_CXX_FLAGS`? Also, I'm wondering if it is more appropriate to hook in at [[ https://cmake.org/cmake/help/v3.16/variable/CMAKE_LANG_CREATE_SHARED_LIBRARY.html | CMAKE_<LANG>_CREATE_SHARED_LIBRARY ]].


================
Comment at: llvm/CMakeLists.txt:842
+  string(REPLACE "-Wl,-brtl" "" CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS}")
+  string(REPLACE "-G " "" CMAKE_SHARED_LIBRARY_CREATE_C_FLAGS
+    "${CMAKE_SHARED_LIBRARY_CREATE_C_FLAGS}")
----------------
hubert.reinterpretcast wrote:
> No need to change `CMAKE_SHARED_LIBRARY_CREATE_CXX_FLAGS`? Also, I'm wondering if it is more appropriate to hook in at [[ https://cmake.org/cmake/help/v3.16/variable/CMAKE_LANG_CREATE_SHARED_LIBRARY.html | CMAKE_<LANG>_CREATE_SHARED_LIBRARY ]].
This would not handle `-G` in all positions and would not properly handle `-G` passed via `-Wl`. Please consider for all lines.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70972/new/

https://reviews.llvm.org/D70972