Bug 211880

<rdar://problem/63213462>

Created attachment 399331 [details]
Patch v1

Comment on attachment 399331 [details]
Patch v1

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

> Source/WTF/wtf/persistence/PersistentDecoder.h:79
>      }

Is this static_assert() still needed now that the Decoder::operator<<() method is a template?

    template<typename E, std::enable_if_t<std::is_enum<E>::value>* = nullptr>
    Decoder& operator>>(Optional<E>& result)
    {
        static_assert(sizeof(E) <= 8, "Enum type T must not be larger than 64 bits!");
        [...]
    }

> Source/WTF/wtf/persistence/PersistentEncoder.h:51
> +        static_assert(sizeof(E) <= sizeof(uint64_t), "Enum type must not be larger than 64 bits.");

Is this static_assert() still needed now that the method is a template?

Comment on attachment 399331 [details]
Patch v1

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

>> Source/WTF/wtf/persistence/PersistentEncoder.h:51
>> +        static_assert(sizeof(E) <= sizeof(uint64_t), "Enum type must not be larger than 64 bits.");
> 
> Is this static_assert() still needed now that the method is a template?

This is still needed.  In the decoder we should change 8 to sizeof(uint64_t).
We also can't make this change (uint64_t to std::underlying_type) because existing persisted data has encoded enums as 64-bit values.

Comment on attachment 399331 [details]
Patch v1

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

>>> Source/WTF/wtf/persistence/PersistentEncoder.h:51
>>> +        static_assert(sizeof(E) <= sizeof(uint64_t), "Enum type must not be larger than 64 bits.");
>> 
>> Is this static_assert() still needed now that the method is a template?
> 
> This is still needed.  In the decoder we should change 8 to sizeof(uint64_t).
> We also can't make this change (uint64_t to std::underlying_type) because existing persisted data has encoded enums as 64-bit values.

Fixed in v2.  Thanks!

Created attachment 399399 [details]
Patch v7

Comment on attachment 399399 [details]
Patch v7

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

> Source/WTF/wtf/persistence/PersistentEncoder.h:-87
> -    template <typename Type> struct Salt;

Made Encoder::Salt public to try to fix WinCairo build.

Comment on attachment 399399 [details]
Patch v7

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

>> Source/WTF/wtf/persistence/PersistentEncoder.h:-87
>> -    template <typename Type> struct Salt;
> 
> Made Encoder::Salt public to try to fix WinCairo build.

That didn't work.  I'm out of ideas.  I think the WinCairo EWS VC++ compiler is out-of-date.  Every other platform (including AppleWin) build these patches.

I need C++ template help to fix the WinCario build errors.  I don't understand what Visual C++ is complaining about:

