Closed Bug 1316545 Opened 8 years ago Closed 7 years ago

Fails linking clang-plugin with unresolved clang symbols

Categories

(Developer Infrastructure :: Source Code Analysis, defect)

Product:
Developer Infrastructure
Component:
Source Code Analysis
Platform:
Unspecified
Windows
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: ting, Assigned: ting)

References

(
URL
)

Details

Attachments

(8 files, 7 obsolete files)

Bug 1316545 part 5 - Make misuseX() concrete such that the AST is computed even if it is unused.
58 bytes, text/x-review-board-request
nika
: review+
Details
Bug 1316545 part 1 - Link clang plugin with the import library of clang.exe to fix unresolved symbols.
58 bytes, text/x-review-board-request
Details
Bug 1316545 part 2 - Enable LLVM_EXPORT_SYMBOLS_FOR_PLUGIN for linking clang plugin on windows.
58 bytes, text/x-review-board-request
Details
Bug 1316545 part 3 - Remove LLVM_EXPORT_REGISTRY which has been dropped in r277806.
58 bytes, text/x-review-board-request
Details
Bug 1316545 part 4 - Run clang.exe instead of clang-cl.exe for loading the plugin.
58 bytes, text/x-review-board-request
ehsan.akhgari
: review+
glandium
: review+
Details
Bug 1316545 part 6 - Suppress the warning of invalid noreturn.
58 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1316545 part 7 - Install also the import library of clang.exe.
1.97 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Bug 1316545 part 8 - Update clang manifests for the binaries with LLVM_EXPORT_SYMBOLS_FOR_PLUGINS enabled.
2.18 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review 
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.
Priority: P2 → --
I could be wrong but i don't have that you have the obj for the AST Matcher Library
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)
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: nobody → janus926
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?
URL: https://llvm.org/bugs/show_bug.cgi?id...
Flags: needinfo?(ehsan)
(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)
See Also: → 1290530
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.
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
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.
Yes, I think you're on to the core of this issue.  clang just ignored plugins that it cannot load, IIRC.
Attached file wip (obsolete) — 
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().
Forget about the assertion, not an issue for clang release build. Now I'm trying to figure out why TestCustomHeap is failed.
(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...
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.
 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 "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?
(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.
(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.
(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.
Attachment #8813528 - Attachment is obsolete: true
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 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 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 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 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 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 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-
Attachment #8814378 - Flags: review?(nfroyd)
Attachment #8814372 - Flags: review?(mh+mozilla)
Attachment #8814375 - Flags: review?(mh+mozilla)
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?
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.
Attachment #8814372 - Attachment is obsolete: true
Attachment #8814373 - Attachment is obsolete: true
Attachment #8814374 - Attachment is obsolete: true
Attachment #8814375 - Attachment is obsolete: true
Attachment #8814377 - Attachment is obsolete: true
Attachment #8814378 - Attachment is obsolete: true
I thought "hg push -c review" pushes only the updated patch and keep the others. :(
I am still reworking part 4 and part 6, will fix the reviews later. Sorry for the mess.
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+
Attachment #8814372 - Attachment is obsolete: false
Attachment #8814372 - Attachment is obsolete: true
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+
Attachment #8815566 - Flags: review?(ehsan) → review+
Attachment #8815567 - Flags: review?(ehsan) → review+
Now the plugin outputs many implicit conversion and few kung fu death grip errors, are there anything I should be careful while fixing them?
If there're no other concerns, I'd like to file another bug for fixing the errors that the plugin reports.
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 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+
(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.
Attachment #8815565 - Flags: review?(ehsan)
Attachment #8815566 - Flags: review?(ehsan)
Attachment #8815567 - Flags: review?(ehsan)
Attachment #8815565 - Flags: review?(ehsan)
Attachment #8815566 - Flags: review?(ehsan)
Attachment #8815567 - Flags: review?(ehsan)
Blocks: 1321453
Attachment #8815566 - Flags: review?(ehsan)
Attachment #8815567 - Flags: review?(ehsan)
Ehsan, I am sorry for the review request spam...
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+
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
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.
(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.
Yes, it has been added, see the patch part 2 here (attachment 8815566 [details]).
clang.lib is not copied to the install dir, so clang.tar.bz2 does not have it under /lib. :(
(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)
(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)
Attachment #8817584 - Flags: review?(ehsan) → review+
Attachment #8817585 - Flags: review?(ehsan) → review+
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
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/f42641bcb344
https://hg.mozilla.org/mozilla-central/rev/463b97f06397
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Product: Core → Firefox Build System
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: