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

Wed Feb 19 07:02:10 PST 2020

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}")
----------------
stevewan wrote:
> daltenty wrote:
> > 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.
> 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.
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. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70972