Add extra check for llvm::Any::TypeId visibility

[serge-sans-paille]

Summary

This check should ensure we don't reproduce the problem fixed by
02df443d2801601a4e42e360e436d97314e9da30

More accurately, it checks every llvm::Any::TypeId symbol in libLLVM-x.so and
make sure they have weak linkage and are not local to the library, which would
lead to duplicate definition if another weak version of the symbol is defined in
another linked library.

It's a follow-up to https://reviews.llvm.org/D108943

[tstellar]

llvm/tools/llvm-shlib/CMakeLists.txt

81

This should use llvm-nm.

Also, can you make it into a lit test like this one:
https://github.com/llvm/llvm-project/tree/main/clang/test/LibClang

[serge-sans-paille]

(reformat)

[MaskRay]

Use llvm-readelf instead. llvm-nm does not check visibility.
Also, weak objects use [vV] instead of [wW].

% llvm-readelf -sC /tmp/out/custom1/lib/libLLVM-14git.so | grep llvm::Any::TypeId
  1774: 000000000079add6     1 OBJECT  WEAK   DEFAULT    10 llvm::Any::TypeId<llvm::Loop const*>::Id@@LLVM_14
  2602: 000000000076dc84     1 OBJECT  WEAK   DEFAULT    10 llvm::Any::TypeId<llvm::MachineFunction const*>::Id@@LLVM_14
...
% llvm-nm -C /tmp/out/custom1/lib/libLLVM-14git.so | grep llvm::Any::TypeId
0000000000cbdb60 V llvm::Any::TypeId<llvm::PreservedAnalyses>::Id
0000000000cbe4f4 V llvm::Any::TypeId<polly::Scop const*>::Id

If you use lit, check (a) WEAK.*DEFAULT.* llvm::Any::TypeId< has matches and (b) HIDDEN.* llvm::Any::TypeId< doesn't have matches.
A test without positive pattern can easily go stale and unnoticed.

llvm/tools/llvm-shlib/CMakeLists.txt

81

The comment should be extended to reference a URL with a very clear problem report.

A reader would be eager to know why

[serge-sans-paille]

Use FileCheck and llvm-nm for the test.
Catch more potential issues.
Be more explicit about the issue in the various comments .

[MaskRay]

llvm/tools/llvm-shlib/CMakeLists.txt

84

Use RUN: in lit instead of the cmake magic.

llvm/tools/llvm-shlib/typeid.pattern

2

I haven't heard of non-unicity.

breaking pointer equality is more common.

8

Make [br] stronger: {{[[^V]]}}

[serge-sans-paille]

Moved to lit-based test

[serge-sans-paille]

llvm/tools/llvm-shlib/typeid.pattern

2

This really sounds the same to me :-)

[serge-sans-paille]

@tstellar does it look better now?

[tstellar]

Yes, the structure of the test looks good.

llvm/tools/llvm-shlib/typeid.pattern

2

fwiw, I don't know what non-unicity means either.

[serge-sans-paille]

Update test comment

[MaskRay]

LGTM.

llvm/test/tools/llvm-shlib/typeids.test

10 ↗

