Bug 187971

Summary: Move-constructing NeverDestroyed should move construct underlying object instead of copy constructing it
Product: WebKit Reporter: Daniel Bates <dbates>
Component: Web Template FrameworkAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, benjamin, cdumez, cmarcelo, darin, ews-watchlist, ross.kirsling, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=188244
Attachments:
Description Flags
Patch and tests
none
Patch and tests
none
Patch and tests
none
Patch and tests
none
Patch and tests none

Description Daniel Bates 2018-07-24 13:43:55 PDT 
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>
Comment 1 Daniel Bates 2018-07-24 13:45:51 PDT 
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.
Comment 2 Daniel Bates 2018-07-24 13:48:42 PDT 
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.
Comment 3 Daniel Bates 2018-07-24 13:51:30 PDT 
Created attachment 345709 [details]
Patch and tests
Comment 4 EWS Watchlist 2018-07-24 13:54:07 PDT Comment hidden (obsolete)
Comment 5 Daniel Bates 2018-07-24 13:56:59 PDT 
Created attachment 345710 [details]
Patch and tests
Comment 6 Daniel Bates 2018-07-24 14:03:33 PDT 
Created attachment 345712 [details]
Patch and tests
Comment 7 EWS Watchlist 2018-07-24 14:06:27 PDT Comment hidden (obsolete)
Comment 8 Daniel Bates 2018-07-24 14:47:29 PDT 
Created attachment 345715 [details]
Patch and tests

Make move constructing NeverDestroyed<const T> work and add a test.
Comment 9 EWS Watchlist 2018-07-24 14:49:47 PDT Comment hidden (obsolete)
Comment 10 Saam Barati 2018-07-24 14:51:30 PDT 
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?
Comment 11 Daniel Bates 2018-07-24 14:52:47 PDT 
Created attachment 345716 [details]
Patch and tests
Comment 12 EWS Watchlist 2018-07-24 14:55:05 PDT Comment hidden (obsolete)
Comment 13 Daniel Bates 2018-07-24 15:01:29 PDT 
(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.
Comment 14 Daniel Bates 2018-07-24 16:30:08 PDT 
Committed r234179: <https://trac.webkit.org/changeset/234179>
Comment 15 Radar WebKit Bug Importer 2018-07-24 16:31:25 PDT 
<rdar://problem/42561513>
Comment 16 Ross Kirsling 2018-08-01 15:27:26 PDT 
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?
Comment 17 Daniel Bates 2018-08-01 17:26:50 PDT 
(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?
Comment 18 Ross Kirsling 2018-08-01 17:46:10 PDT 
(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