On Thu, Mar 31, 2022 at 3:30 AM Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx> wrote: > > On 03/30, David Gow wrote: > > On Wed, Mar 30, 2022 at 7:23 AM Marcelo Schmitt > > <marcelo.schmitt1@xxxxxxxxx> wrote: > > > > > > Enhance the static analysis tools section with a discussion on when to > > > use each of them. > > > > > > This was mainly taken from Dan Carpenter and Julia Lawall's comments on > > > the previous documentation patch for static analysis tools. > > > > > > Lore: https://lore.kernel.org/linux-doc/20220329090911.GX3293@kadam/T/#mb97770c8e938095aadc3ee08f4ac7fe32ae386e6 > > > > > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx> > > > Cc: Dan Carpenter <dan.carpenter@xxxxxxxxxx> > > > Cc: Julia Lawall <julia.lawall@xxxxxxxx> > > > --- > > > > Thanks: this sort of "when to use which tool" information is really > > what the testing guide page needs. > > > > I'm not familiar enough with these tools that I can really review the > > details properly, but nothing stands out as obviously wrong to me. > > I've made a few comments below regardless, but feel free to ignore > > them if they're not quite right. > > > > Acked-by: David Gow <davidgow@xxxxxxxxxx> > > > > Cheers, > > -- David > > > > > Documentation/dev-tools/testing-overview.rst | 33 ++++++++++++++++++++ > > > 1 file changed, 33 insertions(+) > > > > > > diff --git a/Documentation/dev-tools/testing-overview.rst b/Documentation/dev-tools/testing-overview.rst > > > index b5e02dd3fd94..91e479045d3a 100644 > > > --- a/Documentation/dev-tools/testing-overview.rst > > > +++ b/Documentation/dev-tools/testing-overview.rst > > > @@ -146,3 +146,36 @@ Documentation/dev-tools/coccinelle.rst documentation page for details. > > > > > > Beware, though, that static analysis tools suffer from **false positives**. > > > Errors and warns need to be evaluated carefully before attempting to fix them. > > > + > > > +When to use Sparse and Smatch > > > +----------------------------- > > > + > > > +Sparse is useful for type checking, detecting places that use ``__user`` > > > +pointers improperly, or finding endianness bugs. Sparse runs much faster than > > > +Smatch. > > > > Given that the __user pointer and endianness stuff is found as a > > result of Sparse's type checking support, would rewording this as > > "Sparse does type checking, such as [detecting places...]" or similar > > be more clear? > > Myabe. I tried changing it a little while adding a bit of information from > https://lwn.net/Articles/689907/ > > "Sparse does type checking, such as verifying that annotated variables do not > cause endianness bugs, detecting places that use ``__user`` pointers improperly, > and analyzing the compatibility of symbol initializers." > > Does it sound better? > Yeah: that sounds much better to me. Thanks! > > > > > + > > > +Smatch does flow analysis and, if allowed to build the function database, it > > > +also does cross function analysis. Smatch tries to answer questions like where > > > +is this buffer allocated? How big is it? Can this index be controlled by the > > > +user? Is this variable larger than that variable? > > > + > > > +It's generally easier to write checks in Smatch than it is to write checks in > > > +Sparse. Nevertheless, there are some overlaps between Sparse and Smatch checks > > > +because there is no reason for re-implementing Sparse's check in Smatch. > > > > This last sentence isn't totally clear to me. Should this "because" be "so"? > > Smatch uses (is shipped with) a modified Sparse implementation which it uses as > a C parser. Apparently, Sparse does some checkings while parsing the code for > Smatch so that's why we have some overlapping between the checks made when we > run Smatch and the ones made when we run Sparse alone. > > I didn't dig into the code, but I guess further modifying Sparse to prevent it > from doing some types of cheks wouldn't add much to Smatch. That last saying > should've reflected this fact, but it seems to cause confusion without a proper > context. Reading the sentence back again, I think we could just drop the last > part: > > "Nevertheless, there are some overlaps between Sparse and Smatch checks." > Yeah, I do think that makes more sense. I don't think the fact that some of the checks overlap causes any problems at all, to be honest, so you _could_ get rid of the whole sentence without losing too much, but I'm also happy with it as it is in v3. > > > > > + > > > +Strong points of Smatch and Coccinelle > > > +-------------------------------------- > > > + > > > +Coccinelle is probably the easiest for writing checks. It works before the > > > +pre-compiler so it's easier to check for bugs in macros using Coccinelle. > > > +Coccinelle also writes patches fixes for you which no other tool does. > > > + > > > +With Coccinelle you can do a mass conversion from > > > > (Maybe start this with "For example," just to make it clear that this > > paragraph is mostly following on from how useful it is that Coccinelle > > produces fixes, not just warnings.) > > Will do > > > > > > +``kmalloc(x * size, GFP_KERNEL)`` to ``kmalloc_array(x, size, GFP_KERNEL)``, and > > > +that's really useful. If you just created a Smatch warning and try to push the > > > +work of converting on to the maintainers they would be annoyed. You'd have to > > > +argue about each warning if can really overflow or not. > > > + > > > +Coccinelle does no analysis of variable values, which is the strong point of > > > +Smatch. On the other hand, Coccinelle allows you to do simple things in a simple > > > +way. > > > -- > > > 2.35.1 > > >
