Page MenuHomePhabricator

[clangd] Fix shared-lib builds
ClosedPublic

Authored by kadircet on Fri, Nov 20, 4:03 AM.

Diff Detail

Event Timeline

kadircet created this revision.Fri, Nov 20, 4:03 AM
kadircet requested review of this revision.Fri, Nov 20, 4:03 AM
kbobyrev requested changes to this revision.Fri, Nov 20, 4:56 AM

Thank you for taking care of thiis!

The complete fix, however, should include two more things:

  1. Moving this
if (CLANGD_ENABLE_REMOTE)
  include(FindGRPC)
endif()

before that

if(CLANG_INCLUDE_TESTS)
add_subdirectory(test)
add_subdirectory(unittests)
endif()

Otherwise by the time unittests are compiled the include paths are not set appropriately and we will see the errors with Protobuf version mismatch through transitive includes etc.

  1. ClangdTests should be linked against RemoteIndexProto, I didn't notice this problem before since linking clangdRemoteMarshalling already did the trick for static index builds but now we need to do that explicitly in clangd/unittests/CMakeLists.txt.
This revision now requires changes to proceed.Fri, Nov 20, 4:56 AM

Actually, it would be safer if FindGRPC was also higher than add_subdirectory(tool): right now it only includes remote/index/Client.h and Client.h does not depend on any gRPC/Protobuf headers for now but that might change at some point and break builds unexpectedly. I would suggest just moving it as high as possible, there is no harm in that apart from decoupling FindGRPC from add_subdirectory(index/remote) and related code but we kind of have to do that anyway.

I'm not totally following the discussion on the more complete fix (LMK if you want me to dig into that), but if the build is broken in some configurations we should likely prioritize fixing that over ensuring the fix is conceptually complete.

Otherwise by the time unittests are compiled the include paths are not set appropriately and we will see the errors with Protobuf version mismatch through transitive includes etc.

I don't think that's how CMake works, the whole CMakeLists tree is parsed before anything is compiled, so it shouldn't race like that.

clang-tools-extra/clangd/index/remote/CMakeLists.txt
6

oh, this is sad, because we specify the proto dependencies to generate_protos but it doesn't actually set up all the dep relationships we need (and can't because we specify the filename, not the rule).

Can you add a "FIXME: move this into generate_protos somehow"?

I'm not totally following the discussion on the more complete fix (LMK if you want me to dig into that), but if the build is broken in some configurations we should likely prioritize fixing that over ensuring the fix is conceptually complete.

Yes, this is basically an expanded context from a discussion me and Kadir had over the last couple of days. Sorry for confusion, the "complete" means "it's still not working for me without these changes". And the configuration we're looking at is simply shared libs build.

Otherwise by the time unittests are compiled the include paths are not set appropriately and we will see the errors with Protobuf version mismatch through transitive includes etc.

I don't think that's how CMake works, the whole CMakeLists tree is parsed before anything is compiled, so it shouldn't race like that.

This may look counter-intuitive but this is how it works on my machine. MarshallingTests.cpp entry in compile_commands.json does not have gRPC include path. (rg "MarshallingTests.cpp" compile_commands.json | rg grpc is empty). When I put FindGRPC before add_directory(unittests) the compile commands have the right include.

(also please add "Fixes https://github.com/clangd/clangd/issues/598" to the patch description to close the related issue on GH)

kadircet updated this revision to Diff 306830.Sat, Nov 21, 1:39 AM
kadircet marked an inline comment as done.
  • Address comments

I don't think that's how CMake works, the whole CMakeLists tree is parsed before anything is compiled, so it shouldn't race like that.

