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)

x86_64
Windows 8
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla32

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — 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 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-
Blocks: winclang
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
(...)
(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. :-)
Attached patch patch (obsolete) — Splinter Review
Attachment #8435405 - Attachment is obsolete: true
Attachment #8436188 - Flags: review?(mh+mozilla)
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+
Attached patch patchSplinter Review
Attachment #8436188 - Attachment is obsolete: true
Attachment #8436205 - Flags: review?(mh+mozilla)
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)
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.
I'd rather add -D_HAS_EXCEPTIONS=0 to CXXFLAGS, then.
Flags: needinfo?(mh+mozilla)
(instead of STL_FLAGS, that is)
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+
(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.
(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...
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)
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?
(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)
(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
https://hg.mozilla.org/mozilla-central/rev/57f5fad40670
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
This needs to be uplifted to Aurora
(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 → ---
(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 ago10 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: