WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
130260
[GTK][CMake] Add support for building with Clang
https://bugs.webkit.org/show_bug.cgi?id=130260
Summary
[GTK][CMake] Add support for building with Clang
Zan Dobersek
Reported
2014-03-14 13:37:18 PDT
[GTK][CMake] Add support for building with Clang
Attachments
Patch
(6.80 KB, patch)
2014-03-14 13:50 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch
(6.41 KB, patch)
2014-03-17 10:09 PDT
,
Zan Dobersek
mrobinson
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Zan Dobersek
Comment 1
2014-03-14 13:50:17 PDT
Created
attachment 226761
[details]
Patch
Martin Robinson
Comment 2
2014-03-14 14:15:20 PDT
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 3
2014-03-14 14:37:24 PDT
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 4
2014-03-15 07:29:08 PDT
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
Comment 5
2014-03-15 10:04:28 PDT
(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)
Comment 6
2014-03-16 07:01:50 PDT
(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
Comment 7
2014-03-17 09:47:27 PDT
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
Comment 8
2014-03-17 10:09:22 PDT
Created
attachment 226933
[details]
Patch
Raphael Kubo da Costa (:rakuco)
Comment 9
2014-03-18 06:42:00 PDT
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 10
2014-03-18 13:43:47 PDT
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 11
2014-03-18 13:46:11 PDT
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
Comment 12
2014-03-20 11:23:29 PDT
Committed
r165975
: <
http://trac.webkit.org/changeset/165975
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug