Watch out for std::optional's move constructor

# Chris Dumez (6 days ago)

I have now been caught twice by std::optional’s move constructor. It turns out that it leaves the std::optional being moved-out engaged, it merely moves its value.

For example, testOptional.cpp:

#include <iostream>

#include <optional>

int main() { std::optional<int> a = 1; std::optional<int> b = std::move(a); std::cout << "a is engaged? " << !!a << std::endl; std::cout << "b is engaged? " << !!b << std::endl; return 0; }

$ clang++ testOptional.cpp -o testOptional -std=c++17 $ ./testOptional a is engaged? 1 b is engaged? 1

I would have expected: a is engaged? 0 b is engaged? 1

This impacts the standard std::optional implementation on my machine as well as the local copy in WebKit’s wtf/Optional.h.

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. As such, I find that std::optional’s move constructor behavior is error-prone.

I’d like to know how do other feel about this behavior? If enough people agree this is error-prone, would we consider having our own optional type in WTF which resets the engaged flag (and never allow the std::optional)?

Thanks,

 Chris Dumez

Contact us to advertise here
# Saam Barati (6 days ago)

On Dec 14, 2018, at 1:37 PM, Chris Dumez <cdumez at apple.com> wrote:

Hi,

I have now been caught twice by std::optional’s move constructor. It turns out that it leaves the std::optional being moved-out engaged, it merely moves its value.

For example, testOptional.cpp:

#include <iostream>

#include <optional>

int main() { std::optional<int> a = 1; std::optional<int> b = std::move(a); std::cout << "a is engaged? " << !!a << std::endl; std::cout << "b is engaged? " << !!b << std::endl; return 0; }

$ clang++ testOptional.cpp -o testOptional -std=c++17 $ ./testOptional a is engaged? 1 b is engaged? 1

I would have expected: a is engaged? 0 b is engaged? 1

I would have expected this too.

This impacts the standard std::optional implementation on my machine as well as the local copy in WebKit’s wtf/Optional.h.

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.

I believe it's UB to use an object after it has been moved.

# Saam Barati (6 days ago)

On Dec 14, 2018, at 1:54 PM, Saam Barati <sbarati at apple.com> wrote:

On Dec 14, 2018, at 1:37 PM, Chris Dumez <cdumez at apple.com <mailto:cdumez at apple.com>> wrote:

Hi,

I have now been caught twice by std::optional’s move constructor. It turns out that it leaves the std::optional being moved-out engaged, it merely moves its value.

For example, testOptional.cpp:

#include <iostream>

#include <optional>

int main() { std::optional<int> a = 1; std::optional<int> b = std::move(a); std::cout << "a is engaged? " << !!a << std::endl; std::cout << "b is engaged? " << !!b << std::endl; return 0; }

$ clang++ testOptional.cpp -o testOptional -std=c++17 $ ./testOptional a is engaged? 1 b is engaged? 1

I would have expected: a is engaged? 0 b is engaged? 1 I would have expected this too.

This impacts the standard std::optional implementation on my machine as well as the local copy in WebKit’s wtf/Optional.h.

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. I believe it's UB to use an object after it has been moved.

I'm wrong here. Apparently objects are left in a "valid but unspecified state".

stackoverflow.com/questions/32346143/undefined-behavior-with-stdmove, stackoverflow.com/questions/32346143/undefined-behavior-with-stdmove

# Chris Dumez (6 days ago)

On Dec 14, 2018, at 1:56 PM, Saam Barati <sbarati at apple.com> wrote:

On Dec 14, 2018, at 1:54 PM, Saam Barati <sbarati at apple.com <mailto:sbarati at apple.com>> wrote:

On Dec 14, 2018, at 1:37 PM, Chris Dumez <cdumez at apple.com <mailto:cdumez at apple.com>> wrote:

Hi,

I have now been caught twice by std::optional’s move constructor. It turns out that it leaves the std::optional being moved-out engaged, it merely moves its value.

For example, testOptional.cpp:

#include <iostream>

#include <optional>

int main() { std::optional<int> a = 1; std::optional<int> b = std::move(a); std::cout << "a is engaged? " << !!a << std::endl; std::cout << "b is engaged? " << !!b << std::endl; return 0; }

