Issue with std::lock_guard

[llvmbot]

Bugzilla Link	40016
Version	unspecified
OS	All
Attachments	Repro case
Reporter	LLVM Bugzilla Contributor
CC	@AaronBallman,@aaronpuchert,@dwblaikie,@zygoloid

Extended Description

I have found a possible bug in the use of std::lock_guard, although it may simply be a missing warning.

The issue is if a programmer types

std::mutex mtx; std::lock_guard(mtx);

when they meant to have the second line read:

std::lock_guard lg(mtx);

In the first case, the mutex doesn't lock (or possibly only locks for the duration of the statement).

This first case may be legal C++ (I'm not a language lawyer, so I can't tell), but at the very least, I think this merits a warning - the programmer probably meant to lock the mutex for the duration of the current block.

I was compiling with: -std=c++11 -Wall -Wextra -Wthread-safety -Wthread-safety-verbose

And: c++ --version Apple LLVM version 9.1.0 (clang-902.0.39.1) Target: x86_64-apple-darwin17.7.0 Thread model: posix

I have submitted this bug to Apple (Issue 45790820), but there has been no action on it.

The attached file test.cpp demonstrates the issue - compare lines 11 and 21.

I expected output like: $ ./LockTest ShowId: 0x70000cb8a000 ShowId: 0x70000cc0d000 RegularGuard: Starting wait RegularGuard: Ending wait AnonGuard: Starting wait AnonGuard: Ending wait RegularGuard: Starting wait RegularGuard: Ending wait AnonGuard: Starting wait AnonGuard: Ending wait RegularGuard: Starting wait RegularGuard: Ending wait etc.

However, the output I get is: $ ./LockTest ShowId: 0x70000cb8e000 ShowId: 0x70000cc11000 RegularGuard: Starting wait RegularGuard: Ending wait AnonGuard: Starting wait RegularGuard: Starting wait RegularGuard: Ending wait RegularGuard: Starting wait RegularGuard: Ending wait RegularGuard: Starting wait RegularGuard: Ending wait RegularGuard: Starting wait RegularGuard: Ending wait RegularGuard: Starting wait RegularGuard: Ending wait RegularGuard: Starting wait RegularGuard: Ending wait RegularGuard: Starting wait RegularGuard: Ending wait RegularGuard: Starting wait RegularGuard: Ending wait RegularGuard: Starting wait RegularGuard: Ending wait RegularGuard: Starting wait AnonGuard: Ending wait RegularGuard: Ending wait AnonGuard: Starting wait etc.

If I change line 21 to match line 11, then I get the expected output.

[aaronpuchert]

(void)std::lock_guard(), x = 1; (void)std::lock_guard(mtx), x = 1;

[aaronpuchert]

Thought about adding [[nodiscard]] on std::lock_guard. Not sure if the standard allows that. At least with Clang that produces a warning (not with GCC though):

warning: ignoring temporary created by a constructor declared with 'nodiscard' attribute [-Wunused-value] std::lock_guard(mtx); ^~~~~~~~~

Though technically it's not necessarily wrong to discard the guard, for example:

std::lock_guard(mtx), x = 1;

That would have the lock for the duration of the assignment. The warning could be fixed with

(void)std::lock_guard(), x = 1;

But in some sense we'd be abusing [[nodiscard]], because we don't actually care about the value. We want to discard it, just not now.

Potentially we could mix & match - I wonder how many false positives we would find if we used the existing scoped_lockable attribute to imply something like the second category of unused I described above ("has side effects, but only between construction and destruction" -> "warn if no statements occur between construction and destruction")? Sounds interesting, though even there I'm not entirely sure. Locking and immediately unlocking has an effect: we wait for whoever had the lock before. Not sure if this is used anywhere, but I can't for certain say that such a pattern doesn't make sense.

[dwblaikie]

Potentially we could mix & match - I wonder how many false positives we would find if we used the existing scoped_lockable attribute to imply something like the second category of unused I described above ("has side effects, but only between construction and destruction" -> "warn if no statements occur between construction and destruction")?

[aaronpuchert]

I think there are two ways to approach this. One is to say that this is a useless statement, since the mutex is immediately released upon acquisition, and treat it with a -Wunused-* warning. David has already pointed out why that is difficult.

One could argue there's nothing wrong with this code. It doesn't make a lot of sense, but it's not a bug in itself. (Maybe a performance issue.)

A problem arises if some operations follow where we assume the mutex is held (but in this case isn't). Then and only then can -Wthread-safety help you. If you happen to use libcxx (which has thread safety annotations on std::lock_guard) you should get a warning then. Example:

define _LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS

include

std::mutex mtx; int foo attribute((guarded_by(mtx)));

void f() { std::lock_guard{mtx}; foo = 0; // writing variable 'foo' requires holding mutex 'mtx' exclusively [-Wthread-safety-analysis] }

[dwblaikie]

Yeah, it's a bit hard to warn on unused variables with non-trivial ctors and dtors - hard to know what side effects are intended or not.

For some types, even "foo(x);" would be a fine statement because the side effects of the ctor are significant/interesting.

For some types, "{ foo f; y(); }" is acceptable because the side effects of the ctor are undone by the dtor - but are significant during the execution of y()

And for some types (like std::string, etc) even that's not enough, and there should be some statement that references 'f' between its construction and destruction ("{ foo f; z(f); }").

My preference here would be to have two attributes (or a parameterized attribute) that annotates classes of the first and second kind (leaving the default to be value-types like std::string not needing any annotation, because they're the majority) and a warning that warns appropriately for all 3 types - accepts the first one always, warns on the second if there's nothing in between construction and destruction (eg: "foo(x);" or "{ foo f(x); }", etc, but not "foo(x), y();" or the more obvious two-statement example above), and warns on all the usual cases for the third/normal value types.

This warning would have a lot of false positives in a codebase that wasn't already using the annotation, which is unfortunate/difficult. But it could start out as an off-by-default warning, or as a clang-tidy check & enabled in codebases that are willing to do the cleanup of annotating their types (& would have to start with the standard library). It could even offer fixits to add the annotation & farm out applying the annotation - and then let reviewers/developers cleanup the codebase by removing unused variables then removing/downgrading the attributes to enforce that requirement going forward