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

[Richard Smith]

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/564cdeca/attachment.html>