This bug causes build failure for both webkitgtk-2.5.90 and trunk on FreeBSD 10 amd64. In Source/WebKit2/Shared/Network/NetworkResourceLoadParameters.cpp, line 100: encoder << maximumBufferingTime; maximumBufferingTime is a std::chrono::milliseconds. I found std::chrono::milliseconds is a long long in /usr/include/c++/v1/chrono. https://llvm.org/svn/llvm-project/libcxx/trunk/include/chrono ArgumentEncoder::encode (in Source/WebKit2/Platform/IPC/ArgumentEncoder.h) supports int32_t and int64_t. Unfortunately, neither of them are a typedef of long long (stdint.h -> sys/_types.h -> machine/_types.h -> x86/_types.h), which causes build failure because the compiler cannot find the matching function. Here is a copy of x86/_types.h: http://svn.freebsd.org/base/head/sys/x86/include/_types.h __LP64__ is defined, so int32_t is a int and int64_t is a long. The following are the error message: In file included from /home/lantw44/gnome/source/webkit-trunk/Source/WebKit2/Shared/Network/NetworkResourceLoadParameters.cpp:29: In file included from /home/lantw44/gnome/source/webkit-trunk/Source/WebKit2/Platform/IPC/ArgumentCoders.h:29: In file included from /home/lantw44/gnome/source/webkit-trunk/Source/WebKit2/Platform/IPC/ArgumentDecoder.h:29: /home/lantw44/gnome/source/webkit-trunk/Source/WebKit2/Platform/IPC/ArgumentCoder.h:37:10: error: member reference base type 'const long long' is not a structure or union t.encode(encoder); ~^~~~~~~ /home/lantw44/gnome/source/webkit-trunk/Source/WebKit2/Platform/IPC/ArgumentEncoder.h:56:27: note: in instantiation of member function 'IPC::ArgumentCoder<long long>::encode' requested here ArgumentCoder<T>::encode(*this, t); ^ /home/lantw44/gnome/source/webkit-trunk/Source/WebKit2/Platform/IPC/ArgumentEncoder.h:61:9: note: in instantiation of function template specialization 'IPC::ArgumentEncoder::encode<long long>' requested here encode(t); ^ /home/lantw44/gnome/source/webkit-trunk/Source/WebKit2/Platform/IPC/ArgumentCoders.h:110:17: note: in instantiation of function template specialization 'IPC::ArgumentEncoder::operator<<<long long>' requested here encoder << duration.count(); ^ /home/lantw44/gnome/source/webkit-trunk/Source/WebKit2/Platform/IPC/ArgumentEncoder.h:56:27: note: in instantiation of member function 'IPC::ArgumentCoder<std::__1::chrono::duration<long long, std::__1::ratio<1, 1000> > >::encode' requested here ArgumentCoder<T>::encode(*this, t); ^ /home/lantw44/gnome/source/webkit-trunk/Source/WebKit2/Platform/IPC/ArgumentEncoder.h:61:9: note: in instantiation of function template specialization 'IPC::ArgumentEncoder::encode<std::__1::chrono::duration<long long, std::__1::ratio<1, 1000> > >' requested here encode(t); ^ /home/lantw44/gnome/source/webkit-trunk/Source/WebKit2/Shared/Network/NetworkResourceLoadParameters.cpp:100:13: note: in instantiation of function template specialization 'IPC::ArgumentEncoder::operator<<<std::__1::chrono::duration<long long, std::__1::ratio<1, 1000> > >' requested here encoder << maximumBufferingTime; ^ In file included from /home/lantw44/gnome/source/webkit-trunk/Source/WebKit2/Shared/Network/NetworkResourceLoadParameters.cpp:29: In file included from /home/lantw44/gnome/source/webkit-trunk/Source/WebKit2/Platform/IPC/ArgumentCoders.h:29: In file included from /home/lantw44/gnome/source/webkit-trunk/Source/WebKit2/Platform/IPC/ArgumentDecoder.h:29: /home/lantw44/gnome/source/webkit-trunk/Source/WebKit2/Platform/IPC/ArgumentCoder.h:42:16: error: type 'long long' cannot be used prior to '::' because it has no members return T::decode(decoder, t); ^ /home/lantw44/gnome/source/webkit-trunk/Source/WebKit2/Platform/IPC/ArgumentDecoder.h:89:34: note: in instantiation of member function 'IPC::ArgumentCoder<long long>::decode' requested here return ArgumentCoder<T>::decode(*this, t); ^ /home/lantw44/gnome/source/webkit-trunk/Source/WebKit2/Platform/IPC/ArgumentCoders.h:116:22: note: in instantiation of function template specialization 'IPC::ArgumentDecoder::decode<long long>' requested here if (!decoder.decode(count)) ^ /home/lantw44/gnome/source/webkit-trunk/Source/WebKit2/Platform/IPC/ArgumentDecoder.h:89:34: note: in instantiation of member function 'IPC::ArgumentCoder<std::__1::chrono::duration<long long, std::__1::ratio<1, 1000> > >::decode' requested here return ArgumentCoder<T>::decode(*this, t); ^ /home/lantw44/gnome/source/webkit-trunk/Source/WebKit2/Shared/Network/NetworkResourceLoadParameters.cpp:160:18: note: in instantiation of function template specialization 'IPC::ArgumentDecoder::decode<std::__1::chrono::duration<long long, std::__1::ratio<1, 1000> > >' requested here if (!decoder.decode(result.maximumBufferingTime)) ^ 2 errors generated.
Created attachment 238421 [details] A simple patch to fix the build failure
Comment on attachment 238421 [details] A simple patch to fix the build failure This is not the right way to fix the build failure. IPC should use fixed size types. What C++ library are you using? It seems to be buggy or obsolete, std::chrono::milliseconds is of type std::chrono::duration, not a long long.
(In reply to comment #2) > (From update of attachment 238421 [details]) > This is not the right way to fix the build failure. IPC should use fixed size types. I know it is not good to use long long directly, but there is no fixed size type which is a typedef of long long. > > What C++ library are you using? It seems to be buggy or obsolete, std::chrono::milliseconds is of type std::chrono::duration, not a long long. It should be std::chrono::duration<long long, milli>. I am sorry for the wrong information. FreeBSD 10 uses libc++ by default.
There are an encoder and a decoder for duration in ArgumentCoders.h. Are they not matched by the compiler for some reason? template<typename Rep, typename Period> struct ArgumentCoder<std::chrono::duration<Rep, Period>> { static void encode(ArgumentEncoder& encoder, const std::chrono::duration<Rep, Period>& duration) { encoder << duration.count(); } static bool decode(ArgumentDecoder& decoder, std::chrono::duration<Rep, Period>& result) { Rep count; if (!decoder.decode(count)) return false; result = std::chrono::duration<Rep, Period>(count); return true; } };
(In reply to comment #4) > There are an encoder and a decoder for duration in ArgumentCoders.h. Are they not matched by the compiler for some reason? > > template<typename Rep, typename Period> struct ArgumentCoder<std::chrono::duration<Rep, Period>> { > static void encode(ArgumentEncoder& encoder, const std::chrono::duration<Rep, Period>& duration) > { > encoder << duration.count(); > } > > static bool decode(ArgumentDecoder& decoder, std::chrono::duration<Rep, Period>& result) > { > Rep count; > if (!decoder.decode(count)) > return false; > result = std::chrono::duration<Rep, Period>(count); > return true; > } > }; encoder << duration.count() causes it to go back to ArgumentEncoder. duration.count() is a long long, but there is no matching ArgumentEncoder::encode function. Neither int32_t nor int64_t is a long long.
Makes sense (int64_t is a long long on OS X). In this case, the right fix is to have an explicit cast in ArgumentCoder for duration to a type that's guaranteed to be large enough.
Created attachment 238475 [details] Cast std::chrono::duration.count() to int64_t in ArgumentCoder
This also occurs on Linux when compiling with Clang and libc++. On these 64-bit systems int64_t is used to define the `long` type instead of `long long`.
Comment on attachment 238475 [details] Cast std::chrono::duration.count() to int64_t in ArgumentCoder Marking this patch as reviewable.
Comment on attachment 238475 [details] Cast std::chrono::duration.count() to int64_t in ArgumentCoder View in context: https://bugs.webkit.org/attachment.cgi?id=238475&action=review > Source/WebKit2/Platform/IPC/ArgumentCoders.h:110 > + encoder << static_cast<int64_t>(duration.count()); What guarantees that int64_t is big enough for Rep? Or that Rep is not a double? I think that we need either a more specific coder, or a compile time assertion. > Source/WebKit2/Platform/IPC/ArgumentCoders.h:118 > - result = std::chrono::duration<Rep, Period>(count); > + result = std::chrono::duration<Rep, Period>(static_cast<Rep>(count)); Shouldn't we verify that it doesn't overflow Rep? Data sent over IPC is untrusted.
(In reply to comment #10) > (From update of attachment 238475 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=238475&action=review > > > Source/WebKit2/Platform/IPC/ArgumentCoders.h:110 > > + encoder << static_cast<int64_t>(duration.count()); > > What guarantees that int64_t is big enough for Rep? Or that Rep is not a double? I don't know whether int64_t is big enough, but I can make sure Rep is an integer. The standard says: typedef duration<signed integer type of at least 45 bits, milli> milliseconds; Do we really needs a type which is larger than int64_t to represent time? int64_t is already very large. > > I think that we need either a more specific coder, or a compile time assertion. > > > Source/WebKit2/Platform/IPC/ArgumentCoders.h:118 > > - result = std::chrono::duration<Rep, Period>(count); > > + result = std::chrono::duration<Rep, Period>(static_cast<Rep>(count)); > > Shouldn't we verify that it doesn't overflow Rep? Data sent over IPC is untrusted. If we can solve the encoder problem, I think we can just use the same type in the decoder as in the encoder.
Can anyone review the patch again?
I thought that it still needed to be updated to address latest comments, wasn't it?
Comment on attachment 238475 [details] Cast std::chrono::duration.count() to int64_t in ArgumentCoder View in context: https://bugs.webkit.org/attachment.cgi?id=238475&action=review >>> Source/WebKit2/Platform/IPC/ArgumentCoders.h:110 >>> + encoder << static_cast<int64_t>(duration.count()); >> >> What guarantees that int64_t is big enough for Rep? Or that Rep is not a double? >> >> I think that we need either a more specific coder, or a compile time assertion. > > I don't know whether int64_t is big enough, but I can make sure Rep is an integer. > The standard says: typedef duration<signed integer type of at least 45 bits, milli> milliseconds; > > Do we really needs a type which is larger than int64_t to represent time? int64_t is already very large. How about a static_assert here that checks that Rep is a signed 64-bit integer type? Something like: static_assert(std::is_integral<Rep>::value && std::is_signed<Rep>::value && sizeof(Rep) == sizeof(int64_t), "Just to be sure.");
Created attachment 240204 [details] Cast std::chrono::duration.count() to int64_t in ArgumentCoder
Comment on attachment 240204 [details] Cast std::chrono::duration.count() to int64_t in ArgumentCoder View in context: https://bugs.webkit.org/attachment.cgi?id=240204&action=review > Source/WebKit2/Platform/IPC/ArgumentCoders.h:110 > + static_assert(std::is_integral<Rep>::value && std::is_signed<Rep>::value && sizeof(Rep) <= sizeof(int64_t), "Rep should be an integer which can be fit in an int64_t."); I would phrase this differently: "Serialization of this Rep type is not supported yet." With the current phrasing, someone hitting this error will not know whether it's verifying that we never try to encode something else (for some unstated subtle reason), or if it's only documenting a current limitation. Alternatively, we could only specialize the template for supported types.
Created attachment 240550 [details] Cast std::chrono::duration.count() to int64_t in ArgumentCoder
Created attachment 240554 [details] Cast std::chrono::duration.count() to int64_t in ArgumentCoder
Comment on attachment 240554 [details] Cast std::chrono::duration.count() to int64_t in ArgumentCoder Clearing flags on attachment: 240554 Committed r175299: <http://trac.webkit.org/changeset/175299>
All reviewed patches have been landed. Closing bug.