1427808 - Unresolved symbols in Windows clang-plugin with updated clang

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[(Away)]

When building a new Windows clang revision, I get errors like:

22:01:14     INFO -  ..\..\..\clang\bin\lld-link.exe: error: Unified_cpp_build_clang-plugin0.obj: undefined symbol: ?anyOf@ast_matchers@clang@@3U?$VariadicOperatorMatcherFunc@$01$0PPPPPPPP@@internal@12@B
22:01:14     INFO -  ..\..\..\clang\bin\lld-link.exe: error: Unified_cpp_build_clang-plugin0.obj: undefined symbol: ?allOf@ast_matchers@clang@@3U?$VariadicOperatorMatcherFunc@$01$0PPPPPPPP@@internal@12@B
22:01:14     INFO -  ..\..\..\clang\bin\lld-link.exe: error: Unified_cpp_build_clang-plugin0.obj: undefined symbol: ?unless@ast_matchers@clang@@3U?$VariadicOperatorMatcherFunc@$00$00@internal@12@B

I tried adding clangASTMatchers.lib and it works: https://hg.mozilla.org/try/rev/9b534f6d3e930b181342137ba86c1b4b5f870a47

Out of curiosity I also tried removing clang.lib, so that we _only_ use clangASTMatchers.lib, just like Linux and Mac builds. But that breaks on current clang revisions, with errors like:

16:54:56     INFO -  Unified_cpp_build_clang-plugin0.obj : error LNK2019: unresolved external symbol "public: class clang::SourceLocation __thiscall clang::Stmt::getLocStart(void)const " (?getLocStart@Stmt@clang@@QBE?AVSourceLocation@2@XZ) referenced in function "public: virtual void __thiscall ArithmeticArgChecker::check(struct clang::ast_matchers::MatchFinder::MatchResult const &)" (?check@ArithmeticArgChecker@@UAEXABUMatchResult@MatchFinder@ast_matchers@clang@@@Z)
16:54:56     INFO -  Unified_cpp_build_clang-plugin1.obj : error LNK2001: unresolved external symbol "public: class clang::SourceLocation __thiscall clang::Stmt::getLocStart(void)const " (?getLocStart@Stmt@clang@@QBE?AVSourceLocation@2@XZ)

I don't really understand how all this works.

