Bug 90055 - Fix gcc build after r121302
Summary: Fix gcc build after r121302
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Blocker
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-27 00:56 PDT by Ryosuke Niwa
Modified: 2012-06-27 02:15 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.99 KB, patch)
2012-06-27 00:58 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fix clang build (4.15 KB, patch)
2012-06-27 01:18 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (5.43 KB, patch)
2012-06-27 01:57 PDT, Ryosuke Niwa
mrowe: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-06-27 00:56:14 PDT
Fix gcc build after r121302
Comment 1 Ryosuke Niwa 2012-06-27 00:58:31 PDT
Created attachment 149700 [details]
Patch
Comment 2 Ryosuke Niwa 2012-06-27 01:02:30 PDT
I don't know what they were thinking to assume RTTI is enabled for very old versions of gcc. They clearly didn't test this code with old gcc :(
Comment 3 Mark Rowe (bdash) 2012-06-27 01:08:39 PDT
Comment on attachment 149700 [details]
Patch

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

> Source/ThirdParty/gtest/include/gtest/internal/gtest-port.h:349
> +#elif defined(__clang__) || defined(__llvm__) // Assume RTTO is always enabled on LLVM
> +
> +#define GTEST_HAS_RTTI 1
> +

It's not correct to assume that RTTI is enabled. Doing this will break the clang build in the same way that GCC is currently broken.
Comment 4 Ryosuke Niwa 2012-06-27 01:09:24 PDT
Comment on attachment 149700 [details]
Patch

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

>> Source/ThirdParty/gtest/include/gtest/internal/gtest-port.h:349
>> +
> 
> It's not correct to assume that RTTI is enabled. Doing this will break the clang build in the same way that GCC is currently broken.

Does clang also defines __GXX_RTTI flag?
Comment 5 Mark Rowe (bdash) 2012-06-27 01:13:36 PDT
Clang defines it when the -fno-rtti flag is not passed. Note that we build TestWebKitAPI with -fno-rtti for both clang and gcc. I'm not quite sure which code path in gtest-port.h clang currently hits though.
Comment 6 Ryosuke Niwa 2012-06-27 01:17:53 PDT
(In reply to comment #5)
> Clang defines it when the -fno-rtti flag is not passed. Note that we build TestWebKitAPI with -fno-rtti for both clang and gcc. I'm not quite sure which code path in gtest-port.h clang currently hits though.

I see some clang support in http://googletest.googlecode.com/svn/trunk/include/gtest/internal/gtest-port.h so I'll copy it over and tweak it to support old gcc.
Comment 7 Mark Rowe (bdash) 2012-06-27 01:18:12 PDT
The cleanest solution is probably to just set GTEST_HAS_RTTI=0 in GCC_PREPROCESSOR_DEFINITIONS in General.xcconfig. That'll tell gtest not to use RTTI with any of our compilers when building on OS X.
Comment 8 Ryosuke Niwa 2012-06-27 01:18:17 PDT
Created attachment 149702 [details]
Fix clang build
Comment 9 Ryosuke Niwa 2012-06-27 01:21:29 PDT
(In reply to comment #7)
> The cleanest solution is probably to just set GTEST_HAS_RTTI=0 in GCC_PREPROCESSOR_DEFINITIONS in General.xcconfig. That'll tell gtest not to use RTTI with any of our compilers when building on OS X.

I guess I can do that if you don't like my new patch.
Comment 10 Mark Rowe (bdash) 2012-06-27 01:22:41 PDT
(In reply to comment #9)
> (In reply to comment #7)
> > The cleanest solution is probably to just set GTEST_HAS_RTTI=0 in GCC_PREPROCESSOR_DEFINITIONS in General.xcconfig. That'll tell gtest not to use RTTI with any of our compilers when building on OS X.
> 
> I guess I can do that if you don't like my new patch.

I think I'd prefer a tweak to the configuration settings than tweaking a header in a non-obvious way that would be very likely to get undone if we pull in a new version of gtest.
Comment 11 Ryosuke Niwa 2012-06-27 01:30:34 PDT
(In reply to comment #10)
> I think I'd prefer a tweak to the configuration settings than tweaking a header in a non-obvious way that would be very likely to get undone if we pull in a new version of gtest.

Okay, uploading a new patch in a minute.
Comment 12 Ryosuke Niwa 2012-06-27 01:44:46 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > I think I'd prefer a tweak to the configuration settings than tweaking a header in a non-obvious way that would be very likely to get undone if we pull in a new version of gtest.
> 
> Okay, uploading a new patch in a minute.

Unfortunately, some don't seem to be able to seem to be seeing the variable I defined in General.xcconfig (ones in InjectedBundleTestWebKitAPI.build).
Comment 13 Mark Rowe (bdash) 2012-06-27 01:47:54 PDT
Sorry, it'll also need to go in Tools/TestWebKitAPI/Configurations/Base.xcconfig, same as the setting of GTEST_HAS_TR1_TUPLE.
Comment 14 Ryosuke Niwa 2012-06-27 01:57:08 PDT
Created attachment 149708 [details]
Patch
Comment 15 Ryosuke Niwa 2012-06-27 01:57:35 PDT
(In reply to comment #13)
> Sorry, it'll also need to go in Tools/TestWebKitAPI/Configurations/Base.xcconfig, same as the setting of GTEST_HAS_TR1_TUPLE.

Thanks for the help! Done.
Comment 16 Ryosuke Niwa 2012-06-27 02:15:24 PDT
Committed r121331: <http://trac.webkit.org/changeset/121331>