Closed Bug 1425406 Opened 8 years ago Closed 8 years ago

Mac cross-compile toolchain doesn't include native sanitizer dylibs.

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Platform:
x86_64
Unspecified
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox59 wontfix, firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: truber, Assigned: truber)

References

Details

Attachments

(1 file)

Bug 1425406 - Add a linux64 clang 6 (pre) toolchain with the macosx64 native sanitizer dylibs.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
We need a linux64 native clang toolchain which can cross-compile for macosx64 and has the sanitizer dylibs for AddressSanitizer. Until we can do this in a single build with cmake, I am creating a hybrid toolchain which builds the native macosx toolchain, and then overlays it with the native linux toolchain prior to packaging (with the exception of llvm-symbolizer, which must be macosx native).
I'm not sure this is a great idea. And by this I mean, except if you do some targeted building of the native macosx toolchain and only build the part(s) you need, this is not going to be great wrt delays when clang changes land. Especially if you change the current default clang build to do that, because you would be delaying everything else. It's already painful when we land clang changes, let's not make it worse. OTOH, we're *already* building a native macosx clang. Why not create a toolchain job that depends on both linux and macosx clang, and just combines them? There's a problem to overcome that mach artifact toolchain will not be happy to download two toolchains with the same name, but that can be tweaked easily by just invoking mach artifact toolchain twice, once for each, in different directories. I can even volunteer to write such a job (because I know exactly how I'd do it, and it wouldn't take long to implement).
The current native clang is 3.9, which does not work for asan builds at present. The patch I've submitted is for clang 6-pre which matches current linux builds in taskcluster. I intended this as a stopgap to get mac asan builds up (I have this working on try) until we can build a proper multilib clang with both linux and mac libs in one build, which should be possible. That said, it would not be hard to switch this to a native macosx clang 6-pre build to support your hybrid job if that is an acceptable long-term solution.
Mmmm there might not be much to gain from having a native macosx clang-6-pre build, so I guess your approach works out. Could you detail why you need the patches you add? For instance, I remember I had to disable TSAN when I built 3.8 on a mac slave loaner natively, but that was because the slave loaner had old versions of many things, and we can build 3.9 without disabling TSAN, so why do you need to disable it?
Yes, TSAN is broken because it uses DISPATCH_NOESCAPE which doesn't exist in our version of the SDK. This was added recently, https://reviews.llvm.org/D38141. I didn't bother tracking this down previously, but I just tried reversing the above patch instead of disabling TSAN and it looks like it works. rforbes is going to follow up with an LLVM bug and I'll update the review here to reverse the patch for now.
I filed bug 1421755 on improving the clang build script to use llvm's new fancy CMake stuff which should let us do this the right way, which would build both sets of target libs in a single build. There are details in the bug. Hopefully that will work for us as a well-supported long-term solution.
Comment on attachment 8936995 [details] Bug 1425406 - Add a linux64 clang 6 (pre) toolchain with the macosx64 native sanitizer dylibs. https://reviewboard.mozilla.org/r/207736/#review213690 I am going to cancel this review for now because offline conversations indicate changes are coming; please r? when those have been done or to tell me that I'm completely off-base.
Attachment #8936995 - Flags: review?(nfroyd)
Attachment #8936995 - Flags: review?(nfroyd) → review?(ted)
Comment on attachment 8936995 [details] Bug 1425406 - Add a linux64 clang 6 (pre) toolchain with the macosx64 native sanitizer dylibs. https://reviewboard.mozilla.org/r/207736/#review224924 I guess this is the least bad option, though I would like to see if bug 1421755 could be made to work. ::: build/build-clang/clang-6-pre-macosx64.json:25 (Diff revision 5) > + "libtool": "/builds/worker/workspace/build/src/cctools/bin/x86_64-apple-darwin11-libtool", > + "ld": "/builds/worker/workspace/build/src/clang/bin/clang", > + "patches": [ > + "compiler-rt-cross-compile.patch", > + "compiler-rt-disable-codesign.patch", > + "r313929-R.patch" Surely we could come up with a better name than this? ::: build/build-clang/r313929-R.patch:9 (Diff revision 5) > +- TSAN_INTERCEPTOR(void, name, dispatch_queue_t q, \ > +- DISPATCH_NOESCAPE dispatch_block_t block) { \ > ++ TSAN_INTERCEPTOR(void, name, dispatch_queue_t q, dispatch_block_t block) { \ Why are we eliminating `DISPATCH_NOESCAPE` here and in other places below? Do we not have it available in the cross headers or something? Can we write a comment to that effect at the top of the patch file, similarly to compiler-rt-disable-codesign.patch? ::: taskcluster/scripts/misc/build-clang-6-pre-linux-macosx-cross.sh:4 (Diff revision 5) > +#!/bin/bash > +set -x -e -v > + > +# This script is for building clang for Linux with Mac OS X Compiler-RT. Can we be a little bit more precise here? I think this really means "clang for a Linux host but that also includes native OS X compiler-rt libraries". Is that correct?
Attachment #8936995 - Flags: review?(nfroyd) → review+
Comment on attachment 8936995 [details] Bug 1425406 - Add a linux64 clang 6 (pre) toolchain with the macosx64 native sanitizer dylibs. https://reviewboard.mozilla.org/r/207736/#review224924 > Surely we could come up with a better name than this? I was following the naming convention of using LLVM SVN revisions, only this was reversing that change (-R). This has been fixed now upstream since I originally wrote this in December (https://reviews.llvm.org/rL321543). I've updated to use that patch instead, which includes an explanation about DISPATCH_NOESCAPE. > Why are we eliminating `DISPATCH_NOESCAPE` here and in other places below? Do we not have it available in the cross headers or something? Can we write a comment to that effect at the top of the patch file, similarly to compiler-rt-disable-codesign.patch? Our SDK version doesn't have DISPATCH_NOESCAPE, but compiler-rt assumed it existed (until r321543 fixed it). > Can we be a little bit more precise here? I think this really means "clang for a Linux host but that also includes native OS X compiler-rt libraries". Is that correct? Yes, thanks for catching this very awkward sentence :)
Blocks: 1421728
See Also: → 1421755
Pushed by nerli@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b0deb6af3dfb Add a linux64 clang 6 (pre) toolchain with the macosx64 native sanitizer dylibs. r=froydnj
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b0deb6af3dfb
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: