Bug 187971 – Move-constructing NeverDestroyed should move construct underlying object instead of copy constructing it

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Daniel Bates]

Consider the following code taken from attachment #336064 [details] (bug #175784):

static const RenderStyle& hackedDefaultStyle()
{
    static auto defaultStyle = makeNeverDestroyed([] {
        auto style = RenderStyle::create();
        style.setColor(Color { });
        style.setTextFillColor(Color { });
        style.setTextStrokeColor(Color { });
        style.setBackgroundColor(Color { });
        style.setTextEmphasisColor(Color { });
        return style;
    }());
    return defaultStyle;
}

This fails to compile on Mac, GTK, and WPE with an error of the form:

[[
/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/wtf/NeverDestroyed.h:55:46: error: call to implicitly-deleted copy constructor of 'WebCore::RenderStyle'
        MaybeRelax<T>(new (storagePointer()) T(WTFMove(other)));
                                             ^ ~~~~~~~~~~~~~~
In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource411.cpp:7:
./rendering/InlineTextBox.cpp:773:32: note: in instantiation of member function 'WTF::NeverDestroyed<WebCore::RenderStyle>::NeverDestroyed' requested here
    static auto defaultStyle = makeNeverDestroyed([] {
                               ^
In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource411.cpp:2:
In file included from ./rendering/ImageQualityController.cpp:32:
In file included from ./rendering/RenderBoxModelObject.h:28:
In file included from ./rendering/RenderLayerModelObject.h:25:
In file included from ./rendering/RenderElement.h:26:
In file included from ./rendering/RenderObject.h:35:
/Volumes/Data/EWS/WebKit/Source/WebCore/rendering/style/RenderStyle.h:134:5: note: copy constructor is implicitly deleted because 'RenderStyle' has a user-declared move constructor
    RenderStyle(RenderStyle&&);
...
]]
<https://webkit-queues.webkit.org/results/7027403>

[Daniel Bates]

The issue is that the NeverDestroyed move constructor is moving the passed NeverDestroyed object instead of the underlying storage when placement-newing T. As a result, the compiler implicitly converts the to-be-moved-from NeverDestroyed to const T& and invokes T's copy constructor.

[Daniel Bates]

I suspect the reason this does not effect Windows is because MS Visual Studio has a more aggressive optimizer that elides the call to the NeverDestroyed move constructor. Such aggressive eliding is allowed to be performed even if the move constructor has side effects in C++14, but not required. As of C++17 such an optimization is required by all compiler writers and is consistent with my observation that I do not get a compile-time error when compiling the code in comment #0 with clang using C++17.

[Daniel Bates]

Created attachment 345709 [details]
Patch and tests

[EWS Watchlist]

Attachment 345709 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/MoveOnlyLifecycleLogger.cpp:74:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/MoveOnlyLifecycleLogger.cpp:77:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/MoveOnlyLifecycleLogger.cpp:80:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/NeverDestroyed.cpp:54:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/NeverDestroyed.cpp:74:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 345710 [details]
Patch and tests

[Daniel Bates]

Created attachment 345712 [details]
Patch and tests

[EWS Watchlist]

Attachment 345712 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/MoveOnlyLifecycleLogger.cpp:74:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/MoveOnlyLifecycleLogger.cpp:77:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/MoveOnlyLifecycleLogger.cpp:80:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/NeverDestroyed.cpp:54:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/NeverDestroyed.cpp:74:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 345715 [details]
Patch and tests

Make move constructing NeverDestroyed<const T> work and add a test.

[EWS Watchlist]

Attachment 345715 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/MoveOnlyLifecycleLogger.cpp:74:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/MoveOnlyLifecycleLogger.cpp:77:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/MoveOnlyLifecycleLogger.cpp:80:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/NeverDestroyed.cpp:54:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/NeverDestroyed.cpp:81:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 345715 [details]
Patch and tests

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

r=me

> Tools/ChangeLog:9
> +        Fixes an issue where move constructing a NeverDestroyed<T> would always copy construct T into
> +        the destination storage buffer regardless of whether T is move constructable. For example:

Do we really need this changelog copied here too?

[Daniel Bates]

Created attachment 345716 [details]
Patch and tests

[EWS Watchlist]

Attachment 345716 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/MoveOnlyLifecycleLogger.cpp:74:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/MoveOnlyLifecycleLogger.cpp:77:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/MoveOnlyLifecycleLogger.cpp:80:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/NeverDestroyed.cpp:54:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/NeverDestroyed.cpp:81:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

(In reply to Saam Barati from comment #10)
> > Tools/ChangeLog:9
> > +        Fixes an issue where move constructing a NeverDestroyed<T> would always copy construct T into
> > +        the destination storage buffer regardless of whether T is move constructable. For example:
> 
> Do we really need this changelog copied here too?

Will consider writing a more specific description for Tools/ChangeLog. I do not see an issue with duplicating the ChangeLog description in multiple ChangeLog files so long as it is descriptive. I use script commit-log-editor as my Git editor (this is part of the standard WebKit OpenSource Project setup). So, the commit message when I land the change will not contain duplication as commit-log-editor automatically collapses duplicate ChangeLog descriptions to create the commit message.

[Daniel Bates]

Committed r234179: <https://trac.webkit.org/changeset/234179>

[Radar WebKit Bug Importer]

<rdar://problem/42561513>

[Ross Kirsling]

It turns out that the two tests checking for...
> "construct(name) set-name(x) move-construct(x) destruct(<default>) "

...are failed by MSVC in Debug mode, where "move-construct(x) destruct(<default>) " appears an extra time.
(https://build.webkit.org/builders/WinCairo%2064-bit%20WKL%20Debug%20%28Tests%29/builds/703/steps/run-api-tests/logs/stdio)

This seems like a valid thing for a compiler to do when optimizations are off though, given that there's an immediately-invoked lambda followed by a makeNeverDestroyed call.

What do you think we should do about this?

[Daniel Bates]

(In reply to Ross Kirsling from comment #16)
> It turns out that the two tests checking for...
> > "construct(name) set-name(x) move-construct(x) destruct(<default>) "
> 
> ...are failed by MSVC in Debug mode, where "move-construct(x)
> destruct(<default>) " appears an extra time.
> (https://build.webkit.org/builders/WinCairo%2064-
> bit%20WKL%20Debug%20%28Tests%29/builds/703/steps/run-api-tests/logs/stdio)
> 
> This seems like a valid thing for a compiler to do when optimizations are
> off though, given that there's an immediately-invoked lambda followed by a
> makeNeverDestroyed call.
> 
> What do you think we should do about this?

Can we add a compiler-specific result for this test?

[Ross Kirsling]

(In reply to Daniel Bates from comment #17)
> (In reply to Ross Kirsling from comment #16)
> > It turns out that the two tests checking for...
> > > "construct(name) set-name(x) move-construct(x) destruct(<default>) "
> > 
> > ...are failed by MSVC in Debug mode, where "move-construct(x)
> > destruct(<default>) " appears an extra time.
> > (https://build.webkit.org/builders/WinCairo%2064-
> > bit%20WKL%20Debug%20%28Tests%29/builds/703/steps/run-api-tests/logs/stdio)
> > 
> > This seems like a valid thing for a compiler to do when optimizations are
> > off though, given that there's an immediately-invoked lambda followed by a
> > makeNeverDestroyed call.
> > 
> > What do you think we should do about this?
> 
> Can we add a compiler-specific result for this test?

Works for me! Patch submitted: https://bugs.webkit.org/show_bug.cgi?id=188244