Re: [PATCH 1/3] make sparse keep its promise about context tracking

[Josh Triplett <josh@freedesktop.org>]

[-- Attachment #1: Type: text/plain, Size: 2208 bytes --]

Johannes Berg wrote:
>>>     static void *dev_mc_seq_start(struct seq_file *seq, loff_t * pos)
>>>             __acquires(dev_base_lock)
>>>     {
>>>             [...]
>>>             __acquire__(dev_base_lock);
>>>             read_lock(&dev_base_lock);
>>>             [...]
>>>     }
>> I don't understand why you count this as wrong.  read_lock should
>> handle acquiring the context itself, either via an __acquires
>> annotation if written as a function, or via an __acquire__ statement
>> if written as a macro.  Callers of read_lock shouldn't need to
>> explicitly call __acquire__.
> 
> Well, the question is how you want to name things. What I did is that
> the context identifier is just a name and bears no other relation to the
> code. Hence, read_lock() can't say that it acquires 'dev_base_lock'
> because it doesn't know what the name should be.
> 
> With a slightly different sparse patch than mine you could declare
> read_lock() something like this:
> 
> #define read_lock(x) do { \
> 	__acquire__(x); \
> 	__read_lock((x)); \
> } while (0)
> 
> but then you'd have different names everywhere, say somebody passed
> '&dev_base_lock' and somebody else used
> 
> readlock_t *dbl = &dev_base_lock;
> read_lock(dbl)
> 
> and you'd end up with one context named "dbl" and another one named
> "&dev_base_lock".

That might still work, depending on how consistently kernel code uses
the same argument.  If you suggest explicitly changing calls to
read_lock to call __acquire__ with the appropriate context, it might
prove equally easy to make the argument of read_lock that context.

> If you have suggestions on how to solve this I'm all ears, so far I
> decided it wasn't worth it and opted for explicitly naming all the
> contexts.
> 
> (with my patch the above would just create a context called "x")

That doesn't make sense to me; after preprocessing, x no longer
exists, so I don't see how you could pick up the identifier "x".  I
can understand that you might pick up two different expressions in
place of x which you can't easily compare, though.

And as for how to solve it: I think alias analysis might work.

- Josh Triplett


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]