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 ↗

(On Diff #370638)

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 ↗

(On Diff #370640)

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 ↗

(On Diff #370990)

Use RUN: in lit instead of the cmake magic.

llvm/tools/llvm-shlib/typeid.pattern

2 ↗

(On Diff #370990)

I haven't heard of non-unicity.

breaking pointer equality is more common.

8 ↗

(On Diff #370990)

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

[serge-sans-paille]

Moved to lit-based test

[serge-sans-paille]

llvm/tools/llvm-shlib/typeid.pattern

2 ↗

(On Diff #370990)

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 ↗

(On Diff #370990)

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

Don't use # for blank lines.

14

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

[skatkov]

llvm/test/lit.cfg.py

243

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

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

[skatkov]

llvm/test/lit.cfg.py

243

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

[skatkov]

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

15

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

15

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

15

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/lit.cfg.py

config.substitutions.append(('%loadnewpmbye',
                                  .format(config.llvm_shlib_dir,
                                          config.llvm_shlib_ext)))
 
 
 # Static libraries are not built if BUILD_SHARED_LIBS is ON.
 if not config.build_shared_libs and not config.link_llvm_dylib:
     config.available_features.add('static-libs')
 
+if config.link_llvm_dylib:
+    config.available_features.add('llvm-dylib')
+    config.substitutions.append(
+        ('%llvmdylib',
+         '{}/libLLVM-{}{}'.format(config.llvm_shlib_dir,
+                                  config.llvm_dylib_version,
+                                  config.llvm_shlib_ext)))
+
 if config.have_tf_aot:
     config.available_features.add("have_tf_aot")
 
 if config.have_tf_api:
     config.available_features.add("have_tf_api")
 
 def have_cxx_shared_library():
     readobj_exe = lit.util.which('llvm-readobj', config.llvm_tools_dir)

llvm/test/lit.site.cfg.py.in

 config.have_libxar = @LLVM_HAVE_LIBXAR@
 config.have_libxml2 = @LLVM_ENABLE_LIBXML2@
 config.have_dia_sdk = @LLVM_ENABLE_DIA_SDK@
 config.enable_ffi = @LLVM_ENABLE_FFI@
 config.build_examples = @LLVM_BUILD_EXAMPLES@
 config.enable_threads = @LLVM_ENABLE_THREADS@
 config.build_shared_libs = @BUILD_SHARED_LIBS@
 config.link_llvm_dylib = @LLVM_LINK_LLVM_DYLIB@
+config.llvm_dylib_version = "@LLVM_VERSION_MAJOR@@LLVM_VERSION_SUFFIX@"
 config.llvm_host_triple = '@LLVM_HOST_TRIPLE@'
 config.host_arch = "@HOST_ARCH@"
 config.have_opt_viewer_modules = @LLVM_HAVE_OPT_VIEWER_MODULES@
 config.libcxx_used = @LLVM_LIBCXX_USED@
 config.has_plugins = @LLVM_ENABLE_PLUGINS@
 config.linked_bye_extension = @LLVM_BYE_LINK_INTO_TOOLS@
 config.have_tf_aot = @LLVM_HAVE_TF_AOT@
 config.have_tf_api = @LLVM_HAVE_TF_API@

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

+# Make sure there's no llvm::Any::TypeId symbol local to the shared library.  As
+# the adress of llvm::Any::TypeId::Id is used as a global unique identifier for
+# each type, having another visibility defeats the original purpose.
+#
+# This happens when llvm::Any::TypeId is parametrized by a type with hidden
+# visibility.
+
+# RUN: llvm-nm -C %llvmdylib | FileCheck %s
+# RUN: llvm-nm -C %llvmdylib | FileCheck %s --check-prefix=SAFETY-CHECK
+#
+# CHECK-NOT: {{[^V]}} llvm::Any::TypeId
+# SAFETY-CHECK: V llvm::Any::TypeId
+#
+# REQUIRES: x86_64-linux, llvm-dylib