Re: [PATCH 2/2] Introduce __cond_lock_err

[Luc Van Oostenryck]

On Fri, Dec 22, 2017 at 04:31:12AM -0800, Matthew Wilcox wrote:
> On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote:
> 
> While I've got you, I've been looking at some other sparse warnings from
> this file.  There are several caused by sparse being unable to handle
> the following construct:
> 
> 	if (foo)
> 		x = NULL;
> 	else {
> 		x = bar;
> 		__acquire(bar);
> 	}
> 	if (!x)
> 		return -ENOMEM;
> 
> Writing it as:
> 
> 	if (foo)
> 		return -ENOMEM;
> 	else {
> 		x = bar;
> 		__acquire(bar);
> 	}
> 
> works just fine.  ie this removes the warning:

It must be noted that these two versions are not equivalent
(in the first version, it also returns with -ENOMEM if bar
is NULL/zero).
 
It must be noted that sparse's goal regarding the context imbalance
is to give the warning if some point in the code can be reached via
two paths (or more) and the lock state (the context) is not identical
in each of these paths.

> 
> Is there any chance sparse's dataflow analysis will be improved in the
> near future?

A lot of functions in the kernel have this context imbalance,
really a lot. For example, any function doing conditional locking
is a problem here. Happily when these functions are inlined,
sparse, thanks to its optimizations, can remove some paths and
merge some others. 
So yes, by adding some smartness to sparse, some of the false
warnings will be removed, however:
1) some __must_hold()/__acquires()/__releases() annotations are
   missing, making sparse's job impossible.
2) a lot of the 'false warnings' are not so false because there is
   indeed two possible paths with different lock state
3) it has its limits (at the end, giving the correct warning is
   equivalent to the halting problem).

Now, to answer to your question, I'm not aware of any effort that would
make a significant differences (it would need, IMO, code hoisting & 
value range propagation).

-- Luc Van Oostenryck