lebedev.ri added inline comments.
================
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";
----------------
dcoughlin wrote:
> "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."
>
Yep, i did not like my wording either :)
================
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:1051
+ ExplodedNodeSet Dst3;
+ evalStore(Dst3, U, U, *I, state, loc, V2_untested);
+ Bldr.addNodes(Dst3);
----------------
xazax.hun wrote:
> This will trigger checkBind when unknown value is pre/post incremented. I
> wonder if this is the desired behavior. Couldn't you achieve the same on the
> checker side by having a checkPreStmt hook?
>
> @NoQ, @dcoughlin what do you think about triggering checkBind here? In fact,
> there is a bind during pre/post increment/decrement. But do we want these
> events to be visible for checkers with undefined values?
> This will trigger checkBind when unknown value is pre/post incremented. I
> wonder if this is the desired behavior. Couldn't you achieve the same on the
> checker side by having a checkPreStmt hook?
The current approach is what was suggested in the
https://bugs.llvm.org/show_bug.cgi?id=35419#c6, unless i have misread it of
course.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:1046
if (V2_untested.isUnknownOrUndef()) {
Bldr.generateNode(U, *I, state->BindExpr(U, LCtx, V2_untested));
+
----------------
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);
```
?
================
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
----------------
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?
================
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
----------------
dcoughlin wrote:
> Same point applies here.
Same here.
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
