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 <[email protected]> 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 <[email protected]
> <mailto:[email protected]>> 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 <[email protected]
>> <mailto:[email protected]>> 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 <[email protected]
>>> <mailto:[email protected]>> wrote:
>>>
>>>
>>>
>>>> On Dec 17, 2018, at 11:10 AM, Chris Dumez <[email protected]
>>>> <mailto:[email protected]>> wrote:
>>>>
>>>>
>>>>
>>>>> On Dec 17, 2018, at 10:27 AM, Alex Christensen <[email protected]
>>>>> <mailto:[email protected]>> wrote:
>>>>>
>>>>>>>>> On Dec 14, 2018, at 1:37 PM, Chris Dumez <[email protected]
>>>>>>>>> <mailto:[email protected]>> 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
>> [email protected] <mailto:[email protected]>
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>> <https://lists.webkit.org/mailman/listinfo/webkit-dev>
>
> _______________________________________________
> webkit-dev mailing list
> [email protected] <mailto:[email protected]>
> https://lists.webkit.org/mailman/listinfo/webkit-dev
> <https://lists.webkit.org/mailman/listinfo/webkit-dev>
_______________________________________________
webkit-dev mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-dev
