New linker warnings during a build

With a recent update, I’m getting a bunch of new visibility warnings from the linker like this:

ld: warning: direct access in function 'void mlir::AbstractOperation::insert<circt::comb::ShrSOp>(mlir::Dialect&)' from file 'lib/libCIRCTComb.a(CombDialect.cpp.o)' to global weak symbol 'llvm::detail::UniqueFunctionBase<void, mlir::RewritePatternSet&, mlir::MLIRContext*>::CallbacksHolder<void (*)(mlir::RewritePatternSet&, mlir::MLIRContext*), void (* const)(mlir::RewritePatternSet&, mlir::MLIRContext*), void>::Callbacks' from file '/Users/chrisl/circt/llvm/build/lib/libMLIRStandard.a(Ops.cpp.o)' means the weak symbol cannot be overridden at runtime. This was likely caused by different translation units being compiled with different visibility settings.
ld: warning: direct access in function 'void mlir::AbstractOperation::insert<circt::comb::ShrSOp>(mlir::Dialect&)' from file 'lib/libCIRCTComb.a(CombDialect.cpp.o)' to global weak symbol 'llvm::detail::UniqueFunctionBase<mlir::LogicalResult, mlir::Operation*>::CallbacksHolder<mlir::LogicalResult (*)(mlir::Operation*), mlir::LogicalResult (* const)(mlir::Operation*), void>::Callbacks' from file '/Users/chrisl/circt/llvm/build/lib/libMLIRStandard.a(Ops.cpp.o)' means the weak symbol cannot be overridden at runtime. This was likely caused by different translation units being compiled with different visibility settings.
ld: warning: direct access in function 'void mlir::AbstractOperation::insert<circt::comb::ShrSOp>(mlir::Dialect&)' from file 'lib/libCIRCTComb.a(CombDialect.cpp.o)' to global weak symbol 'llvm::detail::UniqueFunctionBase<void, mlir::Operation*, mlir::OpAsmPrinter&>::CallbacksHolder<void (*)(mlir::Operation*, mlir::OpAsmPrinter&), void (* const)(mlir::Operation*, mlir::OpAsmPrinter&), void>::Callbacks' from file '/Users/chrisl/circt/llvm/build/lib/libMLIRAffine.a(AffineOps.cpp.o)' means the weak symbol cannot be overridden at runtime. This was likely caused by different translation units being compiled with different visibility settings.
ld: warning: direct access in function 'void mlir::AbstractOperation::insert<circt::comb::ShrSOp>(mlir::Dialect&)' from file 'lib/libCIRCTComb.a(CombDialect.cpp.o)' to global weak symbol 'llvm::detail::UniqueFunctionBase<mlir::ParseResult, mlir::OpAsmParser&, mlir::OperationState&>::CallbacksHolder<mlir::ParseResult (*)(mlir::OpAsmParser&, mlir::OperationState&), mlir::ParseResult (* const)(mlir::OpAsmParser&, mlir::OperationState&), void>::Callbacks' from file '/Users/chrisl/circt/llvm/build/lib/libMLIRStandard.a(Ops.cpp.o)' means the weak symbol cannot be overridden at runtime. This was likely caused by different translation units being compiled with different visibility settings.
...

I’m building on a mac (11.4) with the following cmake flags:

LLVM:
cmake -G Ninja ../llvm -DLLVM_ENABLE_PROJECTS="mlir" -DLLVM_TARGETS_TO_BUILD="X86" -DLLVM_ENABLE_ASSERTIONS=ON -DCMAKE_BUILD_TYPE=Release

CIRCT:
cmake -G Ninja .. -DMLIR_DIR=$CIRCT_PATH/llvm/build/lib/cmake/mlir -DLLVM_DIR=$CIRCT_PATH/llvm/build/lib/cmake/llvm -DLLVM_ENABLE_ASSERTIONS=ON -DCMAKE_BUILD_TYPE=Release

Did something recently change in the CIRCT cmake build?

-Chris

I’m pretty sure this is due to a change I added while incorporating some upstream improvements for how the Python bindings are built. This setting was suggested by @stellaraccident after she made the same change in NPComp.

