This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by serge-sans-paille on Sep 3 2021, 11:56 AM.

Details

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

Diff Detail

Event Timeline

serge-sans-paille requested review of this revision.Sep 3 2021, 11:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2021, 11:56 AM
tstellar added inline comments.Sep 3 2021, 11:58 AM
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

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

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

serge-sans-paille edited the summary of this revision. (Show Details)Sep 6 2021, 10:39 PM
MaskRay added inline comments.Sep 7 2021, 11:48 AM
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]]}}

Moved to lit-based test

serge-sans-paille marked 2 inline comments as done.Sep 8 2021, 2:18 AM
serge-sans-paille added inline comments.
llvm/tools/llvm-shlib/typeid.pattern
2

This really sounds the same to me :-)

@tstellar does it look better now?

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.

Update test comment

MaskRay accepted this revision.Sep 14 2021, 1:47 PM

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.

This revision is now accepted and ready to land.Sep 14 2021, 1:47 PM
This revision was landed with ongoing or failed builds.Sep 14 2021, 11:33 PM
This revision was automatically updated to reflect the committed changes.
skatkov added inline comments.
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 added inline comments.Sep 15 2021, 2:49 AM
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 added inline comments.Sep 15 2021, 3:30 AM
llvm/test/lit.cfg.py
243 ↗(On Diff #372638)

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

skatkov added inline comments.Sep 15 2021, 8:46 AM
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.

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 added inline comments.Sep 15 2021, 8:09 PM
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?