Duplicate and confusing warnings from -Wuninitialized

[chandlerc]

Bugzilla Link	9626
Resolution	FIXED
Resolved on	Apr 04, 2011 20:08
Version	unspecified
OS	All
CC	@tkremenek

Extended Description

This is partially my fault. =[ But I'm not sure what exactly to do here. I added a special check for -Wuninitialized with self-initialization cases. The recent changes to the CFG-based uninitialized checker now warn for the same construct, resulting in this:

% cat x.cc
void test() {
int x = x + 42;
(void)x;
}

% ./bin/clang -fsyntax-only -Wuninitialized x.cc
x.cc:2:11: warning: variable 'x' is uninitialized when used within its own initialization [-Wuninitialized]
int x = x + 42;
~ ^
x.cc:2:11: warning: variable 'x' is possibly uninitialized when used here [-Wuninitialized]
int x = x + 42;
^
x.cc:2:3: note: variable 'x' is declared here
int x = x + 42;
^
x.cc:2:17: note: add initialization to silence this warning
int x = x + 42;
^
= 0
2 warnings generated.

At the very least we shouldn't generate two warnings here, but more-over:

• The first is much more concise
• The declaration note is general unhelpful when warning on the initializer
• The fixit hint is just nuts here. =/

I wonder if we should stick to the custom logic for the initializer case for now? Or is there an easy way to fix these in the CFG?

[chandlerc]

assigned to @tkremenek

[tkremenek]

This is partially my fault. =[ But I'm not sure what exactly to do here. I
added a special check for -Wuninitialized with self-initialization cases. The
recent changes to the CFG-based uninitialized checker now warn for the same
construct, resulting in this:

There haven't been any recent changes to the CFG-based initializer checker. These were likely already caught when the self-initialization check went in.

% cat x.cc
void test() {
int x = x + 42;
(void)x;
}

% ./bin/clang -fsyntax-only -Wuninitialized x.cc
x.cc:2:11: warning: variable 'x' is uninitialized when used within its own
initialization [-Wuninitialized]
int x = x + 42;
~ ^
x.cc:2:11: warning: variable 'x' is possibly uninitialized when used here
[-Wuninitialized]
int x = x + 42;
^
x.cc:2:3: note: variable 'x' is declared here
int x = x + 42;
^
x.cc:2:17: note: add initialization to silence this warning
int x = x + 42;
^
= 0
2 warnings generated.

At the very least we shouldn't generate two warnings here, but more-over:

• The first is much more concise
• The declaration note is general unhelpful when warning on the initializer
• The fixit hint is just nuts here. =/

I wonder if we should stick to the custom logic for the initializer case for
now? Or is there an easy way to fix these in the CFG?

I think this can easily be fixed within the CFG-based dataflow analysis. We can use the diagnostic from the self-init check, and remove that specialized logic. The FIXIT is just wrong since there is already an initializer.

[tkremenek]

I removed the fixit for variables that already have an initializer:

r128838

[tkremenek]

The dataflow-analysis based -Wuninitialized now uses the "self-init" warning when the uninitalized use occurs within an initializer:

r128843

We now have actual duplicated diagnostics:

/tmp/t.cc:2:11: warning: variable 'x' is uninitialized when used within its own initialization [-Wuninitialized]
int x = x + 42;
~ ^
/tmp/t.cc:2:11: warning: variable 'x' is uninitialized when used within its own initialization [-Wuninitialized]
int x = x + 42;
^
/tmp/t.cc:2:3: note: variable 'x' is declared here
int x = x + 42;
^
2 warnings generated.

Note that the test case I had to update in r128843 shows that the dataflow-based analysis has warned about self-initialization problems for a long time.

[tkremenek]

When I comment out the custom self-initialization check, these are the cases the dataflow-based analysis does not handle:

• initializers in global variables

This causes the following test to fail:

******************** TEST 'Clang :: SemaCXX/uninitialized.cpp' FAILED ********************
Script:

/Users/kremenek/llvm-xcode/bin/Debug/clang -cc1 -fsyntax-only -Wall -Wuninitialized -verify /Volumes/Data/src/llvm/tools/clang/test/SemaCXX/uninitialized.cpp

Exit Code: 1
Command Output (stderr):

error: 'warning' diagnostics expected but not seen:
Line 11: variable 'b' is uninitialized when used within its own initialization
Line 12: variable 'c' is uninitialized when used within its own initialization
Line 12: variable 'c' is uninitialized when used within its own initialization
Line 14: variable 'd' is uninitialized when used within its own initialization
Line 14: variable 'd' is uninitialized when used within its own initialization
Line 16: variable 'e' is uninitialized when used within its own initialization
Line 17: variable 'f' is uninitialized when used within its own initialization
7 errors generated.

[tkremenek]

It's also conspicuous that removing SelfReferenceChecker doesn't cause any -Wuninitialized warnings to go away for C++ initializers. Is it suppose to handle this case? If so, are there no test cases?

[tkremenek]

This is partially my fault. =[ But I'm not sure what exactly to do here. I
added a special check for -Wuninitialized with self-initialization cases. The
recent changes to the CFG-based uninitialized checker now warn for the same
construct, resulting in this:

There haven't been any recent changes to the CFG-based initializer checker.
These were likely already caught when the self-initialization check went in.

Mea culpa. After looking at UninitializedValues.cpp more, I can see where the dataflow analysis would not catch some cases of self-initialization. This is a bug, however, not a fundamental design flaw.

[tkremenek]

This is partially my fault. =[ But I'm not sure what exactly to do here. I
added a special check for -Wuninitialized with self-initialization cases. The
recent changes to the CFG-based uninitialized checker now warn for the same
construct, resulting in this:

There haven't been any recent changes to the CFG-based initializer checker.
These were likely already caught when the self-initialization check went in.

Mea culpa. After looking at UninitializedValues.cpp more, I can see where the dataflow analysis would not catch some cases of self-initialization. This is a bug, however, not a fundamental design flaw.

[tkremenek]

This is partially my fault. =[ But I'm not sure what exactly to do here. I
added a special check for -Wuninitialized with self-initialization cases. The
recent changes to the CFG-based uninitialized checker now warn for the same
construct, resulting in this:

There haven't been any recent changes to the CFG-based initializer checker.
These were likely already caught when the self-initialization check went in.

Mea culpa. After looking at UninitializedValues.cpp more, I can see where the
dataflow analysis would not catch some cases of self-initialization. This is a
bug, however, not a fundamental design flaw.

It looks like self-init warnings would get flagged by the dataflow analysis when the initializer was trivial and contained no control-flow. When the initializer contained control-flow, e.g. a statement expression, no warning was issued because the analysis did not register that a variable was uninitialized before seeing the DeclStmt in the CFG. The initializer, however, appears before the DeclStmt in the CFG if it has control-flow.

[tkremenek]

Fixed: r128858

Now the SelfReferenceChecker only handles global variables; the dataflow analysis handles other cases. We can continue to further evolve this design.

[chandlerc]

The dataflow-analysis based -Wuninitialized now uses the "self-init" warning
when the uninitalized use occurs within an initializer:

Awesome!

We now have actual duplicated diagnostics:

Yea, we should get rid of the hacky self-init ones, especially as there are only a few bugs left...

/tmp/t.cc:2:11: warning: variable 'x' is uninitialized when used within its own
initialization [-Wuninitialized]
int x = x + 42;
~ ^
/tmp/t.cc:2:11: warning: variable 'x' is uninitialized when used within its own
initialization [-Wuninitialized]
int x = x + 42;
^
/tmp/t.cc:2:3: note: variable 'x' is declared here
int x = x + 42;
^
2 warnings generated.

For reference, I think we should get rid of the note as well, and just underline the declaration in the warning snippet. I'll happily look at that part of the QoI here, and I can rip out some of the hacky old self-init while I'm in there... (and if you don't beat me to it...)

It's also conspicuous that removing SelfReferenceChecker doesn't cause any
-Wuninitialized warnings to go away for C++ initializers. Is it suppose to
handle this case? If so, are there no test cases?

I just don't think we had finished fleshing out that, and I had held back looking for some time to hack on the CFG-based solution. I'm thrilled you've had time to look at it.

[chandlerc]

Fixed: r128858

Now the SelfReferenceChecker only handles global variables; the dataflow
analysis handles other cases. We can continue to further evolve this design.

Awesome. I'm still catching up (in meetings all day) but I'll read through the patches. Thanks for all the fixes, and I'll file any more test cases we hit.

[chandlerc]

For reference, I think we should get rid of the note as well, and just
underline the declaration in the warning snippet. I'll happily look at that
part of the QoI here, and I can rip out some of the hacky old self-init while
I'm in there... (and if you don't beat me to it...)

Heh, looks like in the version of the code committed, it does suppress the added note. Cool.