[Cocci] Re: Question on Coccinelle

Julia Lawall julia at diku.dk
Thu Sep 2 17:47:44 CEST 2010


On Thu, 2 Sep 2010, Paul E. McKenney wrote:

> On Thu, Sep 02, 2010 at 07:25:02AM +0200, Julia Lawall wrote:
> > > > This time there is a call to foo after the call to bar and before the call 
> > > > to xxx, and this call to foo is that same as the one that was matched 
> > > > before the call to bar.
> > > 
> > > OK, because we would have to cycle around the loop to match, and the
> > > "when" clause would disqualify the only reasonable match.
> > 
> > Yes.
> > 
> > > Thank you again!
> > > 
> > > And yet another question...
> > > 
> > > spatch does not seem to like the following:
> > > 
> > > 	when != if (p) atomic_inc(&<+...p->f1...+>)
> > 
> > The branch of an if is a statement.  A function call that is a statement 
> > has a ; after it.  I see that the parse error is quite unhelpful, due to 
> > the way the yacc-like parsing works.
> 
> Ah, got it!
> 
> > But here you assume that the if (p) will only have the atomic_inc.  
> > Perhaps the following would be better:
> > 
> > when != if (p) { ... atomic_inc(&<+...p->f1...+>); ... }
> > 
> > Note that all of the when code has to stay on the same line.  An 
> > isomorphism will allow this to match a case where there are no {}.
> 
> This does work nicely, thank you!  The isomorphism works in both
> directions?

No.  You can find the definition in standard.iso.  But in general, 
isomorphisms only remove things.  Adding things would probably result in 
too many permutations.

> > > The problem I am trying to solve is that it is OK to leak a pointer
> > > out of an RCU read-side critical section if you have done something
> > > to keep the underlying data structure from going away.  One way of
> > > doing this is to increment a reference count in the pointed-to structure,
> > > for example:
> > > 
> > > 	rcu_read_lock();
> > > 	p = rcu_dereference(gp);
> > > 	if (p && something_bad_happened(p))
> > > 		p = NULL
> > > 	if (p)
> > > 		atomic_inc(&p->refcount);
> > > 	rcu_read_unlock();
> > > 
> > > 	if (p)
> > > 		do_something_with(p->a);
> > > 
> > > My current set of rules will flag the last line as a pointer leak,
> > > presumably because there is a code path that skips the atomic_inc().
> > > My first reaction was to just put the "if" statement in a when clause
> > > as above, but that gives a parse error.
> > > 
> > > I tried a number of variations, including a separate rule matching the
> > > "if" (which worked) and recording the "if"s position (which did not).
> > > For your amusement, one attempt follows.
> > 
> > I don't think when != @guardedref.q works at all, or if it does, then it 
> > is being interpreted as something other than what you intended.  When you 
> > inherit a metavariable, the name of the rule from which it is inherited 
> > only appears in the metavariable declaration.  So guardedref.q is a 
> > structure field reference.  And a position variable has to be attached to 
> > something.  In this case, it should be an identifier, because that is what 
> > the position was attached to in guardedref.  It can be any identifier at 
> > all, not necessarily an inherited one, because the position is enough to 
> > make it unique.
> 
> OK, so the rule inheriting the metavariable must not have another
> metavariable with that same name.  Fair enough!

Yes, this is the case.

julia


More information about the Cocci mailing list