(On Diff #372546)

Don't use # for blank lines.

14 ↗

(On Diff #372546)

It is more common to place REQUIRES: at the top.

[skatkov]

llvm/test/lit.cfg.py

243 ↗

(On Diff #372638)

I wonder whether config.build_shared_libs should be checked also to ensure that shared libraries built?

I've got an error on the added test by this commit due to shared library is not exist.

[skatkov]

llvm/test/lit.cfg.py

243 ↗

(On Diff #372638)

I used -DLLVM_BUILD_LLVM_DYLIB=ON -DLLVM_LINK_LLVM_DYLIB=ON to run cmake.

[skatkov]

llvm/test/lit.cfg.py

243 ↗

(On Diff #372638)

ok, probably it is problem in my config. Looking deeper. Sorry for bothering...

[skatkov]

llvm/test/tools/llvm-shlib/typeids.test

14 ↗

(On Diff #372638)

ok, why do you expect only V here? As I understood the problem is that TypeId should not be local.

For example u is a unique global symbol and it should be accepted as well.

Do I miss anything? why u is worse than v?
Such symbols are not weak linkage but they are not local to the library, so they do not break llvm::Any behavior.
Test should be modified to check explicitly for all kinds of local linkage, instead of requiring one particular variance of global linkage.

[serge-sans-paille]

llvm/test/tools/llvm-shlib/typeids.test

14 ↗

(On Diff #372638)

My understanding is that templated symbol are granted weak linkage as a consequence of ODR. Reading the nm manpage, it indeed seems they could also be u or v. Do you see any other potential flag?

[skatkov]

llvm/test/tools/llvm-shlib/typeids.test

14 ↗

(On Diff #372638)

I guess it is better to check local linkage but do not see others flags so far.
If you'd like to check globals as well u, v and V seems to be enough for now. We can extend this list later.

could you please fix this test?

llvm/test/CMakeLists.txt

 
 add_lit_testsuite(check-llvm "Running the LLVM regression tests"
   ${CMAKE_CURRENT_BINARY_DIR}
   ${exclude_from_check_all}
   DEPENDS ${LLVM_TEST_DEPENDS}
   )
 set_target_properties(check-llvm PROPERTIES FOLDER "Tests")
 
+if(TARGET check-typeid-visibility)
+  add_dependencies(check-llvm check-typeid-visibility)
+endif()
+
+
 add_lit_testsuites(LLVM ${CMAKE_CURRENT_SOURCE_DIR}
   ${exclude_from_check_all}
   DEPENDS ${LLVM_TEST_DEPENDS}
   )
 
 # Setup an alias for 'check-all'.
 add_custom_target(check)
 add_dependencies(check check-all)
 set_target_properties(check PROPERTIES FOLDER "Tests")

llvm/tools/llvm-shlib/CMakeLists.txt

if (APPLE)
     set_property(TARGET LLVM APPEND_STRING PROPERTY
                 LINK_FLAGS
                 " -compatibility_version 1 -current_version ${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}")
   endif()
 
   if(TARGET libLLVMExports)
     add_dependencies(LLVM libLLVMExports)
   endif()
+
+  set(LIB_DIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX})
+  set(LIB_NAME ${LIB_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}LLVM)
+
+  if(UNIX)
+      # This checks unicity of the adress of llvm::Any::TypeId symbols used as unique type identifiers.
+      # See ${CMAKE_CURRENT_SOURCE_DIR}/typeid.pattern for more details.
+      add_custom_target(
+        check-typeid-visibility
+        COMMAND ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/llvm-nm -C ${LIB_NAME}-${LLVM_VERSION_MAJOR}${LLVM_VERSION_SUFFIX}${CMAKE_SHARED_LIBRARY_SUFFIX} | ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/FileCheck ${CMAKE_CURRENT_SOURCE_DIR}/typeid.pattern
+        DEPENDS LLVM llvm-nm FileCheck
+        WORKING_DIRECTORY ${LIB_DIR}
+        COMMENT "Checking llvm::Any::TypeId visibility")
+  endif()
+
 endif()
 
 if(LLVM_BUILD_LLVM_C_DYLIB AND NOT MSVC)
   if(NOT APPLE)
     message(FATAL_ERROR "Generating libLLVM-c is only supported on Darwin")
   endif()
 
   if(NOT LLVM_BUILD_LLVM_DYLIB)
     message(FATAL_ERROR "Generating libLLVM-c requires LLVM_BUILD_LLVM_C_DYLIB on Darwin")
   endif()
 
   # To get the export list for a single llvm library:
   # nm ${LIB_PATH} | awk "/T _LLVM/ { print $3 }" | sort -u | sed -e "s/^_//g" > ${LIB_PATH}.exports
 
   set(LLVM_EXPORTED_SYMBOL_FILE ${CMAKE_BINARY_DIR}/libllvm-c.exports)
 
-  set(LIB_DIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX})
-  set(LIB_NAME ${LIB_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}LLVM)
   set(LIB_PATH ${LIB_NAME}${CMAKE_SHARED_LIBRARY_SUFFIX})
   set(LIB_EXPORTS_PATH ${LIB_NAME}.exports)
   list(APPEND LLVM_DYLIB_REQUIRED_EXPORTS ${LIB_EXPORTS_PATH})
 
   add_custom_command(OUTPUT ${LLVM_EXPORTED_SYMBOL_FILE}
     COMMAND nm ${LIB_PATH} | awk "/T _LLVM/ || /T LLVM/ { print $3 }" | sort -u | sed -e "s/^_//g" > ${LLVM_EXPORTED_SYMBOL_FILE}
     WORKING_DIRECTORY ${LIB_DIR}
     DEPENDS LLVM

llvm/tools/llvm-shlib/typeid.pattern

+# Make sure there's no llvm::Any::TypeId symbol local to the shared library.
+# That would cause non-unicity of the adresses used as unique identifier for
+# each type when another library is linked in with default visibility (weak)
+# for llvm::Any::TypeId.
+# This happens when llvm::Any::TypeId is parametrized by a type with
+# hidden visibility.
+
+# CHECK-NOT: b llvm::Any::TypeId
+# CHECK-NOT: r llvm::Any::TypeId
+# CHECK: V llvm::Any::TypeId