dcoughlin added inline comments.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:1046
if (V2_untested.isUnknownOrUndef()) {
Bldr.generateNode(U, *I, state->BindExpr(U, LCtx, V2_untested));
+
----------------
lebedev.ri wrote:
> dcoughlin wrote:
> > 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.
> >
> > 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().
> Hm, so the only change needed here is
> ```diff
> - Bldr.generateNode(U, *I, state->BindExpr(U, LCtx, V2_untested));
> + state = state->BindExpr(U, LCtx, V2_untested);
> ```
> ?
Yes, that's right. What you have here looks good to me.
================
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
----------------
lebedev.ri wrote:
> dcoughlin wrote:
> > 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.
> Hmm, *this* test did not break after i changed
> `ExprEngine::VisitIncrementDecrementOperator()`.
> Did i change it wrong?
Aah, my mistake. I though this test was passing -verify (which verifies
expected-warning{{}} contents). Instead, it is only checking the plists, so the
missing expected leak would only be caught by the plist change.
I suggest changing the test to:
```
...
if (in > 5) {
int *p = malloc(12);
*p = 0;
(*p)++;
}
...
```
So that the test doesn't generate a fatal error for the access to uninitialized
memory and instead continues to check for the path of the leak.
================
Comment at: test/Analysis/malloc-plist.c:112
+ (*p)++; // expected-warning {{The expression is an uninitialized value.
The computed value will also be garbage}}
+ *p = 0; // no-warning
+ (*p)++; // no-warning
----------------
Here you should stick the `*p = 0;` before the post increment so the rest of
the code is exercised.
Repository:
rC Clang
https://reviews.llvm.org/D40463
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
