[PATCH] D54439: CMake: Make most target symbols hidden by default

Tue Jun 18 11:53:26 PDT 2019

rnk added a subscriber: echristo.
rnk added inline comments.

================
Comment at: llvm/include/llvm/PassSupport.h:62-63
   static llvm::once_flag Initialize##passName##PassFlag;                       \
+  LLVM_EXTERNAL_VISIBILITY /* FIXME: Workaround to avoid ld warnings on OS X*/ \
+			   /* when this is compiled with -fvisibility=hidden*/ \
   void llvm::initialize##passName##Pass(PassRegistry &Registry) {              \
----------------
tstellar wrote:
> rnk wrote:
> > Is this a speculative fix, or were you able to reproduce the problem and confirm this fixes it?
> This is speculative.   I don't have an OS X system to test on.  I have tried to reproduce on linux with clang-7, but I don't see any object file differences in files mentioned in the warnings, like X86Targetmachine.cpp.o.
> 
> It seems like this warning is triggered when you have a cpp file that uses one of the INITIALIZE_PASS macros and also calls the initialize*Pass function in the same file.   My best guess is this is related to inlining decisions in some way, but I still don't know the root cause.
@echristo, do you want to apply this locally and try to reproduce, since you reported the issue last time around?

@tstellar My worry is that maybe this warning is triggered more broadly by any templates instantiated and called by hidden and default visibility functions. I'd like to nail down when it happens, or disable it entirely, since I'm positive we don't care about being able to override C++ template functions.

================
Comment at: llvm/lib/Target/CMakeLists.txt:21
+  # in libLLVM.so.
+  set(CMAKE_CXX_VISIBILITY_PRESET hidden)
+endif()
----------------
If the warning does re-occur, I'd suggest working around it by disabling this logic here on Mac. Then we can fix it with smaller patches that aren't too large for phab to display by default.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54439