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