Fails linking clang-plugin with unresolved clang symbols

RESOLVED FIXED in Firefox 53

Status

RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: ting, Assigned: ting)

Tracking

Trunk
mozilla53
Unspecified
Windows
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(URL)

Attachments

(8 attachments, 7 obsolete attachments)

58 bytes, text/x-review-board-request
Nika
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Ehsan
: review+
glandium
: review+
Details
58 bytes, text/x-review-board-request
Ehsan
: review+
Details
1.97 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
2.18 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
https://treeherder.mozilla.org/logviewer.html#?job_id=30807738&repo=try#L4526-L4717

I failed also local build with mozconfig:

  export CC=clang-cl
  export CXX=clang-cl
  mk_add_options "export INCLUDE=$INCLUDE;C:\\w\\tools\\llvm\\src\\tools\\clang\\include;C:\\w\\tools\\llvm\\obj\\tools\\clang\\include"
  mk_add_options "export LIB=$LIB;C:\\w\\tools\\llvm\\obj\\lib"
  ac_add_options --enable-clang-plugin

Not sure how much bug 1278135 is related with this.
(Assignee)

Updated

2 years ago
Priority: P2 → --
I could be wrong but i don't have that you have the obj for the AST Matcher Library
(Assignee)

Comment 2

2 years ago
Some of the linking error on treeherder, you can see clangASTMatchers.lib is there: 

 00:27:40     INFO -  clangASTMatchers.lib(ASTMatchFinder.cpp.obj) : error LNK2001: unresolved external symbol "protected: class clang::Stmt * & __thiscall clang::StmtIteratorBase::GetDeclExpr(void)const " (?GetDeclExpr@StmtIteratorBase@clang@@IBEAAPAVStmt@2@XZ)
 4532 
 00:27:40     INFO -  clang-plugin.obj : error LNK2019: unresolved external symbol "protected: void __thiscall clang::StmtIteratorBase::NextDecl(bool)" (?NextDecl@StmtIteratorBase@clang@@IAEX_N@Z) referenced in function "bool __cdecl hasSideEffectAssignment(class clang::Expr const *)" (?hasSideEffectAssignment@@YA_NPBVExpr@clang@@@Z)
 4533 
 00:27:40     INFO -  clangASTMatchers.lib(ASTMatchFinder.cpp.obj) : error LNK2001: unresolved external symbol "protected: void __thiscall clang::StmtIteratorBase::NextDecl(bool)" (?NextDecl@StmtIteratorBase@clang@@IAEX_N@Z)

Comment 3

2 years ago
I believe that we don't support building firefox with clang-cl on windows yet, yet alone with the clang-plugin enabled. See bug 752004.
(Assignee)

Updated

2 years ago
Assignee: nobody → janus926
(Assignee)

Comment 5

2 years ago
Add "-FORCE:UNRESOLVED" doesn't help:

 2:57.65 The following command failed to execute properly:
 2:57.65 c:/w/fx/mc/obj-stan/_virtualenv/Scripts/python.exe -m mozbuild.action.cl c:/w/tools/llvm/obj/bin/clang-cl.EXE -fms-compatibility-version=19.00.23918 -fallback -FoTestGlobalClass.obj -c -DDEBUG=1 -DTRACING=1 -Ic:/w/fx/mc/build/clang-plugin/tests -Ic:/w/fx/mc/obj-stan/build/clang-plugin/tests -Ic:/w/fx/mc/obj-stan/dist/include -Ic:/w/fx/mc/obj-stan/dist/include/nspr -Ic:/w/fx/mc/obj-stan/dist/include/nss -MD -FI c:/w/fx/mc/obj-stan/mozilla-config.h -DMOZILLA_CLIENT -Qunused-arguments -Qunused-arguments -TP -nologo -wd5026 -wd5027 -Zc:sizedDealloc- -Zc:threadSafeInit- -wd4091 -wd4577 -D_HAS_EXCEPTIONS=0 -Gy -Zc:inline -arch:SSE2 -FS -Gw -wd4251 -wd4244 -wd4267 -wd4345 -wd4351 -wd4800 -wd4819 -wd4595 -we4553 -GR- -fsyntax-only -Xclang -verify -ferror-limit=0 -Zi -Xclang -load -Xclang c:/w/fx/mc/obj-stan/build/clang-plugin/clang-plugin.dll -Xclang -add-plugin -Xclang moz-check -O1 -Oi -Oy- -Fdgenerated.pdb c:/w/fx/mc/build/clang-plugin/tests/TestGlobalClass.cpp
 2:57.65 c:/w/fx/mc/config/rules.mk:882: recipe for target 'TestNANTestingExprC.obj' failed
 2:57.65 mozmake.EXE[5]: *** [TestNANTestingExprC.obj] Error 1
 2:57.65 c:/w/fx/mc/config/rules.mk:942: recipe for target 'TestExplicitOperatorBool.obj' failed
 2:57.65 mozmake.EXE[5]: *** [TestExplicitOperatorBool.obj] Error 1
 2:57.66
 2:57.66 c:/w/fx/mc/config/rules.mk:942: recipe for target 'TestGlobalClass.obj' failed
 2:57.66 mozmake.EXE[5]: *** [TestGlobalClass.obj] Error 1

Reapplying r260265 ran into an error because LLVM_IMPORT_REGISTRY has been removed in r277806. Ehsan, do you have any updates for clang plugin on windows?
Flags: needinfo?(ehsan)

Comment 6

2 years ago
(In reply to Ting-Yu Chou [:ting] from comment #5)
> Add "-FORCE:UNRESOLVED" doesn't help:
> 
>  2:57.65 The following command failed to execute properly:
>  2:57.65 c:/w/fx/mc/obj-stan/_virtualenv/Scripts/python.exe -m
> mozbuild.action.cl c:/w/tools/llvm/obj/bin/clang-cl.EXE
> -fms-compatibility-version=19.00.23918 -fallback -FoTestGlobalClass.obj -c
> -DDEBUG=1 -DTRACING=1 -Ic:/w/fx/mc/build/clang-plugin/tests
> -Ic:/w/fx/mc/obj-stan/build/clang-plugin/tests
> -Ic:/w/fx/mc/obj-stan/dist/include -Ic:/w/fx/mc/obj-stan/dist/include/nspr
> -Ic:/w/fx/mc/obj-stan/dist/include/nss -MD -FI
> c:/w/fx/mc/obj-stan/mozilla-config.h -DMOZILLA_CLIENT -Qunused-arguments
> -Qunused-arguments -TP -nologo -wd5026 -wd5027 -Zc:sizedDealloc-
> -Zc:threadSafeInit- -wd4091 -wd4577 -D_HAS_EXCEPTIONS=0 -Gy -Zc:inline
> -arch:SSE2 -FS -Gw -wd4251 -wd4244 -wd4267 -wd4345 -wd4351 -wd4800 -wd4819
> -wd4595 -we4553 -GR- -fsyntax-only -Xclang -verify -ferror-limit=0 -Zi
> -Xclang -load -Xclang
> c:/w/fx/mc/obj-stan/build/clang-plugin/clang-plugin.dll -Xclang -add-plugin
> -Xclang moz-check -O1 -Oi -Oy- -Fdgenerated.pdb
> c:/w/fx/mc/build/clang-plugin/tests/TestGlobalClass.cpp
>  2:57.65 c:/w/fx/mc/config/rules.mk:882: recipe for target
> 'TestNANTestingExprC.obj' failed
>  2:57.65 mozmake.EXE[5]: *** [TestNANTestingExprC.obj] Error 1
>  2:57.65 c:/w/fx/mc/config/rules.mk:942: recipe for target
> 'TestExplicitOperatorBool.obj' failed
>  2:57.65 mozmake.EXE[5]: *** [TestExplicitOperatorBool.obj] Error 1
>  2:57.66
>  2:57.66 c:/w/fx/mc/config/rules.mk:942: recipe for target
> 'TestGlobalClass.obj' failed
>  2:57.66 mozmake.EXE[5]: *** [TestGlobalClass.obj] Error 1
> 
> Reapplying r260265 ran into an error because LLVM_IMPORT_REGISTRY has been
> removed in r277806. Ehsan, do you have any updates for clang plugin on
> windows?

Yeah.  See https://reviews.llvm.org/D21385 for the LLVM change that removed LLVM_IMPORT_REGISTRY.

Basically with the new setup, clang is expected to export a function that the plugin will import.  Apparently the only thing that you need to do to make this work is ensuring that clang is built with LLVM_EXPORT_SYMBOLS_FOR_PLUGINS.  Can you please check your cmake config for the clang-cl build to make sure this setting is ON?  It may not be turned on by default on Windows.

(FWIW, I had filed bug 1290530 to test this fix for our clang-plugin, so if you end up fixing this here, please feel free to dupe that bug against this one.)

Note that a long time ago (before r277806) I had a clang-cl build working with --enable-clang-plugin, so hopefully this is just a matter of LLVM_EXPORT_SYMBOLS_FOR_PLUGINS not being turned on...
Flags: needinfo?(ehsan)

Updated

2 years ago
See Also: → bug 1290530
(Assignee)

Comment 7

2 years ago
Now I'm stuck at errors like below, and am trying to figure out where goes wrong:

 0:20.25 error(clang): 'error' diagnostics expected but not seen:
 0:20.25   File c:/w/fx/mc/build/clang-plugin/tests/TestCustomHeap.cpp Line 26: variable of type 'X' is not valid on
the heap
 0:20.25   File c:/w/fx/mc/build/clang-plugin/tests/TestCustomHeap.cpp Line 27: variable of type 'X' is not valid on
the heap
 0:20.25 error(clang): 'note' diagnostics expected but not seen:
 0:20.25   File c:/w/fx/mc/build/clang-plugin/tests/TestCustomHeap.cpp Line 26: value incorrectly allocated on the he
ap
 0:20.25   File c:/w/fx/mc/build/clang-plugin/tests/TestCustomHeap.cpp Line 27: value incorrectly allocated on the he
ap
 0:20.25 4 errors generated.
 0:20.26
 0:20.26 In the directory  /c/w/fx/mc/obj-stan/build/clang-plugin/tests
 0:20.26 The following command failed to execute properly:
 0:20.26 c:/w/fx/mc/obj-stan/_virtualenv/Scripts/python.exe -m mozbuild.action.cl c:/w/tools/llvm/obj/bin/clang-cl.EX
E -fms-compatibility-version=19.00.23918 -fallback -FoTestCustomHeap.obj -c -DDEBUG=1 -DTRACING=1 -Ic:/w/fx/mc/build/
clang-plugin/tests -Ic:/w/fx/mc/obj-stan/build/clang-plugin/tests -Ic:/w/fx/mc/obj-stan/dist/include -Ic:/w/fx/mc/obj
-stan/dist/include/nspr -Ic:/w/fx/mc/obj-stan/dist/include/nss -MD -FI c:/w/fx/mc/obj-stan/mozilla-config.h -DMOZILLA
_CLIENT -Qunused-arguments -Qunused-arguments -TP -nologo -wd5026 -wd5027 -Zc:sizedDealloc- -Zc:threadSafeInit- -wd40
91 -wd4577 -D_HAS_EXCEPTIONS=0 -Gy -Zc:inline -arch:SSE2 -FS -Gw -wd4251 -wd4244 -wd4267 -wd4345 -wd4351 -wd4800 -wd4
819 -wd4595 -we4553 -GR- -fsyntax-only -Xclang -verify -ferror-limit=0 -Zi -Xclang -load -Xclang c:/w/fx/mc/obj-stan/
build/clang-plugin/clang-plugin.dll -Xclang -add-plugin -Xclang moz-check -O1 -Oi -Oy- -Fdgenerated.pdb c:/w/fx/mc/bu
ild/clang-plugin/tests/TestCustomHeap.cpp
 0:20.27 c:/w/fx/mc/config/rules.mk:942: recipe for target 'TestCustomHeap.obj' failed
 0:20.27 mozmake.EXE[5]: *** [TestCustomHeap.obj] Error 1
Those are generated by our static analysis tool. It seems that the messages from the test changed against the ones reported by the clang-plugin.
(Assignee)

Comment 9

2 years ago
Seems not, running the example PrintFuncitonNames outputs nothing:

C:\w\tools\llvm\obj\bin>clang-cl.exe -Ic:\w\tools\llvm\obj\tools\clang\include -Ic:\w\tools\llvm\src\tools\clang\include -Ic:\w\tools\llvm\obj\include -Ic:\w\tools\llvm\src\include -fsyntax-only -Xclang -load -Xclang c:\w\tools\llvm\obj\bin\PrintFunctionNames.dll -Xclang -add-plugin -Xclang print-fns c:\w\tools\llvm\src\tools\clang\tools\clang-check\ClangCheck.cpp
(Assignee)

Comment 10

2 years ago
Because the plugin links to the import library of clang.exe, the binary loading it has to be clang.exe otherwise the path is not matched and another copy of clang module will be loaded. I am trying to fix it from makefile.

Comment 11

2 years ago
Yes, I think you're on to the core of this issue.  clang just ignored plugins that it cannot load, IIRC.
(Assignee)

Comment 12

2 years ago
Created attachment 8813528 [details]
wip

I'm now investigating why the assertion:

  https://github.com/llvm-mirror/clang/blob/master/lib/ASTMatchers/ASTMatchersInternal.cpp#L116

is triggered when DiagnosticsMatcher() calls addMatcher().
(Assignee)

Comment 13

2 years ago
Forget about the assertion, not an issue for clang release build. Now I'm trying to figure out why TestCustomHeap is failed.

Comment 14

2 years ago
(In reply to Ting-Yu Chou [:ting] from comment #13)
> Forget about the assertion, not an issue for clang release build.

That's probably because assertions are disabled in that build configuration.

The last time I tried building with a debug clang, I also got tons of assertions from our plugin...
(Assignee)

Comment 15

2 years ago
The foo and foo2 in misuseX() at TestCustomHeap.cpp are not listed in the output of "clang -Xclang -ast-dump", I guess they're optimized out somehow.
(Assignee)

Comment 16

2 years ago
 0:04.68 error(clang): 'warning' diagnostics seen but not expected:
 0:04.68   File c:/w/fx/mc/obj-stan/dist/include\mozilla/Assertions.h Line 213: function declared 'noreturn' should not return
 0:04.68 1 error generated.

I'm not sure how to deal with this one, "-W%" are stripped from OS_CXXFLAGS in Makefile.in so I assume adding -Wno-invalid-noreturn is not relevant.
(Assignee)

Comment 17

2 years ago
Add "explicit" to char16ptr_t constructors causes

 0:32.95 In file included from c:/w/fx/mc/obj-stan/dist/include/mozilla/dom/DOMString.h:16:
 0:32.95 c:/w/fx/mc/obj-stan/dist/include/nsIAtom.h(53,12):  error(clang): no viable conversion from returned value of type 'char16_t *const' to function return type 'char16ptr_t'
 0:32.95     return mString;
 0:32.95            ^~~~~~~

so probably they're intentionally not added. Ehsan, do you have any ideas why they're not added in bug 1009631?
(Assignee)

Comment 18

2 years ago
(In reply to Ting-Yu Chou [:ting] from comment #17)
> Add "explicit" to char16ptr_t constructors causes
> 
>  0:32.95 In file included from
> c:/w/fx/mc/obj-stan/dist/include/mozilla/dom/DOMString.h:16:
>  0:32.95 c:/w/fx/mc/obj-stan/dist/include/nsIAtom.h(53,12):  error(clang):
> no viable conversion from returned value of type 'char16_t *const' to
> function return type 'char16ptr_t'
>  0:32.95     return mString;
>  0:32.95            ^~~~~~~
> 
> so probably they're intentionally not added. Ehsan, do you have any ideas
> why they're not added in bug 1009631?

Based on bug 928351, they seem to be needed for implicit conversion from char16_t. I'll add Char16.h to the ignored path.
(Assignee)

Comment 19

2 years ago
(In reply to Ting-Yu Chou [:ting] from comment #16)
>  0:04.68 error(clang): 'warning' diagnostics seen but not expected:
>  0:04.68   File c:/w/fx/mc/obj-stan/dist/include\mozilla/Assertions.h Line
> 213: function declared 'noreturn' should not return
>  0:04.68 1 error generated.
> 
> I'm not sure how to deal with this one, "-W%" are stripped from OS_CXXFLAGS
> in Makefile.in so I assume adding -Wno-invalid-noreturn is not relevant.

Add MOZ_PRETEND_NORETURN_FOR_STATIC_ANALYSIS doesn't help, I'll see if I can make it ignored from clang-plugin.
(Assignee)

Comment 20

2 years ago
(In reply to Ting-Yu Chou [:ting] from comment #19)
> Add MOZ_PRETEND_NORETURN_FOR_STATIC_ANALYSIS doesn't help, I'll see if I can
> make it ignored from clang-plugin.

I can't.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8813528 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1290530

Comment 29

2 years ago
mozreview-review
Comment on attachment 8814376 [details]
Bug 1316545 part 5 - Make misuseX() concrete such that the AST is computed even if it is unused.

https://reviewboard.mozilla.org/r/95642/#review95708

So, I don't think that this is the right solution to the problem at hand here. The problem which you are running into is that clang-cl handles templates differently than clang does in order to have compatibility with cl's strange non-standard behaviors. One of those behaviors is that cl is more permissive with regard to templates than clang is. Clang makes an effort to understand the body of a templated function before specialization, while cl basically stores around the body and recompiles it every time the template is specialized. This means that clang would reject templated code which cl wouldn't. For compat reasons, clang-cl acts like cl does in this situation, and doesn't compute an AST for the body of an unspecialized template.

I think that the better way to solve this problem would be either to:

a) Create a concrete function which calls a specialized version of misuseX() (which would force specialization), or
b) Make misuseX() concrete, as its body doesn't depend on the template parameter anyways.
Attachment #8814376 - Flags: review?(michael) → review-

Comment 30

2 years ago
mozreview-review
Comment on attachment 8814372 [details]
Bug 1316545 part 1 - Link clang plugin with the import library of clang.exe to fix unresolved symbols.

https://reviewboard.mozilla.org/r/95634/#review95728
Attachment #8814372 - Flags: review+

Comment 31

2 years ago
mozreview-review
Comment on attachment 8814373 [details]
Bug 1316545 part 2 - Enable LLVM_EXPORT_SYMBOLS_FOR_PLUGIN for linking clang plugin on windows.

https://reviewboard.mozilla.org/r/95636/#review95730
Attachment #8814373 - Flags: review?(ehsan) → review+

Comment 32

2 years ago
mozreview-review
Comment on attachment 8814374 [details]
Bug 1316545 part 3 - Remove LLVM_EXPORT_REGISTRY which has been dropped in r277806.

https://reviewboard.mozilla.org/r/95638/#review95732
Attachment #8814374 - Flags: review?(ehsan) → review+

Comment 33

2 years ago
mozreview-review
Comment on attachment 8814375 [details]
Bug 1316545 part 4 - Run clang.exe instead of clang-cl.exe for loading the plugin.

https://reviewboard.mozilla.org/r/95640/#review95734

::: build/clang-plugin/tests/Makefile.in:8
(Diff revision 1)
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  # Build without any warning flags, and with clang verify flag for a
>  # syntax-only build (no codegen), without a limit on the number of errors.
> -OS_CFLAGS := $(filter-out -W%,$(OS_CFLAGS)) -fsyntax-only -Xclang -verify -ferror-limit=0 -std=c11
> -OS_CXXFLAGS := $(filter-out -W%,$(OS_CXXFLAGS)) -fsyntax-only -Xclang -verify -ferror-limit=0
> +OS_CFLAGS := $(filter-out -W% -w%,$(OS_CFLAGS)) -fsyntax-only -Xclang -verify -ferror-limit=0 -std=c11
> +OS_CXXFLAGS := $(filter-out -W% -w%,$(OS_CXXFLAGS)) -fsyntax-only -Xclang -verify -ferror-limit=0

Why these changes?

::: build/clang-plugin/tests/Makefile.in:16
(Diff revision 1)
> +# module clang.exe again when load the plugin dll, which links to the import
> +# library of clang.exe.
> +ifeq ($(OS_ARCH),WINNT)
> +CC := $(subst clang-cl.exe,clang.exe --driver-mode=cl,$(CC:.EXE=.exe))
> +CXX := $(subst clang-cl.exe,clang.exe --driver-mode=cl,$(CXX:.EXE=.exe))
> +endif

Can you please describe why you're doing this?  I don't understand how these changes are only needed in this directory.  I think this may be a result of some other problem elsewhere that you're working around like this.

Comment 34

2 years ago
mozreview-review
Comment on attachment 8814377 [details]
Bug 1316545 part 6 - Ignore the implicit conversions from Char16.h as they're intended.

https://reviewboard.mozilla.org/r/95644/#review95736

Instead of special casing the analysis, you need to mark the char16ptr_t constructors as MOZ_IMPLICIT.
Attachment #8814377 - Flags: review?(ehsan) → review-

Comment 35

2 years ago
mozreview-review
Comment on attachment 8814378 [details]
Bug 1316545 part 7 - Suppress the warning of invalid noreturn for static analysis.

https://reviewboard.mozilla.org/r/95646/#review95738

Instead of doing this, you need to add -Wno-invalid-noreturn to OS_CFLAGS and OS_CXXFLAGS in build/clang-plugin/tests/Makefile.in.
Attachment #8814378 - Flags: review-

Updated

2 years ago
Attachment #8814378 - Flags: review?(nfroyd)

Updated

2 years ago
Attachment #8814372 - Flags: review?(mh+mozilla)

Updated

2 years ago
Attachment #8814375 - Flags: review?(mh+mozilla)
(Assignee)

Comment 36

2 years ago
mozreview-review-reply
Comment on attachment 8814375 [details]
Bug 1316545 part 4 - Run clang.exe instead of clang-cl.exe for loading the plugin.

https://reviewboard.mozilla.org/r/95640/#review95734

> Can you please describe why you're doing this?  I don't understand how these changes are only needed in this directory.  I think this may be a result of some other problem elsewhere that you're working around like this.

I see, so the place I should fix is config/static-checking-config.mk, is my understanding correct?
(Assignee)

Comment 37

2 years ago
mozreview-review-reply
Comment on attachment 8814375 [details]
Bug 1316545 part 4 - Run clang.exe instead of clang-cl.exe for loading the plugin.

https://reviewboard.mozilla.org/r/95640/#review95734

> Why these changes?

Will remove them.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8814372 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8814373 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8814374 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8814375 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8814377 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8814378 - Attachment is obsolete: true
(Assignee)

Comment 39

2 years ago
I thought "hg push -c review" pushes only the updated patch and keep the others. :(
(Assignee)

Comment 40

2 years ago
I am still reworking part 4 and part 6, will fix the reviews later. Sorry for the mess.

Comment 41

2 years ago
mozreview-review
Comment on attachment 8814376 [details]
Bug 1316545 part 5 - Make misuseX() concrete such that the AST is computed even if it is unused.

https://reviewboard.mozilla.org/r/95642/#review96490

LGTM but can you change the commit message? I think that something like
`Part 5 - Make misuseX() concrete such that the AST is computed even if it is unused`

It is no longer a template, so we aren't making clang-cl compute the AST for the body of an unspecialized template.
Attachment #8814376 - Flags: review?(michael) → review+
(Assignee)

Updated

2 years ago
Attachment #8814372 - Attachment is obsolete: false
(Assignee)

Updated

2 years ago
Attachment #8814372 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 48

2 years ago
Comment on attachment 8815565 [details]
Bug 1316545 part 1 - Link clang plugin with the import library of clang.exe to fix unresolved symbols.

Carry r+.
Attachment #8815565 - Flags: review?(ehsan) → review+
(Assignee)

Updated

2 years ago
Attachment #8815566 - Flags: review?(ehsan) → review+
(Assignee)

Updated

2 years ago
Attachment #8815567 - Flags: review?(ehsan) → review+
(Assignee)

Comment 49

2 years ago
Now the plugin outputs many implicit conversion and few kung fu death grip errors, are there anything I should be careful while fixing them?
(Assignee)

Comment 50

2 years ago
If there're no other concerns, I'd like to file another bug for fixing the errors that the plugin reports.

Comment 51

2 years ago
mozreview-review
Comment on attachment 8815568 [details]
Bug 1316545 part 4 - Run clang.exe instead of clang-cl.exe for loading the plugin.

https://reviewboard.mozilla.org/r/96252/#review96828

Yes, this is the right place to do this, I think.  Please also get a review from glandium on this from the build system perspective.
Attachment #8815568 - Flags: review?(ehsan) → review+

Comment 52

2 years ago
mozreview-review
Comment on attachment 8815569 [details]
Bug 1316545 part 6 - Suppress the warning of invalid noreturn.

https://reviewboard.mozilla.org/r/96254/#review96830
Attachment #8815569 - Flags: review?(ehsan) → review+

Comment 53

2 years ago
(In reply to Ting-Yu Chou [:ting] from comment #49)
> Now the plugin outputs many implicit conversion and few kung fu death grip
> errors, are there anything I should be careful while fixing them?

The errors about implicit conversions are usually easy to fix by making the constructor explicit (or making it MOZ_IMPLICIT if the constructor is really intended to make the implicit conversion possible.)

The kung fu death grip errors should be fixed like this.  Where you have a pattern like:

  nsCOMPtr<nsIFoo> kungFuDeathGrip(obj);
  obj->DoStuff();

you need to change the usages of obj to also happen on the kungfudeathgrip, like |kungFuDeathGrip->DoStuff();|.

Please let me know if you had any questions, I'd be happy to help!

(In reply to Ting-Yu Chou [:ting] from comment #50)
> If there're no other concerns, I'd like to file another bug for fixing the
> errors that the plugin reports.

That sounds like a great idea!

I think you can land these patches before that work is complete, FWIW.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8815565 - Flags: review?(ehsan)
Attachment #8815566 - Flags: review?(ehsan)
Attachment #8815567 - Flags: review?(ehsan)
(Assignee)

Updated

2 years ago
Attachment #8815565 - Flags: review?(ehsan)
Attachment #8815566 - Flags: review?(ehsan)
Attachment #8815567 - Flags: review?(ehsan)
(Assignee)

Updated

2 years ago
Blocks: 1321453
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8815566 - Flags: review?(ehsan)
Attachment #8815567 - Flags: review?(ehsan)
(Assignee)

Comment 62

2 years ago
Ehsan, I am sorry for the review request spam...

Comment 63

2 years ago
mozreview-review
Comment on attachment 8815568 [details]
Bug 1316545 part 4 - Run clang.exe instead of clang-cl.exe for loading the plugin.

https://reviewboard.mozilla.org/r/96252/#review97422

Ugly hack, but meh.
Attachment #8815568 - Flags: review?(mh+mozilla) → review+

Comment 64

2 years ago
Pushed by tchou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6efe44b0c70d
part 1 - Link clang plugin with the import library of clang.exe to fix unresolved symbols. r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbda600d4a14
part 2 - Enable LLVM_EXPORT_SYMBOLS_FOR_PLUGINS for linking clang plugin on windows. r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cbc0be626a0
part 3 - Remove LLVM_EXPORT_REGISTRY which has been dropped in r277806. r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/46bc9b158001
part 4 - Run clang.exe instead of clang-cl.exe for loading the plugin. r=ehsan,glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a86e67f78c6
part 5 - Make misuseX() concrete such that the AST is computed even if it is unused. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad869ab08f36
part 6 - Suppress the warning of invalid noreturn. r=ehsan
(Assignee)

Comment 65

2 years ago
I'll make another patch to update browser/config/tooltool-manifests/winxx/clang.manifest once the patches are landed.
Keywords: leave-open
(In reply to Ting-Yu Chou [:ting] from comment #65)
> I'll make another patch to update
> browser/config/tooltool-manifests/winxx/clang.manifest once the patches are
> landed.

There's a patch over in bug 1321444 comment 2 for exactly this.
(Assignee)

Comment 68

2 years ago
(In reply to Nathan Froyd [:froydnj] from comment #67)
> There's a patch over in bug 1321444 comment 2 for exactly this.

Yes, but I assume it does not have LLVM_EXPORT_SYMBOLS_FOR_PLUGINS enabled, which is required for the plugin?
(In reply to Ting-Yu Chou [:ting] from comment #68)
> (In reply to Nathan Froyd [:froydnj] from comment #67)
> > There's a patch over in bug 1321444 comment 2 for exactly this.
> 
> Yes, but I assume it does not have LLVM_EXPORT_SYMBOLS_FOR_PLUGINS enabled,
> which is required for the plugin?

It does not.  If that is needed for the plugin to work, then we should add that to the build-clang scripts.
(Assignee)

Comment 70

2 years ago
Yes, it has been added, see the patch part 2 here (attachment 8815566 [details]).
(Assignee)

Comment 71

2 years ago
clang.lib is not copied to the install dir, so clang.tar.bz2 does not have it under /lib. :(
(Assignee)

Comment 72

2 years ago
(In reply to Ting-Yu Chou [:ting] from comment #71)
> clang.lib is not copied to the install dir, so clang.tar.bz2 does not have
> it under /lib. :(

I've sent an email to llvm-dev and the author of LLVM_EXPORT_SYMBOLS_FOR_PLUGINS (john.brawn) to ask if this intended, but haven't got any responses.

Ehsan, it seems that not all lib/*.lib files in the build tree are installed, do you know why? If it's intended, we can workaround it in build-clang.py.
Flags: needinfo?(ehsan)

Comment 73

2 years ago
(In reply to Ting-Yu Chou [:ting] from comment #72)
> Ehsan, it seems that not all lib/*.lib files in the build tree are
> installed, do you know why? If it's intended, we can workaround it in
> build-clang.py.

I'm not sure why, it could be that they didn't think those libraries are useful?  I think we should just copy all additional libraries we need in build-clang.py.
Flags: needinfo?(ehsan)
(Assignee)

Comment 74

2 years ago
Created attachment 8817584 [details] [diff] [review]
Bug 1316545 part 7 - Install also the import library of clang.exe.
Attachment #8817584 - Flags: review?(ehsan)
(Assignee)

Comment 75

2 years ago
Created attachment 8817585 [details] [diff] [review]
Bug 1316545 part 8 - Update clang manifests for the binaries with LLVM_EXPORT_SYMBOLS_FOR_PLUGINS enabled.
Attachment #8817585 - Flags: review?(ehsan)

Updated

2 years ago
Attachment #8817584 - Flags: review?(ehsan) → review+

Updated

2 years ago
Attachment #8817585 - Flags: review?(ehsan) → review+

Comment 76

2 years ago
Pushed by tchou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f42641bcb344
part 7 - Install also the import library of clang.exe. r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/463b97f06397
part 8 - Update clang manifests for the binaries with LLVM_EXPORT_SYMBOLS_FOR_PLUGINS enabled. r=ehsan
(Assignee)

Updated

2 years ago
Keywords: leave-open

Comment 77

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f42641bcb344
https://hg.mozilla.org/mozilla-central/rev/463b97f06397
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
status-firefox52: affected → ---

Updated

8 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.