I would like to upgrade the Clang we use for our ASan builds to Clang Trunk because it has several improvements related to memory usage that are not yet in Clang 5.0 (according to kcc), but which we need for the ASan Nightly Project. My understanding is that I can simply add a build job definition "linux64-clang-trunk" to taskcluster/ci/toolchain/linux.yml, just like we did for Clang 5 recently. Once I have that, I guess it is sufficient to change "linux64-clang" to the job I added, for all the ASan builds in taskcluster/ci/build/linux.yml. Mike, can you confirm that these steps are correct? Am I missing something that I should be aware of? Thanks!

Yes, but please don't call it clang-trunk, because that would kind of imply it's always on trunk, while it will be some version fixed in time. Call it clang-6 or clang-6-pre.

(In reply to Christian Holler (:decoder) from comment #2) > Created attachment 8927809 [details] The attached patch does the following things: * Adds a clang-6-pre build for linux64 (Clang Trunk revision that was current when I wrote the patch) * Adds a patch for the ASan runtime in compiler-rt that allows ASan to look up the llvm-symbolizer binary next to the firefox binary (default is CWD and PATH only) * Switches all ASan Linux builds to the new clang-6-pre * Disables two new warnings (-Wunused-private-field and -Wdeprecated-register) until the follow-up bugs (bug 1416183 and bug 1416185) are fixed This is green on try with the patches from bug 1415782 on top (thanks Julian!). The compiler-rt patch is important for the ASan Nightly project because in testing, we ended up with unsymbolized traces in the backend (and we cannot expect people to install llvm-symbolizer into PATH or CWD). I missed this earlier in testing because I always started the Firefox binary after changing to the containing firefox/ directory, so it is CWD. The patch is Linux only and I've already asked the ASan devs if this can be implemented. If they decide to implement this, then we can simply remove our patch. Otherwise, we will have to find another way to do this (e.g. by hacking the same code into __asan_default_options in mozglue/build/ but that option doesn't seem very pleasant if we can have this use case supported by compiler-rt natively). In a follow-up bug, I will then enable the scribble feature (overwrites free'd memory to detect more uafs) and possibly tune some memory options if necessary, but I wanted to do that in a separate bug because some of these changes also affect performance and might be asan-reporter only.

Comment on attachment 8927809 [details] Bug 1415689 - Add Clang 6 (pre) and use it for ASan builds. https://reviewboard.mozilla.org/r/199104/#review204534 Looks pretty good, just a few things to fix. ::: build/build-clang/find_symbolizer_linux.patch:1 (Diff revision 1) > +diff --git a/compiler-rtlib/sanitizer_common/sanitizer_file.cc b/compiler-rt/lib/sanitizer_common/sanitizer_file.cc Can you provide some descriptive comment, either in the patch itself, or prior to the `diff --git` line, so somebody doesn't have to trawl through blame to figure out why this patch was added? Are we submitting this patch upstream? ::: build/build-clang/find_symbolizer_linux.patch:22 (Diff revision 1) > + if (*end == '\0') break; > + beg = end + 1; > + } > ++ > ++#if SANITIZER_LINUX > ++ internal_readlink("/proc/self/exe", buffer.data(), kMaxPathLength); Can this call fail? I know some people run without `/proc` mounted... We should either handle failure here or use something that can handle failure gracefully. ::: build/unix/mozconfig.asan:16 (Diff revision 1) > +# These can be removed once bugs 1416183 and 1416185 are fixed > +export CFLAGS="$CFLAGS -Wno-unused-private-field -Wno-deprecated-register" > +export CXXFLAGS="$CXXFLAGS -Wno-unused-private-field -Wno-deprecated-register" Let's just not commit this patch until the dependent bugs are fixed, please?

(In reply to Nathan Froyd [:froydnj] from comment #4) > > ::: build/build-clang/find_symbolizer_linux.patch:1 > (Diff revision 1) > > +diff --git a/compiler-rtlib/sanitizer_common/sanitizer_file.cc b/compiler-rt/lib/sanitizer_common/sanitizer_file.cc > > Can you provide some descriptive comment, either in the patch itself, or > prior to the `diff --git` line, so somebody doesn't have to trawl through > blame to figure out why this patch was added? Yes, I can do that on Thursday when I'm back from PTO. > > Are we submitting this patch upstream? I am discussing with upstream how we should handle this. They asked for the patch, but in order to make this upstream, they probably would want a platform independent solution so making this work for upstream might take longer. Also they might request changes to the way I'm doing this here. I just implemented it such that it works for the ASan Nightly Project and the project is unblocked (because not getting symbolized traces is a hard blocker). > > ::: build/build-clang/find_symbolizer_linux.patch:22 > (Diff revision 1) > > + if (*end == '\0') break; > > + beg = end + 1; > > + } > > ++ > > ++#if SANITIZER_LINUX > > ++ internal_readlink("/proc/self/exe", buffer.data(), kMaxPathLength); > > Can this call fail? I know some people run without `/proc` mounted... We > should either handle failure here or use something that can handle failure > gracefully. You're right. A simple return nullptr on failure will do here, I will add this. Thanks! > > ::: build/unix/mozconfig.asan:16 > (Diff revision 1) > > +# These can be removed once bugs 1416183 and 1416185 are fixed > > +export CFLAGS="$CFLAGS -Wno-unused-private-field -Wno-deprecated-register" > > +export CXXFLAGS="$CXXFLAGS -Wno-unused-private-field -Wno-deprecated-register" > > Let's just not commit this patch until the dependent bugs are fixed, please? I disagree, unless the patch in bug 1416183 is reviewed until I'm back. Looking at comments like https://bugzilla.mozilla.org/show_bug.cgi?id=1416183#c4 it seems that fixing these is more effort. I am not willing to block such an important project (and the work on bug 1404860) on such kind of refactorings. I want this project to move forward as quickly as possible and since we are only working on the ASan builds here (and not any kind of production/release builds), I think that's a fair requirement.

(In reply to Christian Holler (:decoder) from comment #5) > (In reply to Nathan Froyd [:froydnj] from comment #4) > > ::: build/unix/mozconfig.asan:16 > > (Diff revision 1) > > > +# These can be removed once bugs 1416183 and 1416185 are fixed > > > +export CFLAGS="$CFLAGS -Wno-unused-private-field -Wno-deprecated-register" > > > +export CXXFLAGS="$CXXFLAGS -Wno-unused-private-field -Wno-deprecated-register" > > > > Let's just not commit this patch until the dependent bugs are fixed, please? > > I disagree, unless the patch in bug 1416183 is reviewed until I'm back. > Looking at comments like > https://bugzilla.mozilla.org/show_bug.cgi?id=1416183#c4 it seems that fixing > these is more effort. I am not willing to block such an important project > (and the work on bug 1404860) on such kind of refactorings. I want this > project to move forward as quickly as possible and since we are only working > on the ASan builds here (and not any kind of production/release builds), I > think that's a fair requirement. It looks like both of these are moving forward quickly enough that removing the suggested block of code is reasonable.

(In reply to Christian Holler (:decoder) from comment #7) > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/199104/diff/1-2/ Confirmed that no more warnings are popping up after the warning fixes from the bug dependencies here, so I removed the -Wno flags as requested. I also added comments both inside the patch code as well as on the top of the patch, explaining why we are patching this right now. We will have to see then if we stick to this patch or trade it for another solution later. As for landing, we still have to wait for bug 1415782 to be fixed or disable that test with newer Clang if a fix is not feasible in time.

Comment on attachment 8927809 [details] Bug 1415689 - Add Clang 6 (pre) and use it for ASan builds. https://reviewboard.mozilla.org/r/199104/#review205706 WFM, thank you for testing with the flags removed!

Pushed by choller@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8d6fd02420f2 Add Clang 6 (pre) and use it for ASan builds. r=froydnj

Backout by aiakab@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0fd137f9b3fe Backed out changeset 8d6fd02420f2 for failing gtests on request from "decoder" on a CLOSED TREE

Pushed by choller@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bf902590846a Add Clang 6 (pre) and use it for ASan builds. r=froydnj