$ clang++ testOptional.cpp -o testOptional -std=c++17 $ ./testOptional a is engaged? 1 b is engaged? 1

I would have expected: a is engaged? 0 b is engaged? 1 I would have expected this too.

This impacts the standard std::optional implementation on my machine as well as the local copy in WebKit’s wtf/Optional.h.

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. I believe it's UB to use an object after it has been moved. I'm wrong here. Apparently objects are left in a "valid but unspecified state".

stackoverflow.com/questions/32346143/undefined-behavior-with-stdmove, stackoverflow.com/questions/32346143/undefined-behavior-with-stdmove

I believe in WebKit, however, we’ve made sure our types are left in a valid “empty” state, thus my proposal for our own optional type that would be less error-prone / more convenient to use.

# Saam Barati (6 days ago)

On Dec 14, 2018, at 1:59 PM, Chris Dumez <cdumez at apple.com> wrote:

On Dec 14, 2018, at 1:56 PM, Saam Barati <sbarati at apple.com <mailto:sbarati at apple.com>> wrote:

On Dec 14, 2018, at 1:54 PM, Saam Barati <sbarati at apple.com <mailto:sbarati at apple.com>> wrote:

On Dec 14, 2018, at 1:37 PM, Chris Dumez <cdumez at apple.com <mailto:cdumez at apple.com>> wrote:

Hi,

I have now been caught twice by std::optional’s move constructor. It turns out that it leaves the std::optional being moved-out engaged, it merely moves its value.

For example, testOptional.cpp:

#include <iostream>

#include <optional>

int main() { std::optional<int> a = 1; std::optional<int> b = std::move(a); std::cout << "a is engaged? " << !!a << std::endl; std::cout << "b is engaged? " << !!b << std::endl; return 0; }

$ clang++ testOptional.cpp -o testOptional -std=c++17 $ ./testOptional a is engaged? 1 b is engaged? 1

I would have expected: a is engaged? 0 b is engaged? 1 I would have expected this too.

This impacts the standard std::optional implementation on my machine as well as the local copy in WebKit’s wtf/Optional.h.

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. I believe it's UB to use an object after it has been moved. I'm wrong here. Apparently objects are left in a "valid but unspecified state".

stackoverflow.com/questions/32346143/undefined-behavior-with-stdmove, stackoverflow.com/questions/32346143/undefined-behavior-with-stdmove I believe in WebKit, however, we’ve made sure our types are left in a valid “empty” state, thus my proposal for our own optional type that would be less error-prone / more convenient to use.

I think your proposal is reasonable. I agree it's less error prone.

I think if we make this change, we should pull optional out of std and put it in WTF, since we're now implementing behavior we will rely on being specified.

# Adrian Perez de Castro (6 days ago)

Hello,

On Fri, 14 Dec 2018 14:02:39 -0800, Saam Barati <sbarati at apple.com> wrote:

On Dec 14, 2018, at 1:59 PM, Chris Dumez <cdumez at apple.com> wrote:

On Dec 14, 2018, at 1:56 PM, Saam Barati <sbarati at apple.com <mailto:sbarati at apple.com>> wrote:

On Dec 14, 2018, at 1:54 PM, Saam Barati <sbarati at apple.com <mailto:sbarati at apple.com>> wrote:

On Dec 14, 2018, at 1:37 PM, Chris Dumez <cdumez at apple.com <mailto:cdumez at apple.com>> wrote:

Hi,

I have now been caught twice by std::optional’s move constructor. It turns out that it leaves the std::optional being moved-out engaged, it merely moves its value.

For example, testOptional.cpp:

#include <iostream>

#include <optional>

int main() { std::optional<int> a = 1; std::optional<int> b = std::move(a); std::cout << "a is engaged? " << !!a << std::endl; std::cout << "b is engaged? " << !!b << std::endl; return 0; }

$ clang++ testOptional.cpp -o testOptional -std=c++17 $ ./testOptional a is engaged? 1 b is engaged? 1

I would have expected: a is engaged? 0 b is engaged? 1 I would have expected this too.

