RESOLVED FIXED 176186
Allow classes to have modern and legacy decoders to aid transition
https://bugs.webkit.org/show_bug.cgi?id=176186
Summary Allow classes to have modern and legacy decoders to aid transition
Alex Christensen
Reported 2017-08-31 14:36:30 PDT
Allow classes to have modern and legacy decoders to aid transition
Attachments
Patch (7.25 KB, patch)
2017-08-31 14:39 PDT, Alex Christensen
no flags
Proposed Linux fixes (3.95 KB, patch)
2017-09-02 06:14 PDT, Zan Dobersek
no flags
Patch (7.80 KB, patch)
2017-09-04 21:53 PDT, Alex Christensen
no flags
Alex Christensen
Comment 1 2017-08-31 14:39:37 PDT
Alex Christensen
Comment 2 2017-08-31 18:48:25 PDT
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
Comment 3 2017-09-01 03:27:39 PDT
> ../../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
Comment 4 2017-09-01 09:51:54 PDT
Well, I would like to commit this change so we can move forward. Could someone on Linux find a workaround?
Michael Catanzaro
Comment 5 2017-09-01 12:28:58 PDT
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
Comment 6 2017-09-01 12:37:13 PDT
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
Comment 7 2017-09-02 06:14:49 PDT
Created attachment 319714 [details] Proposed Linux fixes
Zan Dobersek
Comment 8 2017-09-02 06:34:26 PDT
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
Comment 9 2017-09-04 21:53:33 PDT
Alex Christensen
Comment 10 2017-09-04 22:03:19 PDT
Thanks, Zan! This is definitely one of the more interesting compiler quirks I've ever seen.
Alex Christensen
Comment 11 2017-09-05 10:33:25 PDT
Radar WebKit Bug Importer
Comment 12 2017-09-27 12:47:02 PDT
Note You need to log in before you can comment on or make changes to this bug.