Closed
Bug 1349268
Opened 8 years ago
Closed 8 years ago
ASan: obj/dom/bindings/TestCodeGenBinding.cpp:6236:7: error: code will never be executed [-Werror,-Wunreachable-code], due to "if ((false) &&...)" in generated source code
Categories
(Firefox Build System :: General, defect, P3)
Firefox Build System
General
Tracking
(firefox55 affected)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox55 | --- | affected |
People
(Reporter: decoder, Unassigned)
References
Details
(Keywords: sec-want)
Attachments
(1 file)
+++ This bug was initially created as a clone of Bug #1291397 +++
I am currently trying to switch out ASan builds to Clang 3.9 by using the manifest at browser/config/tooltool-manifests/linux64/releng.manifest as a replacement for asan.manifest. The Clang revision in there is r290136 (according to comment), so it should have the fix for the original bug.
Nevertheless, I get the same warning as in bug 1291397:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c2850c8f5bdde61bb20d61834b1f31aee792511&selectedJob=83427761
/home/worker/workspace/build/src/obj-firefox/dom/bindings/TestCodeGenBinding.cpp:6639:7: error: code will never be executed [-Werror,-Wunreachable-code]
/home/worker/workspace/build/src/obj-firefox/dom/bindings/TestCodeGenBinding.cpp:6668:7: error: code will never be executed [-Werror,-Wunreachable-code]
gmake[5]: *** [TestCodeGenBinding.o] Error 1
I'm currently trying to find out when this was regressed on the Clang side so we might have a chance to figure out where the fix went in (more recent Clang from SVN builds this fine apparently).
In the meantime, it might be a viable option to disable -Wunreachable-code on ASan builds to unblock the upgrade to Clang 3.9 which is important for us.
Comment 1•8 years ago
|
||
(In reply to Christian Holler (:decoder) from comment #0)
> I am currently trying to switch out ASan builds to Clang 3.9 by using the
> manifest at browser/config/tooltool-manifests/linux64/releng.manifest
The maybe-problematic clang 3.9 build was added to that manifest file in "Bug 1302028 part 1", FWIW:
https://hg.mozilla.org/mozilla-central/rev/eee5f618279a#l3.13
--> Adding dependency on that bug.
Depends on: 1302028
Comment 2•8 years ago
|
||
froydnj (or decoder), do you know if it's possible to download the clang.tar.xz tarball referenced in that releng.manifest file? (which the manifest calls "clang + llvm 3.9.0, built from SVN r290136")
I checked out llvm+clang revision 290136 locally, and I verified that that revision *does* have the fix for this over-spammy warning (its files do show the changes from https://reviews.llvm.org/D24010 ). So if the clang tarball we're using was really built from that revision, then it should not be spamming this warning...
Flags: needinfo?(nfroyd)
Comment 3•8 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #2)
> froydnj (or decoder), do you know if it's possible to download the
> clang.tar.xz tarball referenced in that releng.manifest file? (which the
> manifest calls "clang + llvm 3.9.0, built from SVN r290136")
You can get it at: https://api.pub.build.mozilla.org/tooltool/sha512/4ab5ff2131e4ce4888d38c17feb192c19bc6ede83abef55af7d2f29e2446f6335dc860377fa25cbb0283b3958c0a3d377a3cfdc7705a85d4843e3ab357ddca7f
..or more properly, use the tooltool script from https://raw.githubusercontent.com/mozilla/build-tooltool/master/tooltool.py
tooltool.py fetch -m <manifest>
Flags: needinfo?(nfroyd)
Comment 4•8 years ago
|
||
Thanks.
I downloaded & extracted that build, and it indeed looks to me like it was built from an older version of clang. I'm attaching the clang/include/clang/AST/Stmt.h file from that build, it does *not* have the chunk that was added in https://reviews.llvm.org/D24010 (a one-liner function "const Stmt *IgnoreImplicit()")
Its "clang --version" does *report* itself as being built from revision 290136, but I'm guessing that might only be the LLVM checkout and not the clang ("cfe") checkout -- those are checked out from SVN separately, and it'd be easy to mistakenly use mismatched versions I think.
Assignee: nobody → dholbert
Comment 5•8 years ago
|
||
(oops, accidentally assigned the bug to myself. Resetting that)
Does anyone (maybe froydnj) know how that clang.tar.xz build was generated?
Assignee: dholbert → nobody
Flags: needinfo?(nfroyd)
Updated•8 years ago
|
Attachment #8849638 -
Attachment mime type: text/x-chdr → text/plain
Comment hidden (obsolete) |
Comment 7•8 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #5)
> (oops, accidentally assigned the bug to myself. Resetting that)
>
> Does anyone (maybe froydnj) know how that clang.tar.xz build was generated?
clang.tar.xz gets generated from build-clang.py, run on our automation machines:
http://dxr.mozilla.org/mozilla-central/source/build/build-clang/build-clang.py
It is entirely possible that the tarball getting used in the releng.manifest file was built without the warning fix, in which case we should force a new build, upload the tarball generated to tooltool (if it's not there already), and update the manifest to match.
Flags: needinfo?(nfroyd)
Comment 8•8 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #7)
> (In reply to Daniel Holbert [:dholbert] from comment #5)
> > (oops, accidentally assigned the bug to myself. Resetting that)
> >
> > Does anyone (maybe froydnj) know how that clang.tar.xz build was generated?
>
> clang.tar.xz gets generated from build-clang.py, run on our automation
> machines:
>
> http://dxr.mozilla.org/mozilla-central/source/build/build-clang/build-clang.
> py
>
> It is entirely possible that the tarball getting used in the releng.manifest
> file was built without the warning fix, in which case we should force a new
> build, upload the tarball generated to tooltool (if it's not there already),
> and update the manifest to match.
The current clang in the manifest was built for bug 1302028. The Linux static analysis builds use clang built after the changes for bug 1331957, which contain the warning fix patch. Using:
2a5458a25792fcade86a56ff0f4acdfa284d2b62966991a7c34a92c2e8c0b4a162ce00512d4467754e7f74598d64c56e91517e1606ed3fba011f7c10e8ad3288
in the tooltool manifest for the asan build should solve problems, I think?
Comment 9•8 years ago
|
||
decoder, can you try using the sha512 in comment 8 (size 168062128 bytes) in your ASan runs and see if that fixes the issue?
Flags: needinfo?(choller)
Comment 10•8 years ago
|
||
Mystery solved over IRC -- looks like llvm just neglected to uplift the fix ( https://reviews.llvm.org/D24010 ) to their release branch (and their release branch & trunk branch use overlapping SVN revision numbers, which is what confused decoder [and me] about wait-this-should-be-fixed-in-this-revision).
The tree that has the fix (which is what I was inspecting in comment 2) is:
https://llvm.org/svn/llvm-project/cfe/trunk
The tree that we're using for our clang tarball is:
https://llvm.org/svn/llvm-project/cfe/tags/RELEASE_390/final
And that latter "release" tree does NOT have the fix, even in its latest revision.
We could bug llvm folks to try to uplift their fix to that release branch, but froydnj points out that they're perhaps unlikely to make any more clang 3.9 changes, since clang 4.0 is already out. (Though if an Linux distros ship with clang 3.9 as the default compiler, it still might be worth doing.)
Comment 11•8 years ago
|
||
(BTW, I posed on the LLVM bug where this was patched, https://bugs.llvm.org/show_bug.cgi?id=29152#c6 , and it sounds like they might uplift for a 3.9.2; stay tuned. But in the meantime, it looks like the hash that froydnj mentioned in comment 8 does include the LLVM fix as a manual spot-fix on our end, via this local clang-patchfile which ehsan imported into our tree in bug 1331957
https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/build/build-clang/r285657.patch
)
Reporter | ||
Comment 12•8 years ago
|
||
The build in comment 8 indeed solves the problem. I added this hash to releng.manifest in bug 1349611.
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(choller)
Resolution: --- → FIXED
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•