[32/422] Building CXX object Source\WebCore\CMakeFiles\WebCore.dir\platform\network\curl\CertificateInfoCurl.cpp.obj
FAILED: Source/WebCore/CMakeFiles/WebCore.dir/platform/network/curl/CertificateInfoCurl.cpp.obj 
[...]
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentEncoder.h(66): error C2672: 'WTF::Persistence::Encoder::updateChecksumForNumber': no matching overloaded function found
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentCoders.h(261): note: see reference to function template instantiation 'WTF::Persistence::Encoder &WTF::Persistence::Encoder::operator <<<double,0x0>(T)' being compiled
        with
        [
            T=double
        ]
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentEncoder.h(66): error C2893: Failed to specialize function template 'void WTF::Persistence::Encoder::updateChecksumForNumber(WTF::SHA1 &,T)'
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentEncoder.h(76): note: see declaration of 'WTF::Persistence::Encoder::updateChecksumForNumber'
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentEncoder.h(66): note: With the following template arguments:
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentEncoder.h(66): note: 'T=T'
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentDecoder.h(54): error C2672: 'WTF::Persistence::Encoder::updateChecksumForNumber': no matching overloaded function found
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentCoders.h(267): note: see reference to function template instantiation 'WTF::Persistence::Decoder &WTF::Persistence::Decoder::operator >><double,0x0>(WTF::Optional<double> &)' being compiled
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentDecoder.h(54): error C2893: Failed to specialize function template 'void WTF::Persistence::Encoder::updateChecksumForNumber(WTF::SHA1 &,T)'
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentEncoder.h(76): note: see declaration of 'WTF::Persistence::Encoder::updateChecksumForNumber'
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentDecoder.h(54): note: With the following template arguments:
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentDecoder.h(54): note: 'T=T'
[36/422] Building CXX object Source\WebCore\CMakeFiles\WebCore.dir\platform\network\curl\CurlContext.cpp.obj
FAILED: Source/WebCore/CMakeFiles/WebCore.dir/platform/network/curl/CurlContext.cpp.obj 
[...]
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentEncoder.h(66): error C2672: 'WTF::Persistence::Encoder::updateChecksumForNumber': no matching overloaded function found
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentCoders.h(261): note: see reference to function template instantiation 'WTF::Persistence::Encoder &WTF::Persistence::Encoder::operator <<<double,0x0>(T)' being compiled
        with
        [
            T=double
        ]
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentEncoder.h(66): error C2893: Failed to specialize function template 'void WTF::Persistence::Encoder::updateChecksumForNumber(WTF::SHA1 &,T)'
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentEncoder.h(76): note: see declaration of 'WTF::Persistence::Encoder::updateChecksumForNumber'
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentEncoder.h(66): note: With the following template arguments:
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentEncoder.h(66): note: 'T=T'
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentDecoder.h(54): error C2672: 'WTF::Persistence::Encoder::updateChecksumForNumber': no matching overloaded function found
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentCoders.h(267): note: see reference to function template instantiation 'WTF::Persistence::Decoder &WTF::Persistence::Decoder::operator >><double,0x0>(WTF::Optional<double> &)' being compiled
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentDecoder.h(54): error C2893: Failed to specialize function template 'void WTF::Persistence::Encoder::updateChecksumForNumber(WTF::SHA1 &,T)'
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentEncoder.h(76): note: see declaration of 'WTF::Persistence::Encoder::updateChecksumForNumber'
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentDecoder.h(54): note: With the following template arguments:
C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentDecoder.h(54): note: 'T=T'

Here are pages explaining the two error codes:

https://docs.microsoft.com/en-us/cpp/error-messages/compiler-errors-2/compiler-error-c2672?view=vs-2019
https://docs.microsoft.com/en-us/cpp/error-messages/compiler-errors-2/compiler-error-c2893?view=vs-2019

Hold on.  I think I know what's going on.

Encoder::updateChecksumForNumber() used `typename Type` instead of `typename T`, which differed from other template functions in the Persistence::Encoder class.

Created attachment 399420 [details]
Patch v9

(In reply to David Kilzer (:ddkilzer) from comment #19)
> Hold on.  I think I know what's going on.
> 
> Encoder::updateChecksumForNumber() used `typename Type` instead of `typename
> T`, which differed from other template functions in the Persistence::Encoder
> class.

And I was wrong.

As an observation from playing with it, with the Coder<Seconds> and Coder<WallTime> bodies in PersistentCoders.cpp rather than the .h, many of the errors went away, but the Coder<CertificateInfo> still failed (either in the h or the cpp) and it seems like the cases that fail are in files that were including CertificateInfo. I don't see why that'd be different than anything else that might be including the persistence files though.

Created attachment 400129 [details]
Patch v11

(In reply to David Kilzer (:ddkilzer) from comment #24)
> Created attachment 400129 [details]
> Patch v11

Style bot is just complaining about having the salt() methods defined on one line:

ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:90:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:91:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:92:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:93:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:94:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:95:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:96:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:97:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:98:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:99:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/persistence/PersistentEncoder.h:100:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 11 in 5 files

(In reply to David Kilzer (:ddkilzer) from comment #24)
> Created attachment 400129 [details]
> Patch v11

Still not sure why WinCairo build is failing.

I just don't understand the error message that MSVC++ is printing.

Created attachment 400131 [details]
Patch v12

(In reply to David Kilzer (:ddkilzer) from comment #27)
> Created attachment 400131 [details]
> Patch v12

One last attempt (which I tried when struct Encode::Salt existed):

-        Encoder::updateChecksumForNumber(m_sha1, value);
+        Encoder::updateChecksumForNumber<U>(m_sha1, value);

Created attachment 400147 [details]
Patch v13

(In reply to David Kilzer (:ddkilzer) from comment #29)
> Created attachment 400147 [details]
> Patch v13

I noticed in this error message:

C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/persistence/PersistentCoders.h(261): note: see reference to function template instantiation 'WTF::Persistence::Encoder &WTF::Persistence::Encoder::operator <<<double,0x0>(U)' being compiled
        with
        [
            U=double
        ]

That the template values are <double,0x0> for operator<<(), while updateChecksumForNumber() would only be <double>, so I tried adding a std::is_arithmetic<> check to updateChecksumForNumber() to see if that fixes it.

I also moved the implementation of updateChecksumForNumber() into the class itself, although I don't think that matters.

Created attachment 400149 [details]
Patch v14

(In reply to David Kilzer (:ddkilzer) from comment #31)
> Created attachment 400149 [details]
> Patch v14

Remove extra template parameter to Encoder::updateChecksumForNumber() (although I've tried this before):

-            Encoder::updateChecksumForNumber<U>(m_sha1, value);
+            Encoder::updateChecksumForNumber(m_sha1, value);

Use unique template parameter for Encoder::updateChecksumForNumber() and Encoder::salt():

-    template <typename U, std::enable_if_t<std::is_arithmetic<U>::value>* = nullptr>
-    static void updateChecksumForNumber(SHA1& sha1, U value)
+    template <typename V, std::enable_if_t<std::is_arithmetic<V>::value>* = nullptr>
+    static void updateChecksumForNumber(SHA1& sha1, V value)

-    template <typename U> static constexpr unsigned salt();
+    template <typename V> static constexpr unsigned salt();

This probably won't matter.  Will just change the "U=U" message in the error to "U=V" for the WinCairo build.

Created attachment 400165 [details]
Patch v15

(In reply to David Kilzer (:ddkilzer) from comment #33)
> Created attachment 400165 [details]
> Patch v15

Move updateChecksumForData() and updateChecksumForNumber() into their own WTF::Persistence::Checksum class since they're used in both the Decoder and Encoder classes.  (The Checksum class could probably be moved to its own header/source files so that PersistentDecoder.h doesn't have to include PersistentEncoder.h.)

Created attachment 400166 [details]
Patch v16

(In reply to David Kilzer (:ddkilzer) from comment #35)
> Created attachment 400166 [details]
> Patch v16

Moved Checksum::updateChecksumForNumber() implementation out-of-line of the class in case it needs the Checksum::Salt<> templates to be defined first with MSVC++ (even though the error doesn't mention Checksum::Salt<> at all).

Created attachment 400167 [details]
Patch v17

Template the whole WTF::Persistence::Checksum class.  I don't like this solution, but I want to see if the WinCairo build still fails.

I wonder if this whole issue has to do with trying to template a static class method in a header.  Technically you can only have one copy of the static method, so maybe that's what MSVC++ is actually complaining about.

In that case, we may just need to move the method out of a class and into a stand-alone function.

(In reply to David Kilzer (:ddkilzer) from comment #37)
> I wonder if this whole issue has to do with trying to template a static
> class method in a header.  Technically you can only have one copy of the
> static method, so maybe that's what MSVC++ is actually complaining about.

Specifically, the issue may be due to calling a templated static method from a templated instance method since MSVC++ realizes that this could cause a scenario where multiple copies of the same static method could be created, thus causing a linker error later.

In Patch v18, I'll try adding `inline` to Checksum::updateChecksumForNumber() based on Patch v16.

Created attachment 400173 [details]
Patch v18

Patch v16 with the addition of the `inline` keyword to Checksum::updateChecksumForNumber().

Created attachment 400174 [details]
Patch v19

(In reply to David Kilzer (:ddkilzer) from comment #40)
> Created attachment 400174 [details]
> Patch v19

Replace Checksum class with Checksum namespace so formerly static class methods may now be inlined as stand-alone functions.

Created attachment 400176 [details]
Patch v20

Patch v20 is Patch v19 with a second template argument for Checksum::Salt:

-template <typename T>
+template <typename T, typename U = T>
 inline void updateChecksumForNumber(SHA1& sha1, T value)
 {
-    auto typeSalt = Salt<T>::value;
+    auto typeSalt = Salt<U>::value;
     sha1.addBytes(reinterpret_cast<uint8_t*>(&typeSalt), sizeof(typeSalt));
     sha1.addBytes(reinterpret_cast<uint8_t*>(&value), sizeof(value));
 }

Perhaps MSVC++ doesn't like to mix full (updateChecksumForNumber()) and partial (Salt) template specialization?

Created attachment 400199 [details]
Patch v21

Patch v21 manually inlines Checksum::updateChecksumForNumber() to eliminate it to find out what MSVC++ will complain about next.  It is based on Patches v20 and v21.

I should have probably tried this about 10-15 patches ago.

Created attachment 400200 [details]
Patch v22 - v19 with Checksum:: updateChecksumForNumber() inlined - WinCairo compiled!

Patch v22 is v21 with build fixes.  (Forgot to build locally first!)

(In reply to David Kilzer (:ddkilzer) from comment #43)
> Created attachment 400199 [details]
> Patch v21
> 
> Patch v21 manually inlines Checksum::updateChecksumForNumber() to eliminate
> it to find out what MSVC++ will complain about next.  It is based on Patches
> v20 and v21.
> 
> I should have probably tried this about 10-15 patches ago.

So Patch v22 (v21 with build fixes) compiles with MSVC++ on WinCairo EWS bot.

This issue seems to have something to do with partial specialization of struct Salt vs. full specialization of Checksum::updateChecksumForNumber().

The more experiments I try, the more this seems like an MSVC++ compiler bug.

Created attachment 400205 [details]
Patch v23

Patch v23 is Patch v19 with a fallback full template added for struct Salt (which is unsafe; if we use this fix, I'd change struct Salt back to a template function so we could static_assert() that the fallback is not implemented if something tries to use it):

+template <typename T> struct Salt { static constexpr unsigned value = static_cast<unsigned>(-1); };

Let's see if this compiles with MSVC++ on WinCairo.

(In reply to Stephan Szabo from comment #22)
> As an observation from playing with it, with the Coder<Seconds> and
> Coder<WallTime> bodies in PersistentCoders.cpp rather than the .h, many of
> the errors went away, but the Coder<CertificateInfo> still failed (either in
> the h or the cpp) and it seems like the cases that fail are in files that
> were including CertificateInfo. I don't see why that'd be different than
> anything else that might be including the persistence files though.

The Source/WebCore/platform/network/curl/CertificateInfo.h header includes Source/WebCore/platform/network/CertificateInfoBase.h, which includes <wtf/Seconds.h>.  Note that <wtf/WallTime.h> also includes <wtf/Seconds.h>.

The interesting thing about <wtf/Seconds.h> is that it includes encode/decode methods that templatize the Encoder and Decoder classes, presumably to reuse the same code for Persistence::Encoder/Persistence::Decoder and IPC::Encoder/IPC::Decoder;

    template<class Encoder>
    void encode(Encoder& encoder) const
    {
        encoder << m_value;
    }

    template<class Decoder>
    static Optional<Seconds> decode(Decoder& decoder)
    {
        Optional<double> seconds;
        decoder >> seconds;
        if (!seconds)
            return WTF::nullopt;
        return Seconds(*seconds);
    }

    template<class Decoder>
    static WARN_UNUSED_RETURN bool decode(Decoder& decoder, Seconds& seconds)
    {
        double value;
        if (!decoder.decode(value))
            return false;

        seconds = Seconds(value);
        return true;
    }

I wonder if there are any other classes that do this?

Perhaps this issue is as simple as the <wtf/Seconds.h> header being included before the <wtf/persistence/Persistent{En,De}coder.h> headers, so MSVC++ claims that it can't find a template specialization?

We can test this theory by including <wtf/persistence/Persistent{De,En}coder.h> in <wtf/Seconds.h> to see if that magically fixes the build.

Comment on attachment 400205 [details]
Patch v23

(In reply to David Kilzer (:ddkilzer) from comment #46)
> Created attachment 400205 [details]
> Patch v23
> 
> Patch v23 is Patch v19 with a fallback full template added for struct Salt
> (which is unsafe; if we use this fix, I'd change struct Salt back to a
> template function so we could static_assert() that the fallback is not
> implemented if something tries to use it):
> 
> +template <typename T> struct Salt { static constexpr unsigned value =
> static_cast<unsigned>(-1); };
> 
> Let's see if this compiles with MSVC++ on WinCairo.

Nope.  Did not compile on WinCairo.

Created attachment 400207 [details]
Patch v24

Patch v24 is the same as Patch v19, but includes Persistent{De,En}coder.h in <wtf/Seconds.h> to try to fix the WinCairo build:

diff --git a/Source/WTF/wtf/Seconds.h b/Source/WTF/wtf/Seconds.h
index c1be3927382..ce281977d32 100644
--- a/Source/WTF/wtf/Seconds.h
+++ b/Source/WTF/wtf/Seconds.h
@@ -27,6 +27,8 @@
 
 #include <wtf/MathExtras.h>
 #include <wtf/Optional.h>
+#include <wtf/persistence/PersistentDecoder.h>
+#include <wtf/persistence/PersistentEncoder.h>
 
 namespace WTF {
 

See Comment #47 for details.

(In reply to David Kilzer (:ddkilzer) from comment #49)
> Created attachment 400207 [details]
> Patch v24
> 
> Patch v24 is the same as Patch v19, but includes Persistent{De,En}coder.h in
> <wtf/Seconds.h> to try to fix the WinCairo build:
> 
> diff --git a/Source/WTF/wtf/Seconds.h b/Source/WTF/wtf/Seconds.h
> index c1be3927382..ce281977d32 100644
> --- a/Source/WTF/wtf/Seconds.h
> +++ b/Source/WTF/wtf/Seconds.h
> @@ -27,6 +27,8 @@
>  
>  #include <wtf/MathExtras.h>
>  #include <wtf/Optional.h>
> +#include <wtf/persistence/PersistentDecoder.h>
> +#include <wtf/persistence/PersistentEncoder.h>
>  
>  namespace WTF {
>  
> 
> See Comment #47 for details.

Of course that introduced a cyclical dependency in header includes, so we can't do that!

Created attachment 400208 [details]
Patch v25

Patch v25 is the same as Patch v19, but does the following:

- Extracts WTF::Persistence::Checksum methods into their own header:  Source/WTF/wtf/persistence/PersistentChecksum.h.
- Adds PersistentChecksum.h to build CMake and Xcode build systems.
- Includes <wtf/persistence/PersistentChecksum.h> in <wtf/Seconds.h> to try to fix the WinCairo build.  See Comment #47 for details.
- Breaks cyclical header include (introduced by adding PersistentChecksum.h to Seconds.h) by moving SHA1::addBytes() that takes a CString from SHA1.h to SHA1.cpp.
- Has nice side benefit of removing include of <wtf/persistence/PersistentEncoder.h> from PersistentDecoder.h in favor of <wtf/persistence/PersistentChecksum.h>.

Hi QuellaZhang, I CC-ed you on this radar because I've been trying to fix a build failure only seen on MSVC++ for the WinCairo port where gcc and clang (and an older MSVC++ for Apple's Windows port) work fine.

Via Slack, the WinCairo bots should be on MSVC 16.4:  <https://webkit.slack.com/archives/CU64U6FDW/p1589492625078800?thread_ts=1589490687.076600&cid=CU64U6FDW>

I will probably work around this by manually inlining the updateChecksumForNumber() method in the two places it's used, but I thought you might wand to take a look at this.

Created attachment 401311 [details]
v2 + OPENSSL_NO_SHA and OPENSSL_NO_SHA1

It seems that OpenSSL SHA1 function is conflicting with WTF::SHA1.
Defining OPENSSL_NO_SHA1 and OPENSSL_NO_SHA macros before including OpenSSL headers seems to solve the issue.