Unresolved symbols in Windows clang-plugin with updated clang

RESOLVED FIXED in Firefox 64

Status

enhancement
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: dmajor, Assigned: dmajor)

Tracking

unspecified
mozilla64
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Assignee

Description

2 years ago
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"?
Flags: needinfo?(janus926)
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.
Assignee

Comment 2

2 years ago
Yes, I was using LLVM trunk from the first day of 7.0.0.
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.
Assignee

Comment 4

2 years ago
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.
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.
Flags: needinfo?(janus926)
Assignee

Comment 6

Last year
Posted patch clangast (obsolete) — Splinter Review
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.
Assignee: nobody → dmajor
Attachment #8946899 - Flags: review?(ehsan)
Attachment #8946899 - Flags: review?(ehsan) → review+

Comment 7

Last year
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e7c060d72eb
Link clang-plugin against clangASTMatchers.lib to fix build. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/0e7c060d72eb
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Assignee

Comment 9

Last year
(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?
Flags: needinfo?(ehsan)
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.  :-(
Flags: needinfo?(ehsan)
Assignee

Comment 11

Last year
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.

Updated

Last year
Product: Core → Firefox Build System
Assignee

Comment 12

Last year
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
Assignee

Comment 13

Last year
Reopening since my patch didn't actually fix this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Assignee

Updated

Last year
Assignee: dmajor → nobody
Assignee

Updated

Last year
See Also: → 1450699
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.
Blocks: 1462498
Status: REOPENED → NEW
Target Milestone: mozilla60 → ---
Assignee

Comment 16

8 months ago
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.
Assignee: nobody → dmajor
Assignee

Updated

8 months ago
Attachment #8946899 - Attachment is obsolete: true
Assignee

Comment 19

8 months ago
Attachment #9016652 - Flags: review?(core-build-config-reviews)
Assignee

Comment 20

8 months ago
Attachment #9016653 - Flags: review?(core-build-config-reviews)
Assignee

Comment 21

8 months ago
Attachment #9016654 - Flags: review?(core-build-config-reviews)
Assignee

Comment 22

8 months ago
Attachment #9016655 - Flags: review?(core-build-config-reviews)
Attachment #9016652 - Flags: review?(core-build-config-reviews) → review+
Attachment #9016653 - Flags: review?(core-build-config-reviews) → review+
Attachment #9016654 - Flags: review?(core-build-config-reviews) → review+
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.
Attachment #9016655 - Flags: review?(core-build-config-reviews) → review+
Assignee

Updated

8 months ago
Keywords: checkin-needed

Comment 24

8 months ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.