Bug 130260 – [GTK][CMake] Add support for building with Clang

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[GTK][CMake] Add support for building with Clang

[Zan Dobersek]

Created attachment 226761 [details]
Patch

[Martin Robinson]

Comment on attachment 226761 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226761&action=review

> Source/cmake/OptionsCommon.cmake:37
> +    set(CMAKE_CXX_FLAGS  "${CMAKE_CXX_FLAGS} -stdlib=libc++ -Qunused-arguments")

CMake + gcc issues warnings about unused arguments, so I don't think we want this to be different with clang.

[Zan Dobersek]

Comment on attachment 226761 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226761&action=review

>> Source/cmake/OptionsCommon.cmake:37
>> +    set(CMAKE_CXX_FLAGS  "${CMAKE_CXX_FLAGS} -stdlib=libc++ -Qunused-arguments")
> 
> CMake + gcc issues warnings about unused arguments, so I don't think we want this to be different with clang.

I believe you're thinking of -Wunused-arguments. -Qunused-arguments suppresses any arguments that were unused by the Clang driver, not unused arguments in code.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 226761 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226761&action=review

> Source/cmake/OptionsCommon.cmake:37
> +    set(CMAKE_CXX_FLAGS  "${CMAKE_CXX_FLAGS} -stdlib=libc++ -Qunused-arguments")

Is there a particular reason for forcing libc++?

[Zan Dobersek]

(In reply to comment #4)
> (From update of attachment 226761 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226761&action=review
> 
> > Source/cmake/OptionsCommon.cmake:37
> > +    set(CMAKE_CXX_FLAGS  "${CMAKE_CXX_FLAGS} -stdlib=libc++ -Qunused-arguments")
> 
> Is there a particular reason for forcing libc++?

It's best optimized for Clang much like libstdc++ is best optimized for GCC. For instance, when benchmarking changes in bug #128995 Clang with libc++ gave better numbers than Clang with libstdc++.

The current WebKit code also has a few tender places where build errors occur when using Clang and libstdc++ 4.7 or 4.8.0 (but not 4.8.1 which is C++11-complete). There are no macro-based mechanisms that are oriented towards differentiating the code for the specific standard library in use, unlike the ones that enable guarding compiler-specific code.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #5)
> (In reply to comment #4)
> > Is there a particular reason for forcing libc++?
>
> It's best optimized for Clang much like libstdc++ is best optimized for GCC. For instance, when benchmarking changes in bug #128995 Clang with libc++ gave better numbers than Clang with libstdc++.

Still, forcing a certain standard library is a problem for downstreams: not all people who may want to try clang also have libc++ installed, and even if they do but libstdc++ is used by default there are going to be crashes if people then mix the WebKit library(ies) with the rest of their code (ie. they produce a binary that depends on both libstdc++ and libc++, which has been a source of many crashes on FreeBSD land, for example). I believe it would be more beneficial and less likely to cause headaches if users were just made aware of the performance differences caused by using different standard libraries with clang.

> The current WebKit code also has a few tender places where build errors occur when using Clang and libstdc++ 4.7 or 4.8.0 (but not 4.8.1 which is C++11-complete). There are no macro-based mechanisms that are oriented towards differentiating the code for the specific standard library in use, unlike the ones that enable guarding compiler-specific code.

The usual idiom (for checking for libc++ at least) is to include a harmless header (for example, ciso646) and check for _LIBCPP_VERSION. See http://trac.webkit.org/changeset/108174 or http://trac.webkit.org/changeset/108179, for example.

[Zan Dobersek]

OK, let's leave it up to Clang to decide what standard library it wants to use. Inspecting the 3.4 release it looks like libstdc++ is chosen before libc++ for most toolchains (including Linux), with at least FreeBSD and NetBSD being exceptions.

libc++ will still be enforceable through CXXFLAGS, of course.

(In reply to comment #6)
> > The current WebKit code also has a few tender places where build errors occur when using Clang and libstdc++ 4.7 or 4.8.0 (but not 4.8.1 which is C++11-complete). There are no macro-based mechanisms that are oriented towards differentiating the code for the specific standard library in use, unlike the ones that enable guarding compiler-specific code.
> 
> The usual idiom (for checking for libc++ at least) is to include a harmless header (for example, ciso646) and check for _LIBCPP_VERSION. See http://trac.webkit.org/changeset/108174 or http://trac.webkit.org/changeset/108179, for example.

I'll try to introduce a configure-time check for the Clang and libstdc++ combo, similar to one used in Autotools.

[Zan Dobersek]

Created attachment 226933 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 226933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226933&action=review

> Source/cmake/gtest/CMakeLists.txt:27
> +if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")

Just a nitpick/suggestion: newer versions of gtest have proper support for libc++, so maybe it would be good to add a comment indicating this could be removed once gtest is updated?

[Zan Dobersek]

Comment on attachment 226933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226933&action=review

>> Source/cmake/gtest/CMakeLists.txt:27
>> +if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
> 
> Just a nitpick/suggestion: newer versions of gtest have proper support for libc++, so maybe it would be good to add a comment indicating this could be removed once gtest is updated?

Sure, I'll add it in the next iteration or before landing.

[Martin Robinson]

Comment on attachment 226933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226933&action=review

> Source/cmake/OptionsCommon.cmake:40
> +    set(CMAKE_C_FLAGS  "${CMAKE_C_FLAGS} -Qunused-arguments")
> +    set(CMAKE_CXX_FLAGS  "${CMAKE_CXX_FLAGS} -Qunused-arguments")
> +endif ()

Small nit. There's an extra space after FLAGS in both of these.

[Zan Dobersek]

Committed r165975: <http://trac.webkit.org/changeset/165975>