Bug 136981 – ArgumentEncoder::encode does not support std::chrono::duration on some platforms

• Component: Platform
• Status: RESOLVED FIXED

[Ting-Wei Lan]

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.

[Ting-Wei Lan]

Created attachment 238421 [details]
A simple patch to fix the build failure

[Alexey Proskuryakov]

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.

[Ting-Wei Lan]

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

[Alexey Proskuryakov]

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;
    }
};

[Ting-Wei Lan]

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

[Alexey Proskuryakov]

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.

[Ting-Wei Lan]

Created attachment 238475 [details]
Cast std::chrono::duration.count() to int64_t in ArgumentCoder

[Zan Dobersek]

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

[Zan Dobersek]

Comment on attachment 238475 [details]
Cast std::chrono::duration.count() to int64_t in ArgumentCoder

Marking this patch as reviewable.

[Alexey Proskuryakov]

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.

[Ting-Wei Lan]

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

[Ting-Wei Lan]

Can anyone review the patch again?

[Alexey Proskuryakov]

I thought that it still needed to be updated to address latest comments, wasn't it?

[Zan Dobersek]

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.");

[Ting-Wei Lan]

Created attachment 240204 [details]
Cast std::chrono::duration.count() to int64_t in ArgumentCoder

[Alexey Proskuryakov]

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.

[Ting-Wei Lan]

Created attachment 240550 [details]
Cast std::chrono::duration.count() to int64_t in ArgumentCoder

[Ting-Wei Lan]

Created attachment 240554 [details]
Cast std::chrono::duration.count() to int64_t in ArgumentCoder

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.