ODR vialotion in mlir-cpu-runner + libmlir_async_runtime.so

[banach-space]

Hi,

The following RUN line from async-group.mlir leads to ODR violation:

// RUN: | mlir-cpu-runner  \
// RUN:     -e main -entry-point-result=void -O0 \
// RUN:     -shared-libs=%mlir_c_runner_utils  \
// RUN:     -shared-libs=%mlir_runner_utils  \
// RUN:     -shared-libs=%mlir_async_runtime  \

Specifically, on my development AArch64 machine running Linux (I doubt the platform actually matters), I see the following:

$ nm libmlir_async_runtime.so | grep EnableABIBreakingChecks

000000000038cb80 B _ZN4llvm23EnableABIBreakingChecksE

$ nm mlir-cpu-runner | grep EnableABIBreakingChecks
000000000474364c B _ZN4llvm23EnableABIBreakingChecksE

And indeed, ASan is able to identify the invocation from async-group.mlir as ODR violation, see sanitizer-x86_64-linux-fast.

This patch implements a workaround that helps silence ASan (there's also a bit more discussion there):

• https://reviews.llvm.org/D146935.

However, that's not a proper solution. To me it feels that libmlir_async_runtime.so should be refactored to fix this, but this isn't really my area of expertise and recommendations (as well as volunteers) are welcome :)

CC @lhames @hctim

-Andrzej

[llvmbot]

@llvm/issue-subscribers-mlir

[joker-eph]

I think that this indicates that libLLVMSupport is statically linked in the two libraries.

One way to handle it could be to link this as an internal library (hide all symbols): the libmlir_async_runtime.so should not need to expose more than handful of entry points?

[banach-space]

I think that this indicates that libLLVMSupport is statically linked in the two libraries.

Yeah, that's the case:

cmake -DCMAKE_CXX_FLAGS="-Wl,--trace-symbol=_ZN4llvm23EnableABIBreakingChecksE"
ninja -v  mlir_async_runtime
/usr/bin/clang++-14 -fPIC -Wl,--trace-symbol=_ZN4llvm23EnableABIBreakingChecksE -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -Werror=mismatched-tags -Werror=global-constructors -O3 -DNDEBUG  -Wl,--gdb-index -fuse-ld=lld  -Wl,-z,defs -Wl,-z,nodelete -Wl,--color-diagnostics   -Wl,--gc-sections -shared -Wl,-soname,libmlir_async_runtime.so.17git -o lib/libmlir_async_runtime.so.17git tools/mlir/lib/ExecutionEngine/CMakeFiles/mlir_async_runtime.dir/AsyncRuntime.cpp.o  -Wl,-rpath,"\$ORIGIN/../lib"  lib/libLLVMSupport.a  -lrt  -ldl  -lm  /usr/lib/aarch64-linux-gnu/libz.so  /usr/lib/aarch64-linux-gnu/libzstd.so  /usr/lib/aarch64-linux-gnu/libtinfo.so  lib/libLLVMDemangle.a && :
lib/libLLVMSupport.a(ABIBreak.cpp.o): definition of _ZN4llvm23EnableABIBreakingChecksE

One way to handle it could be to link this as an internal library (hide all symbols): the libmlir_async_runtime.so should not need to expose more than handful of entry points?

So I think that this is already meant to be happening:

llvm-project/mlir/lib/ExecutionEngine/CMakeLists.txt

Line 169 in 89c4acd

set_property(TARGET mlir_async_runtime PROPERTY CXX_VISIBILITY_PRESET hidden)

Just to confirm - this would "hide" the symbols, but there still would be multiple definition of EnableABIBreakingChecks in:

/ RUN: | mlir-cpu-runner  \
// RUN:     -e main -entry-point-result=void -O0 \
// RUN:     -shared-libs=%mlir_c_runner_utils  \
// RUN:     -shared-libs=%mlir_runner_utils  \
// RUN:     -shared-libs=%mlir_async_runtime 

[joker-eph]

Just to confirm - this would "hide" the symbols, but there still would be multiple definition of EnableABIBreakingChecks in:

Yes, libSupport is an implementation detail of mlir_async_runtime I believe, we can hide a private copy in there.

[banach-space]

This took a bit of digging, but does the job:

target_link_options(mlir_async_runtime PRIVATE "-Wl,-exclude-libs,ALL")

I'll update https://reviews.llvm.org/D146935 accordingly. Does this make sense to you?