Closed
Bug 1021378
Opened 10 years ago
Closed 10 years ago
Add support for clang-cl to the build system
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla32
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
Details
Attachments
(1 file, 2 obsolete files)
8.93 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
This patch allows us to start building with clang-cl. The credit partially goes to Jeff.
Attachment #8435405 -
Flags: review?(mh+mozilla)
Comment 1•10 years ago
|
||
Comment on attachment 8435405 [details] [diff] [review] patch Review of attachment 8435405 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/autoconf/toolchain.m4 @@ +73,3 @@ > fi > AC_SUBST(CLANG_CXX) > +AC_SUBST(CLANG_CL) This whole block is more and more a pile of garbage. We should really just rely on the preprocessor on an input file like: #if defined(__clang__) COMPILER clang __clang_major__.__clang_minor__.__clang_patchlevel__ #elif defined(__GNUC__) COMPILER gcc __GNUC__.__GNUC_MINOR__.__GNUC_PATCHLEVEL___ #elif defined(_MSC_VER) COMPILER msvc _MSC_VER #elif defined(__INTEL_COMPILER) COMPILER icc __INTEL_COMPILER #endif and grep the output on COMPILER. I guess clang-cl doesn't set __clang__ ? If it does, put _MSC_VER first. Can be done with something like this: cat <<EOF > conftest.c #if ... EOF read compiler version <<EOF $($(CC) -E conftest.c 2>/dev/null | grep COMPILER) EOF From there, you can use $compiler and $version to act accordingly. The content of $version would be better to test than whatever we're getting out of cl's version output for the MSVC version tests in configure.in. Useful resource: http://nadeausoftware.com/articles/2012/10/c_c_tip_how_detect_compiler_name_and_version_using_compiler_predefined_macros ::: config/config.mk @@ +289,5 @@ > else # ! MOZ_DEBUG > > +ifdef CLANG_CL > +OS_CFLAGS += -D_HAS_EXCEPTIONS=0 > +OS_CXXFLAGS += -D_HAS_EXCEPTIONS=0 This should be set by virtue of http://hg.mozilla.org/mozilla-central/file/5e8e9d864a4d/configure.in#l619 If it's not, then it means it also doesn't wrap stl headers, and I don't think we want that to happen. ::: configure.in @@ +911,4 @@ > > AC_MSG_CHECKING([compiler version]) > # Just print it so it shows up in the logs. > +cc_version=$($CC -v) Here's the output of gcc -v on my machine: Using built-in specs. COLLECT_GCC=gcc COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/4.8/lto-wrapper Target: x86_64-linux-gnu Configured with: ../src/configure -v --with-pkgversion='Debian 4.8.2-21' --with-bugurl=file:///usr/share/doc/gcc-4.8/README.Bugs --enable-languages=c,c++,java,go,d,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-4.8 --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.8 --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --enable-gnu-unique-object --disable-libmudflap --enable-plugin --with-system-zlib --disable-browser-plugin --enable-java-awt=gtk --enable-gtk-cairo --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-4.8-amd64/jre --enable-java-home --with-jvm-root-dir=/usr/lib/jvm/java-1.5.0-gcj-4.8-amd64 --with-jvm-jar-dir=/usr/lib/jvm-exports/java-1.5.0-gcj-4.8-amd64 --with-arch-directory=amd64 --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --enable-objc-gc --enable-multiarch --with-arch-32=i586 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --with-tune=generic --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu Thread model: posix gcc version 4.8.2 (Debian 4.8.2-21) Compare with gcc --version: gcc (Debian 4.8.2-21) 4.8.2 Copyright (C) 2013 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. clang -v: Debian clang version 3.4-1 (tags/RELEASE_34/final) (based on LLVM 3.4) Target: x86_64-pc-linux-gnu Thread model: posix Found candidate GCC installation: /usr/bin/../lib/gcc/i486-linux-gnu/4.8 Found candidate GCC installation: /usr/bin/../lib/gcc/i486-linux-gnu/4.8.2 Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/4.7 Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/4.7.3 Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8 Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8.2 Found candidate GCC installation: /usr/lib/gcc/i486-linux-gnu/4.8 Found candidate GCC installation: /usr/lib/gcc/i486-linux-gnu/4.8.2 Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.7 Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.7.3 Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.8 Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.8.2 Selected GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8 clang --version: Debian clang version 3.4-1 (tags/RELEASE_34/final) (based on LLVM 3.4) Target: x86_64-pc-linux-gnu Thread model: posix (in both cases, -v sends to stderr, and --version to stdout, fwiw) I'd rather output nothing. ::: js/src/configure.in @@ +1686,5 @@ > MOZ_OPTIMIZE_FLAGS="-O2" > MOZ_FIX_LINK_PATHS= > MOZ_COMPONENT_NSPR_LIBS='$(NSPR_LIBS)' > + if test -z "$CLANG_CL"; then > + LDFLAGS="$LDFLAGS -LARGEADDRESSAWARE -NXCOMPAT" Does clang-cl exit with an error when those flags are used? If it does, I'd rather add checks whether the flags throw an error or not. That would have the nice side effect of adding the flags when clang-cl finally supports them. @@ +1768,5 @@ > LDFLAGS="$LDFLAGS -Wl,--large-address-aware" > else > DSO_LDOPTS="$DSO_LDOPTS -MACHINE:X86" > + if test -z "$CLANG_CL"; then > + LDFLAGS="$LDFLAGS -SAFESEH" Likewise.
Attachment #8435405 -
Flags: review?(mh+mozilla) → review-
Comment 2•10 years ago
|
||
BTW, doing something else, I was seeing weird messages during configure[1]. Turns out they are due to the $CC --version thing... because it's just plain wrong: cc_version=$($CC --version) What that actually does is take $CC --version's stdout, and use that as a command whose output is then put in cc_version. So it can just double die. 1. checking compiler version... Microsoft (R) 32-bit C/C++ Optimizing Compiler Version 16.00.30319.01 for 80x86 Copyright (C) Microsoft Corporation. All rights reserved. cl : Command line warning D9002 : ignoring unknown option '--version' cl : Command line error D8003 : missing source filename checking for c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/mozmake.exe... c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/mozilla/configure: line 5398: test: /c/Program Files (x86)/Windows Kits/8.0/bin/x86/c: binary operator expected c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/mozilla/configure: line 5398: test: /c/PROGRA~2/MICROS~2.0/Common7/IDE/c: binary operator expected c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/mozilla/configure: line 5398: test: /c/PROGRA~2/MICROS~2.0/VC/BIN/c: binary operator expected (...)
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #2) > BTW, doing something else, I was seeing weird messages during configure[1]. > Turns out they are due to the $CC --version thing... because it's just plain > wrong: > > cc_version=$($CC --version) > > What that actually does is take $CC --version's stdout, and use that as a > command whose output is then put in cc_version. So it can just double die. yeah, this is what I was trying to mention on IRC yesterday. :-)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8435405 -
Attachment is obsolete: true
Attachment #8436188 -
Flags: review?(mh+mozilla)
Comment 5•10 years ago
|
||
Comment on attachment 8436188 [details] [diff] [review] patch Review of attachment 8436188 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/autoconf/toolchain.m4 @@ +28,5 @@ > +read dummy compiler version <<EOF > +$($CC -E conftest.c 2>/dev/null | grep COMPILER) > +EOF > +CC_VERSION="$version" > +CXX_VERSION="$version" For completeness, you should make that derived from running $CXX -E and check that both $compiler's are equal. @@ +36,2 @@ > changequote(<<,>>) > GCC_VERSION_FULL=`echo "$CXX_VERSION" | $PERL -pe 's/^.*gcc version ([^ ]*).*/<<$>>1/'` That's obviously going to be broken ::: configure.in @@ +470,1 @@ > _MSC_VER=${_CC_MAJOR_VERSION}${_CC_MINOR_VERSION} _MSC_VER=${CC_VERSION} @@ +493,5 @@ > AC_SUBST(MSVS_VERSION) > > + if test -z "$CLANG_CL"; then > + AC_DEFINE(HAVE_SEH_EXCEPTIONS) > + fi Can you add a comment as to why this is disabled (same for LDFLAGS)
Attachment #8436188 -
Flags: review?(mh+mozilla) → feedback+
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8436188 -
Attachment is obsolete: true
Attachment #8436205 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 7•10 years ago
|
||
So here's a question. http://mxr.mozilla.org/mozilla-central/source/config/config.mk#596 only uses STL_FLAGS if DISABLE_STL_WRAPPING is not turned on in moz.build. This means that the directories which have that won't get -D_HAS_EXCEPTIONS even with this patch. So we need to do something on top of that, right?
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 8•10 years ago
|
||
Just double-checked, and -D_HAS_EXCEPTION=0 is not passed in such directories to cl either, cl gives you a warning instead of an error, like "warning C4530: C++ exception handler used, but unwind semantics are not enabled. Specify /EHsc". So I guess we need to resurrect that hunk from config.mk in my first patch. If you agree I'll do that when landing.
Comment 9•10 years ago
|
||
I'd rather add -D_HAS_EXCEPTIONS=0 to CXXFLAGS, then.
Flags: needinfo?(mh+mozilla)
Comment 10•10 years ago
|
||
(instead of STL_FLAGS, that is)
Comment 11•10 years ago
|
||
Comment on attachment 8436205 [details] [diff] [review] patch Review of attachment 8436205 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/autoconf/toolchain.m4 @@ +24,5 @@ > +#elif defined(__INTEL_COMPILER) > +COMPILER icc __INTEL_COMPILER > +#endif > +EOF > +read dummy compiler version <<EOF Note, you could read CC_VERSION here directly. Same for CXX_VERSION. Avoids a useless intermediate variable.
Attachment #8436205 -
Flags: review?(mh+mozilla) → review+
Comment 12•10 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #9) > I'd rather add -D_HAS_EXCEPTIONS=0 to CXXFLAGS, then. BTW, I wonder if -D_HAS_EXCEPTIONS=0 shouldn't be in CFLAGS for msvc too. Since MSVC likes its mixture of C and C++, I wonder if it doesn't do exceptions in C too.
Assignee | ||
Comment 13•10 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #12) > (In reply to Mike Hommey [:glandium] from comment #9) > > I'd rather add -D_HAS_EXCEPTIONS=0 to CXXFLAGS, then. > > BTW, I wonder if -D_HAS_EXCEPTIONS=0 shouldn't be in CFLAGS for msvc too. I think it should. Avoids the warning in all of the aforementioned directories. > Since MSVC likes its mixture of C and C++, I wonder if it doesn't do > exceptions in C too. MSVC supports SEH in C IIRC...
Assignee | ||
Comment 14•10 years ago
|
||
Hmm, I'm getting b2g-ics-emu failures on try which I cannot reproduce locally. Any ideas what might be causing this?
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 15•10 years ago
|
||
Ah, I can finally reproduce. On this freaking compiler, __GNUC__.__GNUC_MINOR__.__GNUC_PATCHLEVEL__ expands to "4. 4. 3". Can I land this patch without the cleanup requested in comment 1?
Assignee | ||
Comment 16•10 years ago
|
||
(https://tbpl.mozilla.org/?tree=Try&rev=4f1e5f45bb57)
Comment 17•10 years ago
|
||
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #15) > Ah, I can finally reproduce. On this freaking compiler, > __GNUC__.__GNUC_MINOR__.__GNUC_PATCHLEVEL__ expands to "4. 4. 3". Can I > land this patch without the cleanup requested in comment 1? That's actually the case for any version of gcc and clang. Just filter the version with sed 's/ //g'
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 18•10 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #17) > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment > #15) > > Ah, I can finally reproduce. On this freaking compiler, > > __GNUC__.__GNUC_MINOR__.__GNUC_PATCHLEVEL__ expands to "4. 4. 3". Can I > > land this patch without the cleanup requested in comment 1? > > That's actually the case for any version of gcc and clang. Just filter the > version with sed 's/ //g' Oh right, that fixes the issue. https://tbpl.mozilla.org/?tree=Try&rev=98475b783959
Assignee | ||
Comment 19•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/57f5fad40670
Comment 20•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/57f5fad40670
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment 22•10 years ago
|
||
This needs to be uplifted to Aurora
Comment 23•10 years ago
|
||
(In reply to Zac C (:zac) from comment #22) > This needs to be uplifted to Aurora Just to clarify this comment, we need to uplift the following changeset to Aurora: https://hg.mozilla.org/mozilla-central/rev/d91acbddb062
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 24•10 years ago
|
||
(In reply to Rail Aliiev [:rail] from comment #23) > (In reply to Zac C (:zac) from comment #22) > > This needs to be uplifted to Aurora > > Just to clarify this comment, we need to uplift the following changeset to > Aurora: > https://hg.mozilla.org/mozilla-central/rev/d91acbddb062 https://hg.mozilla.org/releases/mozilla-aurora/rev/f0f5c6b5d33c
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•