[cfe-commits] [PATCH] Checking for sometimes-uninitialized variables

Ted Kremenek kremenek at apple.com
Thu May 24 21:33:01 PDT 2012


Looks great.  Thanks so much for doing this!

On May 24, 2012, at 7:19 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> Hi Ted,
> 
> Thanks for the comments!
> 
> On Thu, May 24, 2012 at 2:33 PM, Ted Kremenek <kremenek at apple.com> wrote:
> Hi Richard,
> 
> I very much like this work.  I have some nit picky comments, but I'd like to first focus on the high-level ones.
> 
> First, I'm wondering if this should just be rolled into -Wuninitialized.  If the false positive rate is low (or none), that would seem to be the right thing to do.  Otherwise, most users aren't going to benefit from this warning unless they know about it.  It seems like the whole point of this patch is to find cases where something is not unconditionally uninitialized but we have much higher confidence in reporting an issue.  This patch doesn't try to resolve "maybe uninitialized" issues that could be resolved if we had better path-sensitivity.  Thoughts?
> 
> I think rolling this into -Wuninitialized is best; I've updated the patch to work this way. The old -Wuninitialized is still available via -Wuninitialized -Wno-sometimes-uninitialized, which seems reasonable to me.
>  
> Second, I honestly found the description of the algorithm in the comments of getUninitUse() to be (at first) fairly unintuitive.  Since the algorithm isn't so obvious, actually having an example in the comments would be great.  After I read the code several times, I figured it out, but it also required me working through an example on paper.  After that, the comments made sense to me.
> 
> I've added an example and reworded the comment.
>  
> > int x;
> > for (int n = 0; n < size; ++n) { // Redundant "n < size" test.
> >  // This is somehow guaranteed to happen for some n.
> >  if (f(n) == k) {
> >    x = n;
> >    break;
> >  }
> > }
> > return x;
> 
> This is a common source of false positives with the static analyzer, but I'd argue that the code should just initialize 'x' to be safe.  I can't look at this in isolation and immediately tell if this is safe, because it requires me making assumptions about size, n, which aren't documented in the code.
> 
> Some of the code I found with this pattern had genuine uninitialized-use bugs in it, and the rest of the code seemed clearer when rearranged to avoid the dead condition.
>  
> More comments in the patch itself:
> 
> All done.
>  
> Many of the other changes in the patch seem like just nice cleanups, and not core to this work.  I'd prefer they went in separately so that the key algorithmic change is easy to spot in a single (small) patch, but I don't want to waste your time with having to break it down if it is too complicated.
> 
> Easily separable cleanups are in r157440, revised patch in r157458.
> 
> Thanks!

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120524/8eae5347/attachment.html>


More information about the cfe-commits mailing list