[webkit-dev] Watch out for std::optional's move constructor
Geoffrey Garen
ggaren at apple.com
Mon Dec 17 17:17:25 PST 2018
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”;
(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 <mailto: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 <mailto: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 <mailto: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 <mailto: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 <mailto: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 <mailto: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 <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 <mailto:cdumez at apple.com>> wrote:
>>>>>
>>>>>
>>>>>
>>>>>> On Dec 17, 2018, at 11:10 AM, Chris Dumez <cdumez at apple.com <mailto:cdumez at apple.com>> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>> On Dec 17, 2018, at 10:27 AM, Alex Christensen <achristensen at apple.com <mailto:achristensen at apple.com>> wrote:
>>>>>>>
>>>>>>>>>>> On Dec 14, 2018, at 1:37 PM, Chris Dumez <cdumez at apple.com <mailto: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 <https://trac.webkit.org/changeset/236463/webkit>
>>>> _______________________________________________
>>>> webkit-dev mailing list
>>>> webkit-dev at lists.webkit.org <mailto:webkit-dev at lists.webkit.org>
>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev <https://lists.webkit.org/mailman/listinfo/webkit-dev>
>>>
>>> _______________________________________________
>>> webkit-dev mailing list
>>> webkit-dev at lists.webkit.org <mailto:webkit-dev at lists.webkit.org>
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev <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/20181217/a21feb1a/attachment.html>
More information about the webkit-dev
mailing list