This is also what I would have expected.

This impacts the standard std::optional implementation on my machine as well as the local copy in WebKit’s wtf/Optional.h.

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. I believe it's UB to use an object after it has been moved. I'm wrong here. Apparently objects are left in a "valid but unspecified state".

stackoverflow.com/questions/32346143/undefined-behavior-with-stdmove, stackoverflow.com/questions/32346143/undefined-behavior-with-stdmove I believe in WebKit, however, we’ve made sure our types are left in a valid “empty” state, thus my proposal for our own optional type that would be less error-prone / more convenient to use. I think your proposal is reasonable. I agree it's less error prone.

I think if we make this change, we should pull optional out of std and put it in WTF, since we're now implementing behavior we will rely on being specified.

I am also in favor of having an implementation in WTF that empties the optional after moving the value out from it.

# youenn fablet (5 days ago)

Is it too late to ask for a std::optional change?

Le ven. 14 déc. 2018 à 13:37, Chris Dumez <cdumez at apple.com> a écrit :

# Fujii Hironori (5 days ago)

On Sat, Dec 15, 2018 at 6:38 AM Chris Dumez <cdumez at apple.com> wrote:

I have now been caught twice by std::optional’s move constructor.

I don't understand how this could be useful? Do you want to use the value after it is moved? I'd like to see these your code. Could you show me these two patches?

# Chris Dumez (5 days ago)

On Dec 14, 2018, at 3:39 PM, Fujii Hironori <fujii.hironori at gmail.com> wrote:

On Sat, Dec 15, 2018 at 6:38 AM Chris Dumez <cdumez at apple.com <mailto:cdumez at apple.com>> wrote:

I have now been caught twice by std::optional’s move constructor.

I don't understand how this could be useful? Do you want to use the value after it is moved? I'd like to see these your code. Could you show me these two patches?

This is the latest one: trac.webkit.org/changeset/239228/webkit

# Chris Dumez (5 days ago)

So to be clear, it is often not truly about using the value after it is moved. It is about expecting that the variable / member has been nulled out after moving it. If I WTFMove() out a data member m_dataMember, I expect later on if (m_dataMember) to be false.

--  Chris Dumez

# Fujii Hironori (5 days ago)

On Sat, Dec 15, 2018 at 8:46 AM Chris Dumez <cdumez at apple.com> wrote:

This is the latest one: trac.webkit.org/changeset/239228/webkit

This expression WTFMove(*m_pendingWebsitePolicies) doesn't move std::optional<WebsitePoliciesData>, but moves the content of the

std::optional, WebsitePoliciesData. I think your proposal doesn't work for this code.

# David Kilzer (5 days ago)
# Geoffrey Garen (5 days ago)

This expression WTFMove(*m_pendingWebsitePolicies) doesn't move std::optional<WebsitePoliciesData>, but moves the content of the std::optional, WebsitePoliciesData. I think your proposal doesn't work for this code.

The original code, which asserted, did this:

    if (auto pendingWebsitePolicies = WTFMove(m_pendingWebsitePolicies))
        WebsitePoliciesData::applyToDocumentLoader(WTFMove(*pendingWebsitePolicies), documentLoader);

The relevant expression was "WTFMove(m_pendingWebsitePolicies)”, which did move std::optional<WebsitePoliciesData>. The expectation in the code was that, after WTFMove(m_pendingWebsitePolicies), m_pendingWebsitePolicies would be empty.

Geoff

# Daniel Bates (3 days ago)

On Dec 14, 2018, at 3:52 PM, Chris Dumez <cdumez at apple.com <mailto:cdumez at apple.com>> wrote:

So to be clear, it is often not truly about using the value after it is moved. It is about expecting that the variable / member has been nulled out after moving it. If I WTFMove() out a data member m_dataMember, I expect later on if (m_dataMember) to be false.

I do not mean to derail the discussion about whether or not we should have our own std::optional that disengages on move. I am all for convenience. I think expecting a valid “empty” state (*) when moving out a data member is an anti-pattern and I propose that we should be using std::exchange() instead (or any code analogous to doing std::exchange()) for these cases, including the case in your original email and especially with data members as in <https://trac.webkit.org/changeset/239228 <https://trac.webkit.org/changeset/239228>>.

