dcoughlin added a comment.
Thanks for tackling this! There is one major comment inline (you are generating
an extra node that you shouldn't, which is causing the analyzer to explore code
it shouldn't) and a suggestion for simpler diagnostic text.
================
Comment at: lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp:64
+ if (const UnaryOperator *U = dyn_cast<UnaryOperator>(StoreE)) {
+ str = "The expression of the unary operator is an uninitialized value. "
+ "The computed value will also be garbage";
----------------
"Unary operator" is probably not something we should expect users to know
about. How about just "The expression is an uninitialized value. The computed
value will also be garbage."
================
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:1046
if (V2_untested.isUnknownOrUndef()) {
Bldr.generateNode(U, *I, state->BindExpr(U, LCtx, V2_untested));
+
----------------
Instead of generating a node node here, you'll want to generate a new state:
```
state = state->BindExpr(U, LCtx, V2_untested);
```
and use that state in the call to evalStore().
Otherwise, you'll end up exploring from both the new generated node and from *I.
Here is a test that demonstrates why this is a problem. You should add it to
your tests.
```
void foo() {
int x;
x++; // expected-warning {{The expression of the unary operator is an
uninitialized value. The computed value will also be garbage}}
clang_analyzer_warnIfReached(); // no-warning
```
The undefined assignment checker generates what we call "fatal" errors. These
errors are so bad that it decides not to explore further on that path to report
additional errors. This means that the analyzer should treat any code that is
dominated by such an error as not reachable. You'll need to add the
`debug.ExprInspection` checker to your test RUN line and a prototype for
clang_analyzer_warnIfReached() for this test to to work.
================
Comment at: test/Analysis/malloc-plist.c:2
// RUN: rm -f %t
// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,unix.Malloc
-analyzer-output=plist -analyzer-config path-diagnostics-alternate=false -o %t
%s
// RUN: FileCheck -input-file %t %s
----------------
xazax.hun wrote:
> In case you willing to replace the plist based test with
> `-analyzer-output=text` based, it would be awesome, though not required for
> this patch to be accepted.
Let's hold off on replacing the plist test with text. We'll need to make sure
we're still testing plist emission separately.
================
Comment at: test/Analysis/malloc-plist.c:13
int *p = malloc(12);
- (*p)++;
+ (*p)++; // expected-warning {{The expression of the unary operator is
an uninitialized value. The computed value will also be garbage}}
+ *p = 0; // no-warning
----------------
Once you change the core modeling to not generate an extra node, you'll want to
initialize `*p` to `0` or something to preserve the intent of this test. For
this test it is important that the increment not stop further exploration of
the path.
================
Comment at: test/Analysis/malloc-plist.c:111
p = (int*)malloc(12);
- (*p)++;
+ (*p)++; // expected-warning {{The expression of the unary operator is an
uninitialized value. The computed value will also be garbage}}
+ *p = 0; // no-warning
----------------
Same point applies here.
================
Comment at: test/Analysis/objc-for.m:131
int i;
int j;
for (NSString *a in A) {
----------------
You should just initialize `j` in order to preserve the test's intent. (And
similarly for the other new warnings in this file.
================
Comment at: test/Analysis/uninit-const.c:127
+ int x; // expected-note 5 {{'x' declared without an initial value}}
+ x++; // expected-warning {{The expression of the unary operator is an
uninitialized value. The computed value will also be garbage}}
+ // expected-note@-1 {{The expression of the unary operator is an
uninitialized value. The computed value will also be garbage}}
----------------
You'll need to split this test into multiple functions once you stop generating
the extra node.
================
Comment at: test/Analysis/uninit-const.cpp:11
+// https://bugs.llvm.org/show_bug.cgi?id=35419
+void f11(void) {
----------------
I don't think this test is needed. The logic for C vs. C++ is shared for your
new functionality, so I think it is sufficient to just test the C case.
Repository:
rL LLVM
https://reviews.llvm.org/D40463
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
