Fix DSO missing from command line error when llvm/clang build as shared libs

[xgupta]

When we build LLVM/Clang as a shared library, we need to specify all shared libraries needed by clang-tutor' tools. Without that error like undefined reference to symbol '_ZN5clang12ast_matchers8callExprE' /usr/bin/ld: /home/xgupta/dev/llvm-project-11.0.0/build/lib/libclangASTMatchers.so.11: error adding symbols: DSO missing from command line occured.

[xgupta]

Something is missing, tests on my system are passing successfully but not by upstream GA.

[banach-space]

Hey @xgupta !

Thank you for submitting this. The failing tests are confusing - your change looks rather unrelated!

I've update the GA files and added a manual trigger. So main works perfectly fine, and your branch is failing. That's very odd! I'll try to reproduce it locally, but won't have the time until later today.

-Andrzej

[xgupta]

Hi @banach-space

It seems the second line of these tests is not correct when I run these tools separately I am getting missing compile_commands.json error.

There are two ways to solve it.

1. add -- at the end of command i.e. ./bin/ct-la-commenter ../test/LACBool.cpp --
2. Use -DCMAKE_EXPORT_COMPILE_COMMANDS=ON flag while building clang tool project to generate compilation database.

I tried both ways but it is working and tests are still failing. I hope you know more about them?

ref: https://eli.thegreenplace.net/2014/05/21/compilation-databases-for-clang-based-tools

[banach-space]

Hi @xgupta , thank you for investigating this!

I've applied your changes and used LLVM-11 (and Clang 11) from Ubuntu repositories to build clang-tutor. When I run this test:

bin/ct-code-refactor --class-name=Base --new-name=walk --old-name=run <clang-tutor-dir>/test/CodeRefactor_Class.cpp

I get the following error:

: CommandLine Error: Option 'help-list' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options
Aborted

Could you confirm that that's what you are seeing as well? IIUC (I need to confirm this!), it's caused by the fact that with your updates tools in <clang-tutor-dir>/tools are always linked against multiple libraries in LLVM and Clang. But then these libraries define various options independently from each other. When you link everything together, you get a clash.

I think that that ^^^ can be fixed by making the changes that you propose conditional:

• if LLVM/Clang where build with BUILD_SHARED_LIBS=On, then add the missing shared libs to target_link_libraries
• otherwise, keep the current implementation
  I need to revisit Clang's CMake scripts before I can suggest a specific solution :)

Regarding the compilation database problem - yes, there should be -- at the end of the run lines (good catch, sorry I missed that!). But that won't prevent the tests from passing (i.e. they don't need the compilation database anyway).

Thanks again for looking into this!

[xgupta]

Could you confirm that that's what you are seeing as well?

No, I didn't see any errors. It works perfectly. I download the LLVM source from https://releases.llvm.org/download.html and compile it on Arch Linux.

if LLVM/Clang where build with BUILD_SHARED_LIBS=On, then add the missing shared libs to target_link_libraries

I think we should build clang-tutor considering that LLVM's BUILD_SHARED_LIBS is ON.

I need to revisit Clang's CMake scripts before I can suggest a specific solution :)

Thanks, I will also try if can find a solution.

Regarding the compilation database problem - yes, there should be -- at the end of the run lines (good catch, sorry I missed that!). But that won't prevent the tests from passing (i.e. they don't need the compilation database anyway).

Oh, then we should keep them as it is.

[banach-space]

No, I didn't see any errors. It works perfectly. I download the LLVM source from https://releases.llvm.org/download.html and compile it on Arch Linux.

Ah, so that's very different to what this action does: https://github.com/banach-space/clang-tutor/actions/runs/500436758. The binaries that you compiled on your Arch Linux and the pre-compiled binaries that that Action downloads will be configured differently.

[xgupta]

Yes. The current packages(11.0.1-1) available from Arch Linux, give errors while building clang-tutor so I build them from the source. BTW my CMake command was
cmake -G Ninja ../llvm -DLLVM_ENABLE_PROJECTS="clang" -DLLVM_TARGETS_TO_BUILD="X86" -DCMAKE_BUILD_TYPE=Release -DLLVM_BUILD_TOOLS=ON -DLLVM_USE_LINKER=lld -DBUILD_SHARED_LIBS=ON -DLLVM_OPTIMIZED_TABLEGEN=ON -DCMAKE_C_COMPILER=/usr/local/bin/clang -DCMAKE_CXX_COMPILER=/usr/local/bin/clang++

[xgupta]

@banach-space, I looked at the https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-move/CMakeLists.txt
to fix the issue. Tests are now passing. Please merge the PR if it looks good.

[banach-space]

Thank you for updating this! Unfortunately, with the current change in tools/CMakeLists.txt I get linking errors when using Clang build with BUILD_SHARED_LIBS=ON. That's most likely because LLVM_LINK_COMPONENTS will be ignored unless we use e.g. add_clang_executable instead of add_executable. However, AFAIK, this CMake method not available to out-of-tree projects.

Normally you'd just add include(AddClang) in the top CMake script and that would do the trick. But unfortunately AddClang.cmake is not copied from the source into the build directory. Relevant Phabricator PR.

I need to take a step back and and think whether there's any way around this :) Would you mind checking whether shared library builds work for you? Perhaps I'm doing something wrong.

[xgupta]

Would you mind checking whether shared library builds work for you? Perhaps I'm doing something wrong.

Sorry, I messed with different configurations and branches, With the proposed changes, It is not working on my system also.

Normally you'd just add include(AddClang) in the top CMake script and that would do the trick. But unfortunately AddClang.cmake is not copied from the source into the build directory. Relevant Phabricator PR.

If this is the way then we should wait for PR(Phabricator) to merge into LLVM. And then Ubuntu to update its Clang package.

Thanks for looking into it.

[xgupta]

Thanks, @banach-space for the opening issue. I think we should now close this PR. rational of PR is already on the issue. We will fix the issue once Ubuntu's clang package update next time.