tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: Using coccinelle for (quick?) syntax fixing



On Mon, 9 Aug 2010 13:27:38 +0300, Antti Kantee <pooka%cs.hut.fi@localhost> 
wrote:
> I really dislike untested wide-angle churn, especially if there is 0
> measurable gain.  Converting code to __arraycount is a prime example.
> The only benefit of __arraycount is avoiding typing and therefore typos.
> Neither of those apply when doing a churn.
> (there are subjective beauty values, but every C programmer knows
> the sizeof/sizeof idiom, which is more than what can be said about
> __arraycount)

arraycount is just one example; you could say the same about
roundup/howmany/min/max/... For arraycount, the use is limited; I agree.
However, I would prefer to have code using roundup/MIN/MAX rather than
rewriting them down. It tends to be harder to read, no matter the
developer's expertise can be.

> Examples of measurable benefit are good.  Encouraging churn is less
good,
> even if spatch-churn is a million times better than sed-churn.

Well, that's the eternal "if it ain't broke, don't fix it" debate. I don't
know about what you mean by "measurable benefit." Does code quality, DRY,
or KNF fit in? I hardly see how the thing can go wrong in this /very/
simple case though: dependency on cdefs, exact semantic match.

Anyway, that's not the purpose here.

>> I used these examples to get familiar with it; it starts getting useful
>> when you try to find out buggy code, like double free() in the same
>> function, mutex_exit() missing in a branch before returning, etc.
> 
> Static analysis is good.  However, it might take quite a bit of effort
> to get the rules general enough so that they trigger in more than one
> file and specific enough so that you don't get too many false positives.
> Just to give an example, the ffs allocator routines don't release the
> lock in an error branch.

That's not because there are specific cases where the tool has limited use
that it does not fit the rest :)

> I remember coccinelle had problems with cpp.  Any code which uses macros
> to "skip" C syntax will fail silently.  procfs comes to mind here. 
Also,
> I remember it using so much memory when given our kernel source that I
> could not finish a rototill and had to use it in combo with find/grep.

Indeed, using spatch -dir /usr/cvs/src is quite violent. And does not
scale very well when the cases are too general, and need heavy pattern
lookup.

> That said, if $someone can produce a set of rules which showably find
> bugs in NetBSD code and do not produce a lot of false positives, I'm
> very interested in seeing nightly runs.

Alright, let's get a bit more practical. Warning: patch may not apply
cleanly, and I am working with a "month-old" src:

- logical not with bitwise "&" (attached: notvsand.diff)

This one should be checked, I am not familiar with this code.


- more serious: NULL check then dereference (attached: nullderef.diff)

IMHO, the last ones would be easier to spot with "if (foo == NULL)..."
instead of having "if (!foo)..." Clarity does help (guess it did not for
the other half, anyway :/ )

> ... especially if there are no TAILQ false positives ;)

Only true negatives with that one :p

Purpose was to gather opinions (if it got wildly rejected, I would have
stop doing anything with it now). Note that I view coccinelle more like a
tool making rototilling easier than full blown static analyzer; there are
probably better ones out there (clang).

-- 
Jean-Yves Migeon
jeanyves.migeon%free.fr@localhost

Attachment: notvsand.diff
Description: Binary data

Attachment: nullderef.diff
Description: Binary data



Home | Main Index | Thread Index | Old Index