1349268 - 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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P3)

Description

[Christian Holler (:decoder)]

+++ 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

[Daniel Holbert [:dholbert]]

(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.

Comment 2

[Daniel Holbert [:dholbert]]

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

Comment 3

[Jesse Schwartzentruber (:truber)]

(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>

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: clang/include/clang/AST/Stmt.h from the tarball that we build with (*does not* have the patch)

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.

Comment 5

[Daniel Holbert [:dholbert]]

(oops, accidentally assigned the bug to myself. Resetting that)

Does anyone (maybe froydnj) know how that clang.tar.xz build was generated?

Comment 6

[Daniel Holbert [:dholbert]]

(Bottom-line, to fix this bug, we probably need to regenerate this clang.tar.xz with an actually-matching revision of the "cfe" & "llvm" libraries -- both 290136, if we want to stick with that revision.  Right now, comment 4 proves that the cfe (clang) checkout in this build was taken from an older revision, based on missing changes in its Stmt.h header [which are present in a manual local checkout of cfe revision 290136, as I noted in comment 2)

Comment 7

[Nathan Froyd [:froydnj]]

(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.

Comment 8

[Nathan Froyd [:froydnj]]

(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

[Nathan Froyd [:froydnj]]

decoder, can you try using the sha512 in comment 8 (size 168062128 bytes) in your ASan runs and see if that fixes the issue?

Comment 10

[Daniel Holbert [:dholbert]]

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

[Daniel Holbert [:dholbert]]

(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
)

Comment 12

[Christian Holler (:decoder)]

The build in comment 8 indeed solves the problem. I added this hash to releng.manifest in bug 1349611.