https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89417
--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Federico Kircheis from comment #3)
> My guess, without having looked at the implementation of std::lock, is that
> the algorithm uses try_lock to eventually lock/unlock the mutexes and see if
> it manages to lock both, in order to avoid the deadlock.
Right. So the potential deadlock helgrind complains about can't happen.
> But even if std::lock would be correct (and it is a false positive of
> valgrind), wouldn't sorting by address be an optimization?
Not in the uncontended case, no, because it does extra work to sort them. Your
make_lock function doesn't work for more than two arguments so a more complex
algorithm is needed for the general case (it doesn't even work for fewer than
two arguments, although obviously that's trivial to support).
> As far as I understand, if the mutexes are always locked in the same order,
> one does not need to try_lock.
That's not true. As I already said above, another thread could be locking the
same mutexes without using std::lock (in any order), or it could be locking a
different but overlapping set of mutexes.
> I'm therefore not saying the current algorithm should be dismissed, just
> asking if ordering before applying the algorithm could have any benefit.
> Apart from the fact that helgrind would not complain anymore, which IMHO
> would already be a big win.
Changing the locking algorithm to avoid a false positive from a particular tool
seems unwise.
> Maybe there are known drawbacks to sorting by address, but as mutexes cannot
> be moved,
std::mutex cannot be moved, but std::lock and std::scoped_lock don't only work
with standard mutexes. You have no idea whether foo::Mutex or bar::Lockable can
be moved. You can't even assume that the address is meaningful, I could write a
copyable mutex where different copies share a lock:
class CopyableMutex
{
std::shared_ptr<std::mutex> m = std::make_shared<std::mutex>();
public:
CopyableMutex() = default;
CopyableMutex(const CopyableMutex&) = default;
void lock() { m->lock(); }
bool try_lock() { return m->try_lock(); }
void unlock() { m->unlock(); }
};
