Allow classes to have modern and legacy decoders to aid transition

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Allow classes to have modern and legacy decoders to aid transition

[Alex Christensen]

Created attachment 319524 [details]
Patch

[Alex Christensen]

Could someone look into the linux build failures?  It looks like a bug in gcc or something, and I'm not sure how to make a workaround.

[Zan Dobersek]

> ../../Source/WebKit/Platform/IPC/ArgumentCoder.h:61:41: fatal error: template instantiation depth exceeds maximum of 900 (use -ftemplate-depth= to increase the maximum)
> compilation terminated.

GCC bug?

[Alex Christensen]

Well, I would like to commit this change so we can move forward.  Could someone on Linux find a workaround?

[Michael Catanzaro]

Why do you think it is a compiler bug?

I can reproduce with GCC 7. Here is the error message I see:

In file included from ../../Source/WebKit/Platform/IPC/Decoder.h:28:0,
                 from ../../Source/WebKit/Platform/IPC/ArgumentCoders.h:28,
                 from ../../Source/WebKit/NetworkProcess/cache/CacheStorageEngineConnection.h:28,
                 from ../../Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:29,
                 from ../../Source/WebKit/NetworkProcess/FileAPI/NetworkBlobRegistry.cpp:30:
../../Source/WebKit/Platform/IPC/ArgumentCoder.h: In substitution of ‘template<class T> static uint8_t IPC::UsesLegacyDecoder<long unsigned int>::checkArgumentCoder<T>(IPC::UsesLegacyDecoder<long unsigned int>::Helper<bool (*)(IPC::Decoder&, long unsigned int&), (& IPC::ArgumentCoder<T>::decode)>*) [with T = long unsigned int]’:
../../Source/WebKit/Platform/IPC/ArgumentCoder.h:64:111:   required from ‘constexpr const bool IPC::UsesLegacyDecoder<long unsigned int>::value’
../../Source/WebKit/Platform/IPC/ArgumentCoder.h:95:79:   required by substitution of ‘template<class U, std::enable_if_t<IPC::UsesLegacyDecoder<U>::value, void>* <anonymous> > static bool IPC::ArgumentCoder<long unsigned int>::decode<U, <enumerator> >(IPC::Decoder&, U&) [with U = long unsigned int; std::enable_if_t<IPC::UsesLegacyDecoder<U>::value, void>* <anonymous> = <missing>]’
../../Source/WebKit/Platform/IPC/ArgumentCoder.h:61:41:   required by substitution of ‘template<class T> static uint8_t IPC::UsesLegacyDecoder<long unsigned int>::checkArgumentCoder<T>(IPC::UsesLegacyDecoder<long unsigned int>::Helper<bool (*)(IPC::Decoder&, long unsigned int&), (& IPC::ArgumentCoder<T>::decode)>*) [with T = long unsigned int]’
../../Source/WebKit/Platform/IPC/ArgumentCoder.h:64:111:   required from ‘constexpr const bool IPC::UsesLegacyDecoder<long unsigned int>::value’
../../Source/WebKit/Platform/IPC/ArgumentCoder.h:95:79:   required by substitution of ‘template<class U, std::enable_if_t<IPC::UsesLegacyDecoder<U>::value, void>* <anonymous> > static bool IPC::ArgumentCoder<long unsigned int>::decode<U, <enumerator> >(IPC::Decoder&, U&) [with U = long unsigned int; std::enable_if_t<IPC::UsesLegacyDecoder<U>::value, void>* <anonymous> = <missing>]’
../../Source/WebKit/Platform/IPC/ArgumentCoder.h:61:41:   [ skipping 889 instantiation contexts, use -ftemplate-backtrace-limit=0 to disable ]
../../Source/WebKit/Platform/IPC/ArgumentCoder.h:64:111:   required from ‘constexpr const bool IPC::UsesLegacyDecoder<long unsigned int>::value’
../../Source/WebKit/Platform/IPC/ArgumentCoder.h:95:79:   required by substitution of ‘template<class U, std::enable_if_t<IPC::UsesLegacyDecoder<U>::value, void>* <anonymous> > static bool IPC::ArgumentCoder<long unsigned int>::decode<U, <enumerator> >(IPC::Decoder&, U&) [with U = long unsigned int; std::enable_if_t<IPC::UsesLegacyDecoder<U>::value, void>* <anonymous> = <missing>]’
../../Source/WebKit/Platform/IPC/ArgumentCoder.h:61:41:   required by substitution of ‘template<class T> static uint8_t IPC::UsesLegacyDecoder<long unsigned int>::checkArgumentCoder<T>(IPC::UsesLegacyDecoder<long unsigned int>::Helper<bool (*)(IPC::Decoder&, long unsigned int&), (& IPC::ArgumentCoder<T>::decode)>*) [with T = long unsigned int]’
../../Source/WebKit/Platform/IPC/ArgumentCoder.h:64:111:   required from ‘constexpr const bool IPC::UsesLegacyDecoder<long unsigned int>::value’
../../Source/WebKit/Platform/IPC/Decoder.h:126:67:   required by substitution of ‘template<class T, std::enable_if_t<((! std::is_enum<_Tp>::value) && IPC::UsesLegacyDecoder<U>::value)>* <anonymous> > bool IPC::Decoder::decode(T&) [with T = long unsigned int; std::enable_if_t<((! std::is_enum<_Tp>::value) && IPC::UsesLegacyDecoder<U>::value)>* <anonymous> = <missing>]’
../../Source/WebKit/Platform/IPC/Decoder.h:94:26:   required from here
../../Source/WebKit/Platform/IPC/ArgumentCoder.h:61:41: fatal error: template instantiation depth exceeds maximum of 900 (use -ftemplate-depth= to increase the maximum)
     template<typename T> static uint8_t checkArgumentCoder(Helper<bool (*)(Decoder&, U&), &ArgumentCoder<T>::decode>*);
                                         ^~~~~~~~~~~~~~~~~~
