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

[Johannes Berg <johannes@sipsolutions.net>]

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

Hi,

Managed to take a closer look now.

> > What's this "rdwr" etc. for? And "call"? Also, how are you planning to
> > handle nested contexts, where 1,1 doesn't cut it any >0,>0 is really
> > more like what we need?
> >
> 
> I also use it to check a recusive lock mechanism. 
> 
> The sematics of what I implemented is:
> You simply use the __attribute__((context(ctx,in,out))) to
> annotate the context changes of functions.
> 
> to annotate a variable, struct/union member or a function that
> a certain locking primitive is required for accessing it, you
> do it by 
> 
> __attribute__((require_context(ctx,min,max,"type")))
> 
> ctx ... the expression that describes the locking primitive
> min ... the minimum of locks required of that locking primitive
> max ... the maximum allowed of locks of that locking primitive.
> type .. read, write, rdwr or call for the access type.
> 
> So you can express you need to hold this and that locking 
> primitive to write to something. But an other locking
> primitive might be sufficient for reading that something.
> 
> The annotation for a variable foo protected by a recursive lock
> bar would be:
> 
> int foo __attribute__((require_context(bar,1,99999,"rdwr")))

To be honest, that confuses me. I haven't been able to come up with a
way to make this test case I have result in a warning:

static void bad_macro1(void)
{
    m();
    a();
    r();
}

where a() is acquire(), r() is release() and m() should require the
lock, and all should be implemented as macros. I even tried playing with
dummy inline functions that are called from the macros.

As for read/write, what's wrong with encoding that in the context?

Something like
void read_lock(something) __attribute__((context(read_something,0,1)));
void write_lock(something) __attribute__((context(write_something,0,1)));

or similar. And if it has to be a parameter, why a string?

Another thing I actually like a lot about the fact that my patch changes
the warnings is that it tells you
 - which function caused the error
 - where that function is

I recently had the pleasure of digging through a huge switch statement
that looked like this:

function {

[lots of code]

lock();

[lots more code]

switch(something) {
case A:
	[lots of code]
	unlock();
	[lots of code]
	break;
[repeat a dozen times]
case M:
	[lots of code]
	unlock();
	[lots of code]
	// *** NOTE MISSING break;
case N:
	[lots of code]
	unlock();		// <----- this is where my code tells you the error is
	[lots of code]
	break;
[repeat another dozen times]
}

[lots more code in the function]

}		// <--- this is where current sparse tells you the error is


Also, my code tells you the function name, and not just "oh, some lock
context was wrong" :) I have just created another patch on top of mine
that will make the error look like this:

context.c:360:10: warning: context problem in 'warn_huge_switch': 'r' expected different context
context.c:360:10:    default context: wanted >= 1, got 0



I like the cond_lock() trick, but I was actually planning on expanding
the checking to switch() statements as well and I don't think we can
make it work there with such a trick.

Maybe something like
__attribute__((conditional_context(ctx,req,default,val=ctxchange,...)))

where you could have e.g.

__attribute__((conditional_context(L,1,0,1=1,-1=-1)))

Not sure yet though whether that is useful or not.


Finally, I think you have a bit of an inconsistency: You introduced the
require_context attribute, yet your context() macro still partially
fulfils that role, i.e. if you say that a function decreases a context
then you require that it gets one when called. Hence, extending that to
arbitrary context changes defined via the context attribute seems more
natural to me than to require using the require_context attribute for
positive assertions and the context attribute for negative assertions.

> PS: I just realized that there my e-mail client introduced linebreakes
>     in the first patch I posted. I will repost on request of course.

No worries, was easy enough to fix up.

johannes


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]