That was also my mental model, but it looks like include_directories statement within the FindGRPC submodule only affects targets created afterwards (lexicographically). Even though cmake docs say that it should affect everything within that cmakelists file (https://cmake.org/cmake/help/latest/command/include_directories.html).

I've tried to migrate into target_include_directories instead(i.e. only introduce the include header to targets generated via generate_protos and propagate into their dependents), and it works great with targets introduced via add_clang_executable or add_unittest, but for some reason I could not figure out; propagation doesn't work for targets introduced via add_clang_library :(.

And indeed my compile commands also didn't contain the necessary -isystem, but I had an acceptable version of protobuf installed in /usr/include even though i tried really hard to delete all the remaining packages, so it was working on my machine :/

Hopefully it should be good now. It is quite nice that @nridge has also noticed this while we are working on a fix, so I hope he can try this patch on his setup as well :)

I applied the patch locally and it fixes most of the linker errors but I'm still seeing one:

[6/393] Linking CXX executable bin/clangd-index-server
FAILED: bin/clangd-index-server
: && /usr/bin/clang++-10 -fPIC -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 -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -DNDEBUG -fuse-ld=gold     -Wl,-O3 -Wl,--gc-sections tools/clang/tools/extra/clangd/index/remote/server/CMakeFiles/clangd-index-server.dir/Server.cpp.o -o bin/clangd-index-server  -Wl,-rpath,"\$ORIGIN/../lib"  -lpthread  lib/libRemoteIndexServiceProto.so.12git  lib/libclangdRemoteMarshalling.so.12git  -lgrpc++  lib/libclangDaemon.so.12git  lib/libclangdSupport.so.12git  lib/libRemoteIndexProto.so.12git  lib/libLLVMSupport.so.12git  -Wl,-rpath-link,llvm/prod-build/lib && :
tools/clang/tools/extra/clangd/index/remote/server/CMakeFiles/clangd-index-server.dir/Server.cpp.o:Server.cpp:function llvm::detail::stream_operator_format_adapter<clang::clangd::remote::(anonymous namespace)::RemoteIndexServer::TextProto>::format(llvm::raw_ostream&, llvm::StringRef): error: undefined reference to 'google::protobuf::Message::DebugString[abi:cxx11]() const'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
kbobyrev accepted this revision.Sun, Nov 22, 5:00 AM

Ah, right. I think clangd-remote-index needs protobuf dependency. @nridge, I believe this should fix the problem you're seeing. I'm somewhat confused as to why I'm not seeing the same problem though but it makes total sense to me.

(also please add "Fixes https://github.com/clangd/clangd/issues/598" to the patch description to close the related issue on GH)

Otherwise LGTM + patch description :)

This revision is now accepted and ready to land.Sun, Nov 22, 5:00 AM

I applied the patch locally and it fixes most of the linker errors but I'm still seeing one:

[6/393] Linking CXX executable bin/clangd-index-server
FAILED: bin/clangd-index-server
: && /usr/bin/clang++-10 -fPIC -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 -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -DNDEBUG -fuse-ld=gold     -Wl,-O3 -Wl,--gc-sections tools/clang/tools/extra/clangd/index/remote/server/CMakeFiles/clangd-index-server.dir/Server.cpp.o -o bin/clangd-index-server  -Wl,-rpath,"\$ORIGIN/../lib"  -lpthread  lib/libRemoteIndexServiceProto.so.12git  lib/libclangdRemoteMarshalling.so.12git  -lgrpc++  lib/libclangDaemon.so.12git  lib/libclangdSupport.so.12git  lib/libRemoteIndexProto.so.12git  lib/libLLVMSupport.so.12git  -Wl,-rpath-link,llvm/prod-build/lib && :
tools/clang/tools/extra/clangd/index/remote/server/CMakeFiles/clangd-index-server.dir/Server.cpp.o:Server.cpp:function llvm::detail::stream_operator_format_adapter<clang::clangd::remote::(anonymous namespace)::RemoteIndexServer::TextProto>::format(llvm::raw_ostream&, llvm::StringRef): error: undefined reference to 'google::protobuf::Message::DebugString[abi:cxx11]() const'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

