https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89417
--- Comment #5 from Federico Kircheis <federico.kircheis at gmail dot com> ---
(In reply to Jonathan Wakely from comment #4)
> (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.
Ok, I missed that confirmation.
Mine where only suppositions, I was not sure.
I would normally use a tool to validate such claims in a code-basis (as my
expertise with mutlithreading is very limited), unfortunately it produced the
log I've attached in my first report :-)
> > 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).
Well, for the generic case I though a sort based on address would suffice.
My "implementation" was just the special-case for 2 arguments.
As it would be abnormal to lock a lot (with a lot >=5) mutexes, I guessed the
overhead of sorting would be minimal compared to locking.
> > 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.
Do you know other independent tools that can diagnose possible
deadlocks/race-conditions?
I'm genuinely curious (sorry if this is gettin off-topic).
It could have helped avoid creating this issue if valgrind was the only one
complaining (tools working on windows are fine too).
On the other hand, if other tools complains too, it might make more sense (at
least from my perspective) to accommodate third-party analyzers (while of
course they should be fixed too).
> > 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(); }
> };
Oh, I did not realize that the locking classes works on custom types, I guess
that settles the discussion.
I could of course argue that it is possible to sort only for the standard
mutexes, but I see that it is more work than I thought with very little benefit
(especially if other tools do not complain).
