PR17558 - Question about Uninitialized Variables

Hey all,

I'm looking at this bug report here:
http://llvm.org/bugs/show_bug.cgi?id=17558

And I wanted to get some feedback/ask some questions before putting
the patch onto cfe-commits.

I've narrowed down the issue ClassifyRefs::VisitCastExpr. Once it sees
an expression like (void)&x, it'll proceed to classify the
UnaryOperator which is the immediate subexpression of the Cast
expression rather than the DeclRefExpr whose classification will be
queried later when checking for uninitialized variables.

So my idea was to pretty much make sure that when ClassifyRefs
encounters a Cast expression, it will classify the nested DeclRefExpr
instead. This way Clang will not think that (void)&x is initializing
x.

However, I think that a previous commit's test case (for PR 16054
-http://llvm.org/bugs/show_bug.cgi?id=16054) seems to be relying on
this behavior. In test/Analysis/uninit-sometimes.cpp,we have

void PR16054() {
  int x; // expected-note {{variable}} expected-warning {{used uninitialized whenever function 'PR16054}}
  while (x != 0) { // expected-note {{use}}
    (void)&x;
  }
}

Changing (void)&x to (void)x results in the "sometimes-uninitialized"
warning becoming a regular "uninitialized" warning. I believe in this
case, this warning should be the regular uninitialized warning, am I
correct in assuming this?

Finally, I have this following piece of code which is of questionable
quality. The purpose of this hack is to make the code function the
same as before when it encounters one of these situations. More or
less, I don't want to dig into function calls (including typeid,
sizeof and uuid) and mark them as ignore because that would override
the mark on them as being used. I'm sure there's probably a better way
to do this though, thoughts?

  if (isa<CXXTypeidExpr>(S) || isa<CallExpr>(S) || isa<CXXUuidofExpr>(S)
      >> isa<SizeOfPackExpr>(S)) {
    classify(dyn_cast<Expr>(S), Ignore);
    return;
  }

Thanks!

uninitialized-warning.patch (3.61 KB)

The -Wuninitialized warning isn’t ever going to have zero false negatives. Why is it important to support the non-idiomatic case of

(void)&x;

rather than suggesting people use the normal idiom of

(void)x;

? I’m not fundamentally opposed to this, but it seems rather ad-hoc, and I’d like for us to have a good justification for this complexity.

That said, the patch is definitely going in the right direction, if we do want to handle this. I’ve not looked through VisitCastExprChildren, but I’m surprised by the complexity of it. How did you determine the set of things to check for there?

The -Wuninitialized warning isn't ever going to have zero false negatives.
Why is it important to support the non-idiomatic case of

  (void)&x;

rather than suggesting people use the normal idiom of

  (void)x;

? I'm not fundamentally opposed to this, but it seems rather ad-hoc, and I'd
like for us to have a good justification for this complexity.

I don't necessarily disagree with you here -- especially since it does
seem rather complicated for such a non-common way of doing things.

That said, the patch is definitely going in the right direction, if we do
want to handle this. I've not looked through VisitCastExprChildren, but I'm
surprised by the complexity of it. How did you determine the set of things
to check for there?

In VisitCastExprChildren, the main thing I'm doing is visiting all the
children of the cast expression to find the DeclRefExpr (so I can
handle the more general case of doing a cast and not have Clang mark
the variable as being initialized rather than ignored).

The CXXTypeId, CallExpr, CXXUuid, and SizeOf checks are there as they
are just the cases that I thought of that would potentially be using
the variable (and thus shouldn't be ignored). (I found these out
because some tests weren't passing). Actually on second thought, I
don't think sizeof should be in there.

Then finally the block expression, I had to look into its body rather
than its children (Also found this one out because I was no longer
passing a test).

After thinking about this code some more, I think there are two things
that can be done about this code:

1) If we really want to support this case, it might be best just to
have some code like:

if (isa<UnaryOperator>(CSE->getSubExpr())) {
  classify(CSE->getSubExpr()->getSubExpr(), Ignore);
  return;
}
classify(CSE->getSubExpr(), Ignore);

This way we avoid the bulk of the complexity while still supporting this case.

2) If the more general case is to be supported, I think I should change the

  if (isa<CXXTypeidExpr>(S) || isa<CallExpr>(S) || isa<CXXUuidofExpr>(S)
      >> isa<SizeOfPackExpr>(S)) {
    classify(dyn_cast<Expr>(S), Ignore);
    return;
  }

to something that checks to see whether or not the DeclRefExpr has
already been classified. If it has, then we should behave like the
original code. If it hasn't, then ignore it. That probably would be
less hacky than this current solution.

Thoughts?