This is an archive of the discontinued LLVM Phabricator instance.

[clang][unittests] Fix a clang unittest linking issue
AbandonedPublic

Authored by SixWeining on Nov 23 2021, 11:48 PM.

Details

Summary

Currently the clang/unittests/Basic/CMakeLists.txt links LLVMTestingSupport in an incorrect way that would cause ninja check-clang failing with a linking error when the LLVM_LINK_LLVM_DYLIB variable is set to ON:

$ cmake ../llvm -DLLVM_ENABLE_PROJECTS="clang" \

-DLLVM_TARGETS_TO_BUILD="X86"           \
-DCMAKE_BUILD_TYPE=Release              \
-DLLVM_ENABLE_ASSERTIONS=ON             \
-DLLVM_LINK_LLVM_DYLIB=ON               \
-G Ninja

$ ninja check-clang

...

/usr/bin/ld: tools/clang/unittests/Basic/CMakeFiles/BasicTests.dir/FileManagerTest.cpp.o: in function `(anonymous namespace)::FileManagerTest_getBypassFile_Test::TestBody()':
FileManagerTest.cpp:(.text._ZN12_GLOBAL__N_134FileManagerTest_getBypassFile_Test8TestBodyEv+0x3a3): undefined reference to `llvm::detail::TakeError(llvm::Error)'

This patch changes the linking method of LLVMTestingSupport from clang_target_link_libraries to target_link_libraries just like other tests have done.

Diff Detail

Event Timeline

SixWeining created this revision.Nov 23 2021, 11:48 PM
SixWeining requested review of this revision.Nov 23 2021, 11:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2021, 11:48 PM
SixWeining retitled this revision from [clang][unittests]Fix a clang unittest linking issue to [clang][unittests] Fix a clang unittest linking issue.
SixWeining edited the summary of this revision. (Show Details)

Modify the summary to be more descriptive.

SixWeining edited the summary of this revision. (Show Details)Nov 25 2021, 2:04 AM

diff1 and diff2 are the same, but diff1 build pass while diff2 build fail. So weird. Just reupload the patch to have a try. Sorry for the noise.

dexonsmith accepted this revision.Dec 8 2021, 5:49 PM

LGTM. Sorry for missing this before. Looks to me like the debian build failure above is unrelated.

If you need someone to commit for you, please include the info for GIT_AUTHOR_NAME and GIT_AUTHOR_EMAIL.

This revision is now accepted and ready to land.Dec 8 2021, 5:49 PM

LGTM. Sorry for missing this before. Looks to me like the debian build failure above is unrelated.

If you need someone to commit for you, please include the info for GIT_AUTHOR_NAME and GIT_AUTHOR_EMAIL.

Thanks @dexonsmith . I don't have commit access, could you help to land this patch for me? Please use below info to commit the change.

	email = luweining@loongson.cn
	name = Weining Lu

BTW, according to the policy, a "Differential Revision: <URL>" line should be added to the commit message. But it seems that I'm not allowed to add such line without commit access. Could you help to add it?

Lost track of this again :/. Was just about to commit this for you (finally) but it looks like https://reviews.llvm.org/D115580 has already fixed it.

SixWeining abandoned this revision.Dec 15 2021, 6:22 PM

Abandon because https://reviews.llvm.org/D115580 has already fixed it although later than my fix.

Thanks @dexonsmith