Should we be adding more specific libs (there's a whole bunch of clangXXXXX.lib files) rather than the large clang.lib?

Or should I just land my patch that sets "clang.lib clangASTMatchers.lib"?

Comment 1

[Andi [:andi]]

What version of clang are you building, I'm guessing the a master version. I'm assuming that this happen because on the version that you're using clang stopped bundling ast matchers in clang.lib.

Comment 2

[(Away)]

Yes, I was using LLVM trunk from the first day of 7.0.0.

Comment 3

[Andi [:andi]]

I'm gonna do the same thing tomorrow on my windows vm and see if they sorted out the libraries making it more linux like, I'm afraid if we do this with the current version of clang, we are going to break the linkage since all of the symbols now are built into clang.lib.

Comment 4

[(Away)]

It would be nice if we could have a solution that handles both old and new clang. I'm setting up Windows lld builds with their own build-clang/*.json files, because I'd like to be able to vary my clang revision independently of the static analysis builders. So ideally our tree would handle being built with multiple clang revisions.

AFAICT, "clang.lib clangASTMatchers.lib" builds successfully with both. It feels a little yucky though.

Comment 5

[Ting-Yu Chou [:ting] (away)]

I wonder is this a bug which somehow the symbols aren't exported? If we want to workaround this, we should use "clang.lib clangXXXX.lib" because it's what LLVM_EXPORT_SYMBOLS_FOR_PLUGINS (https://reviews.llvm.org/D18826) intends.

Comment 6

[(Away)]

Attachment: clangast

I still don't understand why this wasn't required before, but this patch seems to work with both our current and latest tip clang.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e7c060d72eb
Link clang-plugin against clangASTMatchers.lib to fix build. r=froydnj

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/0e7c060d72eb

Comment 9

[(Away)]

(In reply to David Major [:dmajor] from comment #4
> AFAICT, "clang.lib clangASTMatchers.lib" builds successfully with both.

I'm not sure where I got that from, but it seems to be wrong.

I did a try push with clang 6.0.0rc2, with the libs as "clang.lib clangASTMatchers.lib" and I got duplicated symbols like:

17:44:25     INFO -  clangASTMatchers.lib(ASTMatchersInternal.cpp.obj) : error LNK2005: "public: static class clang::ast_matchers::internal::DynTypedMatcher __cdecl clang::ast_matchers::internal::DynTypedMatcher::trueMatcher(class clang::ast_type_traits::ASTNodeKind)" (?trueMatcher@DynTypedMatcher@internal@ast_matchers@clang@@SA?AV1234@VASTNodeKind@ast_type_traits@4@@Z) already defined in clang.lib(clang.exe)

So I tried just "clangASTMatchers.lib" on its own, but that's not enough. Now I get unresolved symbols like:

18:27:28     INFO -  Unified_cpp_build_clang-plugin0.obj : error LNK2019: unresolved external symbol "struct clang::ast_matchers::internal::VariadicOperatorMatcherFunc<2,4294967295> const clang::ast_matchers::anyOf" (?anyOf@ast_matchers@clang@@3U?$VariadicOperatorMatcherFunc@$01$0PPPPPPPP@@internal@12@B) referenced in function "public: virtual void __cdecl ArithmeticArgChecker::registerMatchers(class clang::ast_matchers::MatchFinder *)" (?registerMatchers@ArithmeticArgChecker@@UEAAXPEAVMatchFinder@ast_matchers@clang@@@Z)

I don't know what do to here. Ehsan, do you know how these libs are supposed to work?

Comment 10

[(no longer active)]

Ugh.  Sadly there is no supported way for knowing which libraries you are supposed to use for clang (since there is no clang-config equivalent for llvm-config), so you have to investigate yourself.  In the past what I used to do was to see which symbols were missing (e.g., "?anyOf@ast_matchers@clang@@3U?$VariadicOperatorMatcherFunc@$01$0PPPPPPPP@@internal@12@B" in your case) and I would grep in the clang libs directory to see which .lib file provides that symbol (you can use nm to confirm) and would add that to the list of libs we would link against.  It's quite possible that we would need to change the list of .lib files we use depending on the version of clang we link against.  :-(

Comment 11

[(Away)]

Thank you Ehsan.

Your technique pointed out that I was missing "clangASTMatchers.lib" itself, because my earlier try push was a total PEBCAK and didn't do what I thought it did. :-/

Then clangASTMatchers had unresolved externals of its own, so I kept iterating with nm+grep to arrive at:

clangAnalysis.lib clangAST.lib clangASTMatchers.lib clangBasic.lib clangDriver.lib clangEdit.lib clangFrontend.lib clangLex.lib clangParse.lib clangSema.lib clangSerialization.lib LLVMBinaryFormat.lib LLVMBitReader.lib LLVMCore.lib LLVMMC.lib LLVMMCParser.lib LLVMOption.lib LLVMProfileData.lib LLVMSupport.lib version.lib

which finally linked successfully.

(I also tried willy-nilly linking every .lib I could find, and naturally that ran into more duplicate symbol problems, so it looks like trial-and-error is our only option here. :-/)

I'll add these updated LDFLAGS the next time I bump our clang revision.

Comment 12

[(Away)]

Well, that big list of libs in comment 11 links successfully, but the plugin doesn't work. I don't see any output from it, maybe the .dll is not loading right due to dependencies or something.

I've bisected this down to https://github.com/llvm-mirror/clang/commit/0b7fe98286274f0bfbc4f8335208c5e01217d019

Comment 13

[(Away)]

Reopening since my patch didn't actually fix this.

Comment 14

[Andi [:andi]]

(In reply to David Major [:dmajor] from comment #13)
> Reopening since my patch didn't actually fix this.

I can confirm I've encountered the same issue with the official clang 6 release. It may be that the entry point has changed a little bit for the dll.

Comment 15

[Mike Hommey [:glandium]]

Bug 1462498 bounced because of this. clang.lib alone breaks with missing symbols, clangastmatchers.lib alone breaks with missing symbols, and both breaks with duplicate symbols. yay.

Comment 16

[(Away)]

It turns out that the magic phrase is "clangASTMatchers.lib clang.lib", not "clang.lib clangASTMatchers.lib". This doesn't match my understanding of linking on Windows but I'm not going to ask questions.

Applying this on top of bug 1414287, we can get rid of all clang-win*.json other than clang-win64.json \o/ But given that bug's bumpy landing, I'm going to wait for it to settle for a bit.

Comment 17

[Mike Hommey [:glandium]]

\o/

Comment 18

[(not currently active) Ted Mielczarek]

Nice work!

Comment 19

[(Away)]

Attachment: Fix libs for clang-plugin build.

Comment 20

[(Away)]

Attachment: Make static analysis builds use the main win64-clang-cl toolchain.

Comment 21

[(Away)]

Attachment: Static analysis builds no longer need to force the use of link.exe.

Comment 22

[(Away)]

Attachment: Remove the now-unused win64-clang-cl-st-an toolchain.

Comment 23

[Nathan Froyd [:froydnj]]

Comment on attachment 9016655 [details] [diff] [review]
Remove the now-unused win64-clang-cl-st-an toolchain.

Review of attachment 9016655 [details] [diff] [review]:
-----------------------------------------------------------------

So great.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c8e0e2a2d309
Fix libs for clang-plugin build. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/5f551081abcb
Make static analysis builds use the main win64-clang-cl toolchain. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/28f93e22d796
Static analysis builds no longer need to force the use of link.exe. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/5855ac73f9d7
Remove the now-unused win64-clang-cl-st-an toolchain. r=froydnj

Comment 25

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/c8e0e2a2d309
https://hg.mozilla.org/mozilla-central/rev/5f551081abcb
https://hg.mozilla.org/mozilla-central/rev/28f93e22d796
https://hg.mozilla.org/mozilla-central/rev/5855ac73f9d7