Why do I consider it an anti-pattern? See (*) and because the concept of “moving" is not spec'ed to behave like this. Here’s one reference to the spec’ed behavior and an example comparable to the one in your first email (highlighting is my own for emphasis):

[[ Unless otherwise specified, all standard library objects that have been moved from are placed in a valid but unspecified state. That is, only the functions without preconditions, such as the assignment operator, can be safely used on the object after it was moved from: std::vector en.cppreference.com/w/cpp/container/vector<std::string <http://en.cppreference.com/w/cpp/string/basic_string>> v;

std::string en.cppreference.com/w/cpp/string/basic_string str = "example";

v.push_back(std::move(str)); // str is now valid but unspecified str.back(); // undefined behavior if size() == 0: back() has a precondition !empty() str.clear(); // OK, clear() has no preconditions

]] <https://en.cppreference.com/w/cpp/utility/move <https://en.cppreference.com/w/cpp/utility/move>>

(*) What is the valid “empty” state for a type without a default constructor?

Dan

# Fujii Hironori (3 days ago)

I don't like the proposal because it encourages misuse of move. We can use move only for values about to be destroyed.

I like Dan's suggestion. We should use std::exchange or std::optional::swap for the cases. Or, what about adding a new method WTF::Optional::release() for the case?

# Chris Dumez (3 days ago)

On Dec 16, 2018, at 7:43 PM, Fujii Hironori <fujii.hironori at gmail.com> wrote:

I don't like the proposal because it encourages misuse of move. We can use move only for values about to be destroyed.

Just for reference, there are close to 400 matches for "WTFMove(m_” in our code base. People do seem to rely on the state of objects after being moved out. I totally agree that the state of the object being moved out is not defined by the C++ standard. However, so far, in WebKit, we’ve been careful with our move-constructors.

I think that if we do not update std::optional’s move constructor, then I worry we’ll keep having to fix bugs in the future due to its misuse. Although, maybe this mail thread will help.

That being said, I agree with your and Daniel and we should use std::exchange more. I think all the "WTFMove(m_” lines in our code bases should probably be replaced with std::exchange.

# Fujii Hironori (3 days ago)

I've changed my mind.

It is ensured std::vector move constructor makes moved value empty.

After the move, other is guaranteed to be empty().

en.cppreference.com/w/cpp/container/vector/vector

Unfortunately, WTF::Vector move constructor just swaps values. I think this swapping is not a specified behavior WebKit developers can rely on.

trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/Vector.h?rev=238031#L950

I think WTF::Vector also should ensure moved values become empty.

Looking through the grepping "WTFMove(m_”result, some of them are valid because:

  1. It is in dtor. 'this' object is about to be destructed.
  2. It is in ref-qualified method. 'this' object is about to be destructed.
  3. It is Ref<>. As well as std::shared_ptr, it should be defined to become

null after moved.

Common misuse cases are moving Vector like following code.

void SVGDocumentExtensions::rebuildElements()

