Avoiding self assignment in std::shuffle

Question

I stumbled upon the following problem when using the checked implementation of glibcxx:

/usr/include/c++/4.8.2/debug/vector:159:error: attempt to self move assign.
Objects involved in the operation:
sequence "this" @ 0x0x1b3f088 {
  type = NSt7__debug6vectorIiSaIiEEE;
}

Which I have reduced to this minimal example:

#include <vector>
#include <random>
#include <algorithm>

struct Type {
        std::vector<int> ints;
};

int main() {
        std::vector<Type> intVectors = {{{1}}, {{1, 2}}};
        std::shuffle(intVectors.begin(), intVectors.end(), std::mt19937());
}

Tracing the problem I found that shuffle wants to std::swap an element with itself. As the Type is user defined and no specialization for std::swap has been given for it, the default one is used which creates a temporary and uses operator=(&&) to transfer the values:

  _Tp __tmp = _GLIBCXX_MOVE(__a);
  __a = _GLIBCXX_MOVE(__b);
  __b = _GLIBCXX_MOVE(__tmp);

As Type does not explicitly give operator=(&&) it is default implemented by "recursively" applying the same operation on its members.

The problem occurs on line 2 of the swap code where __a and __b point to the same object which results in effect in the code __a.operator=(std::move(__a)) which then triggers the error in the checked implementation of vector::operator=(&&).

My question is: Who's fault is this?

• Is it mine, because I should provide an implementation for swap that makes "self swap" a NOP?
• Is it std::shuffle's, because it should not try to swap an element with itself?
• Is it the checked implementation's, because self-move-assigment is perfectly fine?
• Everything is correct, the checked implementation is just doing me a favor in doing this extra check (but then how to turn it off)?

I have read about shuffle requiring the iterators to be ValueSwappable. Does this extend to self-swap (which is a mere runtime problem and can not be enforced by compile-time concept checks)?

Addendum

To trigger the error more directly one could use:

#include <vector>

int main() {
    std::vector<int> vectorOfInts;
    vectorOfInts = std::move(vectorOfInts);
}

Of course this is quite obvious (why would you move a vector to itself?). If you where swapping std::vectors directly the error would not occur because of the vector class having a custom implementation of the swap function that does not use operator=(&&).

Answer 1

The libstdc++ Debug Mode assertion is based on this rule in the standard, from [res.on.arguments]

If a function argument binds to an rvalue reference parameter, the implementation may assume that this parameter is a unique reference to this argument.

i.e. the implementation can assume that the object bound to the parameter of T::operator=(T&&) does not alias *this, and if the program violates that assumption the behaviour is undefined. So if the Debug Mode detects that in fact the rvalue reference is bound to *this it has detected undefined behaviour and so can abort.

The paragraph contains this note as well (emphasis mine):

[Note: If a program casts an lvalue to an xvalue while passing that lvalue to a library function (e.g., by calling the function with the argument std::move(x)), the program is effectively asking that function to treat that lvalue as a temporary object. The implementation is free to optimize away aliasing checks which might be needed if the argument was an lvalue. —end note]

i.e. if you say x = std::move(x) then the implementation can optimize away any check for aliasing such as:

X::operator=(X&& rval) { if (&rval != this) ...

Since the implementation can optimize that check away, the standard library types don't even bother doing such a check in the first place. They just assume self-move-assignment is undefined.

However, because self-move-assignment can arise in quite innocent code (possibly even outside the user's control, because the std::lib performs a self-swap) the standard was changed by Defect Report 2468. I don't think the resolution of that DR actually helps though. It doesn't change anything in [res.on.arguments], which means it is still undefined behaviour to perform a self-move-assignment, at least until issue 2839 gets resolved. It is clear that the C++ standard committee think self-move-assignment should not result in undefined behaviour (even if they've failed to actually say that in the standard so far) and so it's a libstdc++ bug that our Debug Mode still contains assertions to prevent self-move-assignment.

Until we remove the overeager checks from libstdc++ you can disable that individual assertion (but still keep all the other Debug Mode checks) by doing this before including any other headers:

#include <debug/macros.h>
#undef __glibcxx_check_self_move_assign
#define __glibcxx_check_self_move_assign(x)

Or equivalently, using just command-line flags (so no need to change the source code):

-D_GLIBCXX_DEBUG -include debug/macros.h -U__glibcxx_check_self_move_assign '-D__glibcxx_check_self_move_assign(x)='

This tells the compiler to include <debug/macros.h> at the start of the file, then undefines the macro that performs the self-move-assign assertion, and then redefines it to be empty.

(In general defining, undefining or redefining libstdc++'s internal macros is undefined and unsupported, but this will work, and has my blessing).

Answer 2

It is a bug in GCC's checked implementation. According to the C++11 standard, swappable requirements include (emphasis mine):

17.6.3.2 §4 An rvalue or lvalue t is swappable if and only if t is swappable with any rvalue or lvalue, respectively, of type T

Any rvalue or lvalue includes, by definition, t itself, therefore to be swappable swap(t,t) must be legal. At the same time the default swap implementation requires the following

20.2.2 §2 Requires: Type T shall be MoveConstructible (Table 20) and MoveAssignable (Table 22).

Therefore, to be swappable under the definition of the default swap operator self-move assignment must be valid and have the postcondition that after self assignment t is equivalent to it's old value (not necessarily a no-op though!) as per Table 22.

Although the object you are swapping is not a standard type, MoveAssignable has no precondition that rv and t refer to different objects, and as long as all members are MoveAssignable (as std::vector should be) the generate move assignment operator must be correct (as it performs memberwise move assignment as per 12.8 §29). Furthermore, although the note states that rv has valid but unspecified state, any state except being equivalent to it's original value would be incorrect for self assignment, as otherwise the postcondition would be violated.