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

Wed Feb 19 08:45:25 PST 2020

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70972