Re: Potential static analysis ideas

From: "NeilBrown" <neilb@suse.de>
To: "Laurent Pinchart" <laurent.pinchart@ideasonboard.com>
Cc: "Geert Uytterhoeven" <geert@linux-m68k.org>,
	"Dan Carpenter" <dan.carpenter@oracle.com>,
	ksummit@lists.linux.dev, "Julia Lawall" <julia.lawall@inria.fr>
Subject: Re: Potential static analysis ideas
Date: Sun, 25 Jul 2021 09:45:37 +1000	[thread overview]
Message-ID: <162717033769.3676.6942299974175827854@noble.neil.brown.name> (raw)
In-Reply-To: <YPyf4NkpONFhrbuw@pendragon.ideasonboard.com>

On Sun, 25 Jul 2021, Laurent Pinchart wrote:
> On Sat, Jul 24, 2021 at 03:33:48PM +0200, Geert Uytterhoeven wrote:
> > On Fri, Jul 23, 2021 at 9:11 PM Dan Carpenter wrote:
> > > These days we have disabled GCC's uninitialized warnings so it falls
> > > to static analysis devs to review all the new warnings.  I sometimes
> > > ignore warnings if they look like probably they aren't a bug.  Does this
> > > function allow zero size writes?  Does this switch statement really need
> > > a default case?  Maybe sometimes I miss bugs.
> > 
> > Yeah, I tended to find a few real bugs every release using the good old
> > gcc 4.2, before support for it was dropped.
> > I hope someone still runs modern compilers with GCC's uninitialized
> > warnings enabled again?
> > 
> > > Here is another example of something which I have a check for but I
> > > haven't pushed.
> > >
> > >         ret = frob();
> > >         if (!ret)
> > >                 return ret;
> > >
> > > This code is normally correct but sometimes it means the if statement is
> > > reversed and it should be "if (ret) return ret;".  To me returning a
> > > literal is always more clear but but clearly the original author felt
> > > "ret" was more clear...  It's only a few bugs per year so it's not a big
> > > deal either way.
> > 
> > To make it work well, you need to know if frob() and/or the current
> > function return an error code or not.  While you can use some heuristics
> > (e.g. is there any return -Exxx), perhaps we can add an annotation to
> > indicate if a function returns an error code, or an error pointer?
> 
> https://lore.kernel.org/linux-media/YNMvarFl%2FKU1pGCG@pendragon.ideasonboard.com/
> 
> I think it would be useful, if not for the tools, at least for
> developers.

Agreed.  I added some code to smatch so that I could annotate pointers to
say if they are allowed to be NULL.  The implementation isn't perfect,
but I love having that extra documentation about when I do or don't have
to check for NULL.

NeilBrown