I am not able to reproduce the failure. Maybe we should just make the dependencies introduced in generate_protos for grpc++ and protobuf PUBLIC, to ensure they propagate into users (i believe that's the default for my configuration for whatever reason). Can you share your cmake configuration ?

I applied the patch locally and it fixes most of the linker errors but I'm still seeing one:

[6/393] Linking CXX executable bin/clangd-index-server
FAILED: bin/clangd-index-server
: && /usr/bin/clang++-10 -fPIC -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 -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -DNDEBUG -fuse-ld=gold     -Wl,-O3 -Wl,--gc-sections tools/clang/tools/extra/clangd/index/remote/server/CMakeFiles/clangd-index-server.dir/Server.cpp.o -o bin/clangd-index-server  -Wl,-rpath,"\$ORIGIN/../lib"  -lpthread  lib/libRemoteIndexServiceProto.so.12git  lib/libclangdRemoteMarshalling.so.12git  -lgrpc++  lib/libclangDaemon.so.12git  lib/libclangdSupport.so.12git  lib/libRemoteIndexProto.so.12git  lib/libLLVMSupport.so.12git  -Wl,-rpath-link,llvm/prod-build/lib && :
tools/clang/tools/extra/clangd/index/remote/server/CMakeFiles/clangd-index-server.dir/Server.cpp.o:Server.cpp:function llvm::detail::stream_operator_format_adapter<clang::clangd::remote::(anonymous namespace)::RemoteIndexServer::TextProto>::format(llvm::raw_ostream&, llvm::StringRef): error: undefined reference to 'google::protobuf::Message::DebugString[abi:cxx11]() const'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

I am not able to reproduce the failure. Maybe we should just make the dependencies introduced in generate_protos for grpc++ and protobuf PUBLIC, to ensure they propagate into users (i believe that's the default for my configuration for whatever reason). Can you share your cmake configuration ?

I agree that would be a good thing but since Server.cpp explicitly uses DebugString I think explicitly specifying protobuf dependency there is still a way to go.

I applied the patch locally and it fixes most of the linker errors but I'm still seeing one:

[...]

I am not able to reproduce the failure. Maybe we should just make the dependencies introduced in generate_protos for grpc++ and protobuf PUBLIC, to ensure they propagate into users (i believe that's the default for my configuration for whatever reason). Can you share your cmake configuration ?

The cmake command I'm using for this build building is something like:

cmake -G Ninja <sourcedir> -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" -DLLVM_USE_LINKER=gold -DBUILD_SHARED_LIBS=ON \
            -DLLVM_OPTIMIZED_TABLEGEN=ON -DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_CCACHE_BUILD=ON -DCMAKE_BUILD_TYPE=Release \
            -DCLANGD_ENABLE_REMOTE=ON \
            -DLLVM_APPEND_VC_REV=OFF

Ah, right. I think clangd-remote-index needs protobuf dependency. @nridge, I believe this should fix the problem you're seeing.

Not sure if this is what you meant, but I fixed it by adding a protobuf dependency to clangd-index-server in clang-tools-extra/clangd/index/remote/server/CMakeLists.txt.

After fixing that (and one more shared-libs dependency issue unrelated to clangd), my build completed successfully.

Ah, right. I think clangd-remote-index needs protobuf dependency. @nridge, I believe this should fix the problem you're seeing.

Not sure if this is what you meant, but I fixed it by adding a protobuf dependency to clangd-index-server in clang-tools-extra/clangd/index/remote/server/CMakeLists.txt.

After fixing that (and one more shared-libs dependency issue unrelated to clangd), my build completed successfully.

Uh, yeah, sorry, I meant clangd-index-server, just used the wrong name for some reason. Thanks for checking!

kadircet updated this revision to Diff 307281.Tue, Nov 24, 1:42 AM
  • Link protobuf and grpc++ publicly to generated targets
Herald added a project: Restricted Project. · View Herald TranscriptTue, Nov 24, 1:42 AM
This revision was automatically updated to reflect the committed changes.