{ Vector<SVGElement*> shadowRebuildElements = WTFMove(m_rebuildElements);

I thought std::exchange should be used for these purpose. But, if WTF::Vector move constructor is changed, this code would be valid. m_rebuildElements would become empty as expected, not accidentally.

If above code would be valid, there would be no reason to object we have custom Optional class which guarantees moved values become empty.

BWT, FWIW, clang-tidy has use-after-move checker.

clang-tidy - bugprone-use-after-move — Extra Clang Tools 8 documentation clang.llvm.org/extra/clang-tidy/checks/bugprone-use-after-move.html

As expected, it doesn't support member variables.

The check currently only considers calls of std::move on local variables or

function parameters. It does not check moves of member variables or global variables.

It seems UBSan doesn't check use-after-move.

UndefinedBehaviorSanitizer — Clang 8 documentation clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#ubsan

# Maciej Stachowiak (3 days ago)

On Dec 16, 2018, at 3:47 PM, Daniel Bates <dbates at webkit.org> wrote:

On Dec 14, 2018, at 3:52 PM, Chris Dumez <cdumez at apple.com <mailto:cdumez at apple.com>> wrote:

So to be clear, it is often not truly about using the value after it is moved. It is about expecting that the variable / member has been nulled out after moving it. If I WTFMove() out a data member m_dataMember, I expect later on if (m_dataMember) to be false.

I do not mean to derail the discussion about whether or not we should have our own std::optional that disengages on move. I am all for convenience. I think expecting a valid “empty” state (*) when moving out a data member is an anti-pattern and I propose that we should be using std::exchange() instead (or any code analogous to doing std::exchange()) for these cases, including the case in your original email and especially with data members as in <https://trac.webkit.org/changeset/239228 <https://trac.webkit.org/changeset/239228>>.

Expectations can’t be an anti-pattern, only code can be an anti-pattern. At the point of checking whether a value is “empty” by some definition, you may not know that someone has std::move()d it, and shouldn’t have to. So to deal with the bug-prone nature of check after std::move(), we can do one of these:

(1) Ban all use of std::move() unless it’s possible for some sort of static check to know that no one will examine the moved value (2) Make sure we only use types that leave themselves in a state that meets expectations post-move(). (3) Try real hard not to make mistakes and hope for the best

Maybe there is an option I am missing? Out of these, I think (2) is likely the best, on the whole.

Why do I consider it an anti-pattern? See (*) and because the concept of “moving" is not spec'ed to behave like this. Here’s one reference to the spec’ed behavior and an example comparable to the one in your first email (highlighting is my own for emphasis):

[[ Unless otherwise specified, all standard library objects that have been moved from are placed in a valid but unspecified state. That is, only the functions without preconditions, such as the assignment operator, can be safely used on the object after it was moved from:

Note that this is specified for “standard library objects”. There’s nothing wrong with choosing to give a stronger API contract for our own objects, and then to rely on it.

# Maciej Stachowiak (3 days ago)

On Dec 16, 2018, at 8:37 PM, Chris Dumez <cdumez at apple.com> wrote:

On Dec 16, 2018, at 7:43 PM, Fujii Hironori <fujii.hironori at gmail.com> wrote:

I don't like the proposal because it encourages misuse of move. We can use move only for values about to be destroyed.

Just for reference, there are close to 400 matches for "WTFMove(m_” in our code base. People do seem to rely on the state of objects after being moved out. I totally agree that the state of the object being moved out is not defined by the C++ standard. However, so far, in WebKit, we’ve been careful with our move-constructors.

I think that if we do not update std::optional’s move constructor, then I worry we’ll keep having to fix bugs in the future due to its misuse. Although, maybe this mail thread will help.

That being said, I agree with your and Daniel and we should use std::exchange more. I think all the "WTFMove(m_” lines in our code bases should probably be replaced with std::exchange.

I think it would be easier to enforce a rule of “always use WTF::Optional instead of std::optional” than a rule of “use std::exchange more, but sometimes you really need WTFMove(), but don’t use move when it would be wrong to do so”. Better to set up the code to create simple rules that don’t require judgment IMO.

# Fujii Hironori (3 days ago)

On Mon, Dec 17, 2018 at 4:55 PM Maciej Stachowiak <mjs at apple.com> wrote:

Maybe there is an option I am missing? Out of these, I think (2) is likely the best, on the whole.

Reasonable. But, some classes need to be modification for that. For example, WTF::URL is using default move constructor. I think we should add the move constructor to restore the valid null value after move.

# Alex Christensen (3 days ago)

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.

On Dec 14, 2018, at 3:20 PM, youenn fablet <youennf at gmail.com> wrote:

Is it too late to ask for a std::optional change?

Yes

# Chris Dumez (3 days ago)

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 <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.

# Chris Dumez (3 days ago)

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 <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: trac.webkit.org/changeset/236463/webkit

# Alex Christensen (3 days ago)

This one and the many others like it are fragile, relying

# Geoffrey Garen (3 days ago)

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

# Ryosuke Niwa (3 days ago)

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
# Alex Christensen (2 days ago)

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.

# Chris Dumez (2 days ago)

On 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.

Yes, this is a good example of case where std::exchange should be used or where the parameter should be a RefPtr<Test> and not a RefPtr<Test>&&. But as I mentioned earlier, the issue here is that the caller of WTFMove() is not actually calling any move constructor (it is merely doing a cast) and therefore cannot rely on the effects or a move constructor.

I do not think that this example is a good reason why we would not want optional to have a more predictable move constructor.

# Ryosuke Niwa (2 days ago)

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:

# Ryosuke Niwa (2 days ago)

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
# Alex Christensen (2 days ago)

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.

# Geoffrey Garen (2 days ago)

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

# Maciej Stachowiak (2 days ago)

Right now, after a maybeMove type call on std::optional, you get either the original value, or a value that is officially valid but unspecified, and actually an optional that says it contains a value but doesn’t. With Chris’s proposal, you’d get a WTF::Optional with either the original value, or an optional that doesn’t contain a value and says it doesn’t. Among other things, this allows for a “did anything actually get left here” check after the function that may or may not move a value. Seems like an upgrade.

# Fujii Hironori (2 days ago)

On Tue, Dec 18, 2018 at 11:16 AM Maciej Stachowiak <mjs at apple.com> wrote:

Among other things, this allows for a “did anything actually get left here” check after the function that may or may not move a value. Seems like an upgrade.

Don't recommend such checks. It is simply use-after-move. The expression WTFMove(x) means marking x as disposable.

On Tue, Dec 18, 2018 at 8:55 AM Alex Christensen <achristensen at apple.com>

wrote:

I think we should do something to track and prevent the use of moved-from values instead of introducing our own std::optional replacement.

Do you have a idea?

# Ryosuke Niwa (2 days ago)

On Mon, Dec 17, 2018 at 6:40 PM Fujii Hironori <fujii.hironori at gmail.com>

wrote:

On Tue, Dec 18, 2018 at 11:16 AM Maciej Stachowiak <mjs at apple.com> wrote:

>

Among other things, this allows for a “did anything actually get left here” check after the function that may or may not move a value. Seems like an upgrade.

Don't recommend such checks. It is simply use-after-move. The expression WTFMove(x) means marking x as disposable.

That's simply not true. The semantics of WTFMove / std::move is really that of a type cast. I think std::move was ill-named. It really should have been called std::movable_cast or something. Also, as Geoff has already pointed out, the behavior of STL doesn't prevent us from writing our own template library to always have a very well specified & good state after std::move'ed & its value was move-constructed.

It's one thing to consider that new WebKit contributors who are familiar with C++ semantics of std::move and std::optional getting confused by WebKit's implementation of std::option. But then our behavior of HashMap which doesn't accept the POD integral value of 0 as a key, and how Ref and RefPtr work and they're used by our WebCore code is probably more problematic than this std::optional behavior.

In general, I find these kinds of dogmatic adoption of the best practices / recommended ways of coding to be highly problematic. We shouldn't care who or what entity recommends writing code in one way or another. We should be questioning every and each recommendation anyone makes, and judging for ourselves whether it makes a sense for the WebKit project.

For example, there is a group of people who swear by always wrapping each single statement with { ~ } as a way of preventing bugs which stems from forgetting { ~ } around multiple statements. Yet in my nine years of writing & reviewing code in WebKit, I've never once came across such a mistake made by either me or any other contributor.

  • R. Niwa
# Antti Koivisto (2 days ago)

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

# Fujii Hironori (2 days ago)

On Tue, Dec 18, 2018 at 12:25 PM Ryosuke Niwa <ryosuke.niwa at gmail.com>

wrote:

Also, as Geoff has already pointed out, the behavior of STL doesn't prevent us from writing our own template library to always have a very well specified & good state after std::move'ed & its value was move-constructed.

Do you mean only WTF classes should guarantee the valid empty state after moved-out? Should we use std::exchange and std::move properly for other classes in WebKit?

# Fujii Hironori (2 days ago)

On Tue, Dec 18, 2018 at 10:21 PM Fujii Hironori <fujii.hironori at gmail.com>

wrote:

On Tue, Dec 18, 2018 at 12:25 PM Ryosuke Niwa <ryosuke.niwa at gmail.com> wrote:

Also, as Geoff has already pointed out, the behavior of STL doesn't prevent us from writing our own template library to always have a very well specified & good state after std::move'ed & its value was move-constructed.

Do you mean only WTF classes should guarantee the valid empty state after moved-out? Should we use std::exchange and std::move properly for other classes in WebKit?

I guess the answer is all classes in WebKit which are moved by WTFMove. I'm wondering how much runtime cost it will take to restore valid empty state of all moved-out values.

# Michael Catanzaro (2 days ago)

I know I'm getting a bit far afield here, but:

On Mon, Dec 17, 2018 at 9:26 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:

But then our behavior of HashMap which doesn't accept the POD integral value of 0 as a key

This behavior is really unexpected and dangerous [1], and we should seriously consider changing it. No doubt lots of bugs caused by this are just waiting to be uncovered. I've been working on WebKit since 2014 and didn't know about this until last month.

Another oddity: I recently learned that AtomicStrings are actually interned strings. WTF. Why not call them that? I had thought for years that they were strings safe to be shared across threads, like other atomics. Not at all. Maybe this was dumb of me, but it could have been avoided by better naming.

Michael

[1] trac.webkit.org/changeset/238407/webkit

# Ryosuke Niwa (2 days ago)

On Tue, Dec 18, 2018 at 11:35 AM Michael Catanzaro <mcatanzaro at igalia.com>

wrote:

I know I'm getting a bit far afield here, but:

On Mon, Dec 17, 2018 at 9:26 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:

But then our behavior of HashMap which doesn't accept the POD integral value of 0 as a key

This behavior is really unexpected and dangerous [1], and we should seriously consider changing it. No doubt lots of bugs caused by this are just waiting to be uncovered. I've been working on WebKit since 2014 and didn't know about this until last month.

I tend to agree but then we'd come up with other numbers for the empty & deleted values. I've been thinking that we could use -1 and -2 but that's also somewhat arbitrary restriction.

Another oddity: I recently learned that AtomicStrings are actually

interned strings. WTF. Why not call them that? I had thought for years that they were strings safe to be shared across threads, like other atomics. Not at all. Maybe this was dumb of me, but it could have been avoided by better naming.

This topic has been discussed extensively in the past: lists.webkit.org/pipermail/webkit-dev/2013-June/024988.html

  • R. Niwa
# Konstantin Tokarev (2 days ago)

18.12.2018, 22:35, "Michael Catanzaro" <mcatanzaro at igalia.com>:

I know I'm getting a bit far afield here, but:

On Mon, Dec 17, 2018 at 9:26 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:

But then our behavior of HashMap which doesn't accept the POD integral value of 0 as a key

This behavior is really unexpected and dangerous [1], and we should seriously consider changing it. No doubt lots of bugs caused by this are just waiting to be uncovered. I've been working on WebKit since 2014 and didn't know about this until last month.

Another oddity: I recently learned that AtomicStrings are actually interned strings. WTF. Why not call them that? I had thought for years that they were strings safe to be shared across threads, like other atomics. Not at all. Maybe this was dumb of me, but it could have been avoided by better naming.

I agree that "atomic" part of AtomicString is kinda misleading, however wiki explains it all

trac.webkit.org/wiki/EfficientStrings#AtomicStringVSString

BTW, /me personally didn't know what "interned string" is until today :)

# Fujii Hironori (a day ago)

On Wed, Dec 19, 2018 at 6:41 AM Konstantin Tokarev <annulen at yandex.ru>

wrote:

I agree that "atomic" part of AtomicString is kinda misleading, however wiki explains it all

trac.webkit.org/wiki/EfficientStrings#AtomicStringVSString

BTW, /me personally didn't know what "interned string" is until today :)

The return value of RegisterClass Win32 API is ATOM. And, Lisp has the similar concepts of intern and atom. I've never thought AtomicString is a confusing name until I see you comments today. It is a intrinsic name for me as a Win32 and Lisp programmer.

docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-addatoma, docs.microsoft.com/en-us/windows/desktop/api/winuser/nf

Want more features?

Request early access to our private beta of readable email premium.