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

[David Tenty via Phabricator via llvm-commits]

daltenty marked 2 inline comments as done.
daltenty added inline comments.


================
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}")
----------------
hubert.reinterpretcast wrote:
> 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.
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.


================
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:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70972