1246334 - Add support for building the clang plugin using clang-cl

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[(no longer active)]

Attachment: Add support for building the clang plugin using clang-cl

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8716567 [details] [diff] [review]
Add support for building the clang plugin using clang-cl

Review of attachment 8716567 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/clang-plugin.m4
@@ +15,5 @@
>  
>      AC_MSG_CHECKING([for llvm-config])
>      if test -z "$LLVMCONFIG"; then
> +      if test -n "$CLANG_CL"; then
> +          CXX_COMPILER="${CXX/clang-cl/clang}"

This is a bashism (a shell construct that only works in bash), which should be avoided. It also has the bad property of replacing "clang-cl" anywhere in CXX, so if for some reason, the path contains clang-cl, it will also be replaced... So you'd need to only replace the last one. "$(dirname "$CXX")/clang" ?

@@ +41,5 @@
>      dnl The clang package we use on OSX is old, and its llvm-config doesn't
>      dnl recognize --system-libs, so ask for that separately.  llvm-config's
>      dnl failure here is benign, so we can ignore it if it happens.
> +    LLVM_LDFLAGS=`$LLVMCONFIG --system-libs | tr '\n' ' '`
> +    LLVM_LDFLAGS="$LLVM_LDFLAGS `$LLVMCONFIG --ldflags --libs core mc analysis asmparser mcparser bitreader option | tr '\n' ' '`"

we don't have xargs in mozillabuild? O_o

@@ +65,5 @@
> +        dnl into a dash.
> +        LLVM_REPLACE_CXXFLAGS=''
> +        for arg in $LLVM_CXXFLAGS; do
> +            dnl The following expression replaces a leading slash with a dash.
> +            arg="${arg/#\//-} "

bashism

@@ +67,5 @@
> +        for arg in $LLVM_CXXFLAGS; do
> +            dnl The following expression replaces a leading slash with a dash.
> +            arg="${arg/#\//-} "
> +            dnl Also replace any backslashes with forward slash.
> +            arg="${arg//\\//}"

bashism

@@ +68,5 @@
> +            dnl The following expression replaces a leading slash with a dash.
> +            arg="${arg/#\//-} "
> +            dnl Also replace any backslashes with forward slash.
> +            arg="${arg//\\//}"
> +            LLVM_REPLACE_CXXFLAGS+="$arg"

bashism

You should probably postprocess the llvm-config calls with sed. Something like sed 's,\(^\|\s\)/,\1-,g;s,\\,/,g' (untested).

::: config/static-checking-config.mk
@@ +5,5 @@
>  # The entire tree should be subject to static analysis using the XPCOM
>  # script. Additional scripts may be added by specific subdirectories.
>  
>  ifdef ENABLE_CLANG_PLUGIN
> +CLANG_PLUGIN := $(abspath $(DEPTH)/build/clang-plugin/$(DLL_PREFIX)clang-plugin$(DLL_SUFFIX))

Please use $(topobjdir) instead of $(DEPTH), and remove $(abspath).

Comment 3

[(no longer active)]

(In reply to Mike Hommey [:glandium] from comment #2)
> @@ +41,5 @@
> >      dnl The clang package we use on OSX is old, and its llvm-config doesn't
> >      dnl recognize --system-libs, so ask for that separately.  llvm-config's
> >      dnl failure here is benign, so we can ignore it if it happens.
> > +    LLVM_LDFLAGS=`$LLVMCONFIG --system-libs | tr '\n' ' '`
> > +    LLVM_LDFLAGS="$LLVM_LDFLAGS `$LLVMCONFIG --ldflags --libs core mc analysis asmparser mcparser bitreader option | tr '\n' ' '`"
> 
> we don't have xargs in mozillabuild? O_o

The issue is not that we don't have xargs, but xargs takes something like "-Lc:\path\to\llvm/lib" (which llvm-config outputs) and turns it into: "-Lc:pathtollvm/lib".  Using tr avoids having to dance around path separators here.  I'll add a comment explaining this.

Comment 4

[(no longer active)]

Attachment: Add support for building the clang plugin using clang-cl

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8723582 [details] [diff] [review]
Add support for building the clang plugin using clang-cl

Review of attachment 8723582 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/clang-plugin.m4
@@ +68,5 @@
> +        for arg in $LLVM_CXXFLAGS; do
> +            dnl The following expression replaces a leading slash with a dash.
> +            arg=`echo "$arg "|sed -e 's/^\//-/'`
> +            dnl Also replace any backslashes with forward slash.
> +            arg=`echo "$arg"|sed -e 's/\\\\/\//g'`

You can do echo "$arg" | sed -e 's/^\//-/' -e 's/\\\\/\//g' and avoid running sed twice as many times as necessary.
(btw, that seems like a lot of backslashes, is llvm-config output double backslashes?)

Comment 6

[(no longer active)]

(In reply to Mike Hommey [:glandium] from comment #5)
> (btw, that seems like a lot of backslashes, is llvm-config output double
> backslashes?)

No.  I also don't know why I need 4...  With just two, I get this sed error:

sed: -e expression #1, char 8: unterminated `s' command

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb43c67f8903

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cb43c67f8903