[webkit-dev] Watch out for std::optional's move constructor

Tue Dec 18 01:50:46 PST 2018

On Tue, Dec 18, 2018 at 3:18 AM Geoffrey Garen <ggaren at apple.com> wrote:

> Again, the C++ standard does not say that moving from an object leaves the
> object in an undefined state.
>
> The C++ standard does say:
>
> Objects of types defined in the C++ standard library may be moved from
> (12.8). Move operations may be explicitly specified or implicitly
> generated. Unless otherwise specified, such moved-from objects shall be
> placed in a valid but unspecified state.
>
>
> A few ways this differs from the claim that moving from an object leaves
> the object in an undefined state:
>
> (1) The standard makes no mention of objects in general, only of objects
> in the C++ standard library;
>
> (2) Even for objects in the C++ standard library, the standard makes no
> mention of objects in general, only of objects that are not “otherwise
> specified”;
>

For example, std::unique_ptr does have a fully specified moved-from state
that can be relied on.

   antti

> (3) The standard avoids undefined-ness entirely and instead specifies a
> *valid* but unspecified state.
>
> I think it is accurate to say that the C++ standard specifies that *some* objects
> *in the standard library* have *valid but unspecified* state when moved
> from.
>
> I think it’s also accurate to say that a function that accepts an rvalue
> reference as an argument does not promise to move from that rvalue
> reference.
>
> I think both of these statements are compatible with specifying what
> happens *in WebKit libraries* when we* do move from *an object.
>
> Geoff
>
> On Dec 17, 2018, at 5:04 PM, Alex Christensen <achristensen at apple.com>
> wrote:
>
> We can definitely make our own WTF::Optional instead of using
> std::optional and change its move constructor/operator= and I personally
> think that would not be worth it but if I’m in the minority I’ll deal with
> it.  We cannot diverge from the C++ standards that say that moving from an
> object leaves the object in an undefined state, and right now in WebKit we
> are relying quite a lot on that undefined state.  I think that is the
> bigger problem that Chris was trying to solve.
>
> On Dec 17, 2018, at 4:32 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
>
> Let me add this.
>
> The situation we have here is analogous to having a member function
> "move()" which leave std::optional in undefined state as in:
>
> std::optional<int> a;
> std::optional<int> b = a.move();
>
> would leave a in some undefined state. I don't care what C++ standards say
> or what STL does. That's insane.
> Every object should remain in a well defined state after performing an
> operation.
>
> - R. Niwa
>
>
> On Mon, Dec 17, 2018 at 4:22 PM Ryosuke Niwa <rniwa at webkit.org> wrote:
>
>> It's true that WTFMove or std::move doesn't do anything if the moved
>> variable is not used because WTFMove / std::move is just a type cast.
>>
>> However, that behavior is orthogonal from the issue that calling WTFMove
>> / std::move on std::optional, and the returned value is assigned to another
>> std::optional, the original std::optional will be left a bad state.
>>
>> I completely disagree with your assessment that this calls for the use of
>> std::exchange.
>>
>>
>> On Mon, Dec 17, 2018 at 3:55 PM Alex Christensen <achristensen at apple.com>
>> wrote:
>>
>>> Let me give a concrete example on why, even with our nice-to-use WTF
>>> types, the state of a C++ object is undefined after being moved from:
>>>
>>> #include <wtf/RefCounted.h>
>>> #include <wtf/RefPtr.h>
>>> #include <iostream>
>>>
>>> class Test : public RefCounted<Test> { };
>>>
>>> void useParameter(RefPtr<Test>&& param)
>>> {
>>>   RefPtr<Test> usedParam = WTFMove(param);
>>> }
>>>
>>> void dontUseParameter(RefPtr<Test>&&) { }
>>>
>>> int main() {
>>>   RefPtr<Test> a = adoptRef(new Test);
>>>   RefPtr<Test> b = adoptRef(new Test);
>>>   std::cout << "a null? " << !a << std::endl;
>>>   std::cout << "b null? " << !b << std::endl;
>>>   useParameter(WTFMove(a));
>>>   dontUseParameter(WTFMove(b));
>>>   std::cout << "a null? " << !a << std::endl;
>>>   std::cout << "b null? " << !b << std::endl;
>>>   return 0;
>>> }
>>>
>>> // clang++ test.cpp -I Source/WTF -L WebKitBuild/Debug -l WTF -framework
>>> Foundation -L /usr/lib -l icucore --std=c++17 && ./a.out
>>>
>>> // a null? 0
>>>
>>>
>>> // b null? 0
>>>
>>>
>>> // a null? 1
>>>
>>>
>>> // b null? 0
>>>
>>>
>>>
>>>
>>> As you can see, the internals of callee dontUseParameter (which could be
>>> in a different translation unit) affects the state of the local variable b
>>> in this function.  This is one of the reasons why the state of a moved-from
>>> variable is intentionally undefined, and we can’t fix that by using our own
>>> std::optional replacement.  If we care about the state of a moved-from
>>> object, that is what std::exchange is for.  I think we should do something
>>> to track and prevent the use of moved-from values instead of introducing
>>> our own std::optional replacement.
>>>
>>> On Dec 17, 2018, at 2:47 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
>>>
>>> Yeah, it seems like making std::optional more in line with our own
>>> convention provides more merits than downsides here. People are using
>>> WTFMove as if it's some sort of a swap operation in our codebase, and as
>>> Maciej pointed out, having rules where people have to think carefully as to
>>> when & when not to use WTFMove seems more troublesome than the proposed
>>> fix, which would mean this work for optional.
>>>
>>> - R. Niwa
>>>
>>> On Mon, Dec 17, 2018 at 2:24 PM Geoffrey Garen <ggaren at apple.com> wrote:
>>>
>>>> I don’t understand the claim about “undefined behavior” here. As Maciej
>>>> pointed out, these are our libraries. We are free to define their behaviors.
>>>>
>>>> In general, “undefined behavior” is an unwanted feature of programming
>>>> languages and libraries, which we accept begrudgingly simply because there
>>>> are practical limits to what we can define. This acceptance is not a
>>>> mandate to carry forward undefined-ness as a badge of honor. In any case
>>>> where it would be practical to define a behavior, that defined behavior
>>>> would be preferable to undefined behavior.
>>>>
>>>> I agree that the behavior of move constructors in the standard library
>>>> is undefined. The proposal here, as I understand it, is to (a) define the
>>>> behaviors move constructors in WebKit and (b) avoid std::optional and use
>>>> an optional class with well-defined behavior instead.
>>>>
>>>> Because I do not ❤️ security updates, I do ❤️ defined behavior, and so
>>>> I ❤️ this proposal.
>>>>
>>>> Geoff
>>>>
>>>> On Dec 17, 2018, at 12:50 PM, Alex Christensen <achristensen at apple.com>
>>>> wrote:
>>>>
>>>> This one and the many others like it are fragile, relying on undefined
>>>> behavior, and should be replaced by std::exchange.  Such a change was made
>>>> in https://trac.webkit.org/changeset/198755/webkit and we probably
>>>> need many more like that, but we are getting away with relying on undefined
>>>> behavior which works for us in most places.
>>>>
>>>> On Dec 17, 2018, at 11:24 AM, Chris Dumez <cdumez at apple.com> wrote:
>>>>
>>>>
>>>>
>>>> On Dec 17, 2018, at 11:10 AM, Chris Dumez <cdumez at apple.com> wrote:
>>>>
>>>>
>>>>
>>>> On Dec 17, 2018, at 10:27 AM, Alex Christensen <achristensen at apple.com>
>>>> wrote:
>>>>
>>>> On Dec 14, 2018, at 1:37 PM, Chris Dumez <cdumez at apple.com> wrote:
>>>>
>>>>
>>>> As far as I know, our convention in WebKit so far for our types has
>>>> been that types getting moved-out are left in a valid “empty” state.
>>>>
>>>> This is not necessarily true.  When we move out of an object to pass
>>>> into a function parameter, for example, the state of the moved-from object
>>>> depends on the behavior of the callee.  If the callee function uses the
>>>> object, we often have behavior that leaves the object in an “empty” state
>>>> of some kind, but we are definitely relying on fragile undefined behavior
>>>> when we do so because changing the callee to not use the parameter changes
>>>> the state of the caller.  We should never assume that WTFMove or std::move
>>>> leaves the object in an empty state.  That is always a bug that needs to be
>>>> replaced by std::exchange.
>>>>
>>>>
>>>> Feel like we’re taking about different things. I am talking about move
>>>> constructors (and assignment operators), which have a well defined behavior
>>>> in WebKit. And it seems you are talking about WTFMove(), which despite the
>>>> name does not “move” anything, it is merely a cast.
>>>> In the case you’re talking about the caller does NOT call the move
>>>> constructor, it merely does a cast so I do not think your comment
>>>> invalidates my statement. Note that in my patch, I was nearly WTFMove()ing
>>>> the data member and assigning it to a local variable right away, calling
>>>> the move constructor.
>>>>
>>>>
>>>> Also note that may of us already rely on our move constructors’
>>>> behavior, just search for WTFMove(m_responseCompletionHandler) in:
>>>> https://trac.webkit.org/changeset/236463/webkit
>>>>
>>>>
>>>> _______________________________________________
>>>> webkit-dev mailing list
>>>> webkit-dev at lists.webkit.org
>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>>>
>>>>
>>>> _______________________________________________
>>>> webkit-dev mailing list
>>>> webkit-dev at lists.webkit.org
>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>>>
>>>
>>>
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
>
>
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20181218/d7e04db7/attachment.html>