compilation terminated.

[Alex Christensen]

This exact code works fine on clang.  It should not be making 889 instantiation contexts.  This is just sfinae.

The point of me cc'ing linux folks is that we would like to move forward with landing this and we request someone on linux find a workaround to get this to compile so we don't have to break the linux build.

[Zan Dobersek]

Created attachment 319714 [details]
Proposed Linux fixes

[Zan Dobersek]

Comment on attachment 319714 [details]
Proposed Linux fixes

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

> Source/WebKit/Platform/IPC/ArgumentCoder.h:92
> +    template<typename U = T, std::enable_if_t<UsesLegacyDecoder<U>::argumentCoderValue>* = nullptr>

The problem on GCC manifests because of how UsesLegacyDecoder<U>::value is computed. The checkArgumentCoder() function used in that computation tries to test the ArgumentCoder<T>::decode signature of whatever function is enabled through SFINAE, but on GCC that again requires computation of UsesLegacyDecoder<U>::value, and an infinite loop is entered until GCC breaks it by erring out of the compilation. I think UsesLegacyDecoder<String>::value wouldn't have that problem because of the added ArgumentCoder<String>::decode() overload.

This fix proposes only testing for the T::decode() signature when performing SFINAE for the generic ArgumentCoder<T>::decode() functions. This is done by adding the argumentCoderValue constexpr boolean to UsesModernDecoder and UsesLegacyDecoder classes. The helper DefaultDecoderValues class can be inherited from the UsesModernDecoder and UsesLegacyDecoder specializations that enforce specific encoders for a given type. This could be further simplified if UsesModernDecoder and UsesLegacyDecoder were merged into a single struct.

[Alex Christensen]

Created attachment 319876 [details]
Patch

[Alex Christensen]

Thanks, Zan!
This is definitely one of the more interesting compiler quirks I've ever seen.

[Alex Christensen]

http://trac.webkit.org/r221620

[Radar WebKit Bug Importer]

<rdar://problem/34693974>