Bug 90055 – Fix gcc build after r121302

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ryosuke Niwa]

Fix gcc build after r121302

[Ryosuke Niwa]

Created attachment 149700 [details]
Patch

[Ryosuke Niwa]

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 :(

[Mark Rowe (bdash)]

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.

[Ryosuke Niwa]

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?

[Mark Rowe (bdash)]

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.

[Ryosuke Niwa]

(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.

[Mark Rowe (bdash)]

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.

[Ryosuke Niwa]

Created attachment 149702 [details]
Fix clang build

[Ryosuke Niwa]

(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.

[Mark Rowe (bdash)]

(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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

(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).

[Mark Rowe (bdash)]

Sorry, it'll also need to go in Tools/TestWebKitAPI/Configurations/Base.xcconfig, same as the setting of GTEST_HAS_TR1_TUPLE.

[Ryosuke Niwa]

Created attachment 149708 [details]
Patch

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Committed r121331: <http://trac.webkit.org/changeset/121331>