Fix object usage after move in make_delegate_with_shared_state

[yuvaln-s1]

The function make_delegate_with_shared_state (from strings/base_coroutine_foundation.h) uses on object after it was moved. 'd' is moved using std::move and then it's passed to get_abi. The usage of 'd' after it was moved it undefined.

[kennykerr]

Thanks!

[tim-weis]

I'm not a C++ Language Lawyer, but I believe the problem statement to be incorrect.

'd' is moved using std::move

That's not what std::move actually does:

std::move is used to indicate that an object t may be "moved from" [...]. It is exactly equivalent to a static_cast to an rvalue reference type.

As I understand it this means that when std::move returns, the object passed as an argument has not yet been moved from. It merely had its type changed making it eligible to be moved from at some point in the future. That future here is no sooner than entering the std::pair constructor, which runs only after all of its arguments have been evaluated.

Even if it had been moved already, the following isn't true, either:

The usage of 'd' after it was moved it undefined.

C++ doesn't have "destructive moves". An object that has been moved from must remain in a valid (even if unspecified) state. Subsequent use is thus well defined; the behavior may be unexpected, still. But, unless my interpretation is wrong, we haven't moved yet, so this is moot.

While I don't have an issue with the proposed changes¹, it would be helpful for clients—especially those dealing with an issue in their code—to know, whether this change is fixing a genuine bug in the C++/WinRT library code. Can we get a confirmation on this?

Footnotes

1. On second thought, this change may have actually disabled the ability to move out of d altogether. With abi no longer in the xvalue category, I don't know whether std::pair's "move" constructor is still being considered during overload resolution. A quick test seems to confirm that it isn't (previous compiles, current errors out). ↩

[yuvaln-s1]

@tim-weis thanks for your comment. Your explanation about std::move and the fact that everything is actually well defined is correct. Sorry for my inaccuracy there.

Regarding the possibility for an actual bug and your comment about not being able to move 'd' out of the function due to the overload resolution:

1. 'get_abi' returns a pointer to the underlying data of 'd'. Therefore, if the first object in the pair will be constructed first using a move constructor with 'd' as the parameter to the function, 'd' will be moved and then the data pointed to by the pointer received from 'get_abi' will probably won't be the expected data.

2. From what I've seen and experimented, std::move uses perfect forwarding for the construction of each object in the pair. Therefore, the fact that the second object in the pair will now be constructed using a copy constructor instead of a move constructor, won't affect the move construction of the first object.

[tim-weis]

@yuvaln-s1 The first point is technically correct, but not an issue in practice. make_delegate() returns a winrt::com_ptr (I guess), and as long as that is still alive (even as an xvalue), its wrapped interface pointer (the value returned by get_abi()) remains valid. The order in which a std::pair is constructed from d and abi doesn't make a difference, as the reference count on the abi interface cannot ever drop to zero.

As for the second point, I'm probably not understanding things. As far as I can see, std::pair can be constructed either from rvalue references, or lvalue references, but not a mix of those. If either argument isn't an xvalue, then the constructor accepting lvalue references only is selected. Am I overlooking something here?

[kennykerr]

I doubt the change makes any practical difference, but merely clarifies. Tim, if you find a regression I'm happy to roll it back or you can offer a fix.

[yuvaln-s1]

@tim-weis I understand your point about my first point.
Regarding your comment on my second point:
std::pair has a constructor that uses perfect forwarding. That allows the std::pair to be constructed from a mix of rvalue and lvalue references. Each object in the pair will be constructed using the specific reference for it.

Here's the relevant std::pair ctor:
enable_if_t<conjunction_v<is_constructible<_Ty1, _Other1>, is_constructible<_Ty2, _Other2>>, int> = 0> constexpr explicit(!conjunction_v<is_convertible<_Other1, _Ty1>, is_convertible<_Other2, _Ty2>>) pair(_Other1&& _Val1, _Other2&& _Val2) noexcept( is_nothrow_constructible_v<_Ty1, _Other1>&& is_nothrow_constructible_v<_Ty2, _Other2>) // strengthened : first(_STD forward<_Other1>(_Val1)), second(_STD forward<_Other2>(_Val2)) { }

[tim-weis]

@yuvaln-s1 Thank you, that's the part I was missing. This does indeed allow passing a mix of lvalue and rvalue references, and still have the rvalue references participate in move-construction (verified here; Clang agrees, too).

@kennykerr The primary concern was that this PR (and its title) will eventually make it into the release notes of a future release. If there was no defect then the release notes shouldn't claim that something were fixed. I was asking for confirmation on whether there was or wasn't a defect.