[PATCH] D113610: Only set LLVM_EXTERNAL_VISIBILITY when building libLLVM dylib

Wed Dec 15 13:24:41 PST 2021

benlangmuir added inline comments.

================
Comment at: llvm/include/llvm/Support/Compiler.h:133
+#define LLVM_EXTERNAL_VISIBILITY
+#endif
 #else
----------------
mehdi_amini wrote:
> If I understand correctly, the `-DBUILD_SHARED_LIBS=YES` will use this "else" and not define LLVM_EXTERNAL_VISIBILITY.
> I'm a bit puzzled why is this correct?
> I would expect that `BUILD_SHARED_LIBS` requires at least as much visibility as `LLVM_BUILD_LLVM_DYLIB` (more in some cases).
> 
> That is:  AFAIU we're making the unit of visibility smaller with `BUILD_SHARED_LIBS` (many small shared object) compared to `LLVM_BUILD_LLVM_DYLIB`: if a symbol `foo` from libA is used in libB, it does not need to be externally visible if we link the static libA and libB together in a single dylib, but if libA and libB are each individual dylibs then `libA` needs to make `foo` visible.
`BUILD_SHARED_LIBS=YES` doesn't work with `-fvisibility=hidden` in LLVM, so it seemed misleading to define this macro for that configuration when it wasn't really exercised there.  That being said, I'm not opposed to changing it just for MLIR or other downstreams, so I'll give that a shot.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113610