I think setting visibility hidden is preferred when building static libraries, but now that I think about it, I suspect we only want to set that option when doing a unified build of both LLVM and CIRCT. This is speculation, but I am guessing LLVM was built with visibility=default and CIRCT is now building with visibility=hidden, which is producing the warning about “different translation units being compiled with different visibility settings”.

My intent with the recent CMake updates was to cause no changes for the two-phased build where CIRCT depends on LLVM as a library, but obviously this slipped through. I think it should be a simple fix to put back the default and only change the visibility setting when doing a unified build.

I’m hoping this will address it: [CMake] Only tweak linking settings when doing a unified build. by mikeurbach · Pull Request #1493 · llvm/circt · GitHub. I will try to repro this myself to be sure.

I was looking at our CI check on commits to main to see if I could find the warning there, but the warnings didn’t appear. This check does build CIRCT against a pre-built LLVM (i.e. not unified), but in the clang configuration it uses the BUILD_SHARED_LIBS option, which prevents the visibility=hidden setting. In the gcc configuration, I didn’t see this warning. I’ll test it locally to be sure the above PR fixes the issue.

Amazingly fast turn around, your patch does indeed fix it for me, thank you!

-Chris

1 Like

After just updating Edith, I’m noticing these warnings. Edith uses a unified build and it seems the fix was to only do the thing causing this issue during a unified build. Can we fix this for unified builds as well (or at least provide a CMake option to turn this off)?

UPDATE: -DCMAKE_VISIBILITY_INLINES_HIDDEN=OFF does not seem to fix the issue. Perhaps we can gate this not by unified/split build but behind a PYTHON_SPECIFIC_VISIBILITY_SETTINGS flag? This would enable you to use it with a split build as well (if that makes sense)

UPDATE 2: Removing the code from @mikeurbach’s patch does fix the issue

I am inferring you’re doing a unified build with BUILD_SHARED_LIBS=OFF? That will indeed cause CIRCT to set the CMAKE_CXX_VISIBILITY_PRESET=hidden option. Do you want to not set CMAKE_CXX_VISIBILITY_PRESET=hidden, or do you not care? I added this setting because it was generally considered a good thing when building static libs (source: Discord).

It may be the case you need to re-run cmake (and potentially remove CMakeCache.txt). In a unified build, you shouldn’t get this linker error, unless you’re linking already-built MLIR libs with visibility=default to CIRCT libs with visibility=hidden, or some other project in your build is setting visibility to something else.

I think we can also just make this an option you can override, rather than unilaterally just setting it.

This is entirely conjecture, but my guess is that this option is incompatible with at any point in the future being added to a dynamic library (for instance a dynamic MLIRSwift library). I can add a new flag, but I’m not exactly sure what we should call this thing…

How are you building Edith? I’m not really sure what cmake projects are participating in your unified build, but you shouldn’t get linker issues unless something disagrees. You may need to re-run cmake and re-compile. We can also make this setting something you can configure, if visibility=hidden just isn’t compatible with your setup. My intent was to set a good default, but I neglected to expose a way to override it! I sent a patch for that here: [CMake] Allow users to override CMAKE_CXX_VISIBILITY_PRESET. by mikeurbach · Pull Request #1544 · llvm/circt · GitH

About this part: this is just the project-wide default visibility setting, which can be overridden for specific libs. Before changing the default, I updated a couple libs that want different visibility settings: [CMake] Set CXX_VISIBILITY_PRESET to "default" for some libs, NFC. · llvm/circt@e4a94ec · GitHub. I think you should be able to do something similar for whatever library you want to build. For example, the upstream Python depends on a dynamic shared object built with very specific intent: llvm-project/CMakeLists.txt at a614a28772cbd8e0fc3c5fcf836493c2c8bc80da · llvm/llvm-project · GitHub (~incidentally, I think it also builds with visibility=hidden~, scratch that, was looking at how the pybind11 extensions are built).

My problem here is I need to the visibility to be default for all the libs. I’m compiling the libs manually and instead of creating a mondolib, I’m linking each of them individually to a Swift lib. The Swift lib is compiled by SPM, not CMake so it does its own visibility management.

I see, thanks for the context.