Currently clang static analyzer does warn on:
int x; x+=1; x-=1; x=x+1; x=x-1;
But does warn on:
int x; x++; x--; --x; ++x;
This differential should fix that.
Fixes https://bugs.llvm.org/show_bug.cgi?id=35419
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
| docs/ReleaseNotes.rst | ||
|---|---|---|
| 261 ↗ | (On Diff #124280) | The end of the sentence should be either 'on uninitialized values' or 'on an uninitialized values'. |
| docs/ReleaseNotes.rst | ||
|---|---|---|
| 261 ↗ | (On Diff #124280) |
But it is already on an uninitialized values. ? |
| docs/ReleaseNotes.rst | ||
|---|---|---|
| 261 ↗ | (On Diff #124280) | Sorry, my tablet autocompleted :( With 'an' values needs to be singular 'an' -> 'value'. If you want 'values' drop the 'an'. |
| docs/ReleaseNotes.rst | ||
|---|---|---|
| 261 ↗ | (On Diff #124280) | Maybe Jonas was thinking if removing the plural form if you having an or of you want to keep it plural remove the an. |
| lib/StaticAnalyzer/Core/ExprEngineC.cpp | ||
| 1048 ↗ | (On Diff #124280) | Comments should be whole sentences starting with capital letters. |
| 1051 ↗ | (On Diff #124280) | 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? |
| test/Analysis/malloc-plist.c | ||
| 2 ↗ | (On Diff #124280) | 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. |
Comment Actions
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.
| lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp | ||
|---|---|---|
| 64 ↗ | (On Diff #124288) | "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." |
| lib/StaticAnalyzer/Core/ExprEngineC.cpp | ||
| 1046 ↗ | (On Diff #124288) | 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-warningThe 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. |
| test/Analysis/malloc-plist.c | ||
| 13 ↗ | (On Diff #124288) | 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. |
| 111 ↗ | (On Diff #124288) | Same point applies here. |
| 2 ↗ | (On Diff #124280) | Let's hold off on replacing the plist test with text. We'll need to make sure we're still testing plist emission separately. |
| test/Analysis/objc-for.m | ||
| 131 ↗ | (On Diff #124288) | You should just initialize j in order to preserve the test's intent. (And similarly for the other new warnings in this file. |
| test/Analysis/uninit-const.c | ||
| 127 ↗ | (On Diff #124288) | You'll need to split this test into multiple functions once you stop generating the extra node. |
| test/Analysis/uninit-const.cpp | ||
| 11 ↗ | (On Diff #124288) | 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. |
Comment Actions
@dcoughlin thank you for the review! A few notes of my own below.
| lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp | ||
|---|---|---|
| 64 ↗ | (On Diff #124288) | Yep, i did not like my wording either :) |
| lib/StaticAnalyzer/Core/ExprEngineC.cpp | ||
| 1046 ↗ | (On Diff #124288) |
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); ? |
| 1051 ↗ | (On Diff #124280) |
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. |
| test/Analysis/malloc-plist.c | ||
| 13 ↗ | (On Diff #124288) | Hmm, *this* test did not break after i changed ExprEngine::VisitIncrementDecrementOperator(). |
| 111 ↗ | (On Diff #124288) | Same here. |
| lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp | ||
|---|---|---|
| 64 ↗ | (On Diff #124288) | A value being undefined does nt mean uninitialized. E.g.: the result of a bad shift operation might be UndefVal as well. |
| lib/StaticAnalyzer/Core/ExprEngineC.cpp | ||
|---|---|---|
| 1046 ↗ | (On Diff #124288) | Yes, that's right. What you have here looks good to me. |
| test/Analysis/malloc-plist.c | ||
| 112 ↗ | (On Diff #124724) | Here you should stick the *p = 0; before the post increment so the rest of the code is exercised. |
| 13 ↗ | (On Diff #124288) | 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 Actions
Address @dcoughlin's review notes.
| lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp | ||
|---|---|---|
| 64 ↗ | (On Diff #124288) | I think this is for @dcoughlin to answer. |
| lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp | ||
|---|---|---|
| 64 ↗ | (On Diff #124288) | I was wondering about examples like: int x = 1 << -1; ++x; In this particular case, it will not issue the misleading error message. The shift that results in an undefined value will generate an error node, so we will not warn for the pre increment. But in general I am a bit uncomfortable about the assumption of this check: if a value is undefined the reason is that it is being uninitialized. Note that, of course this problem is not specific to this patch but a general question about the checker. |
Comment Actions
Thanks, this looks good to me!
Do you have commit access or do you need someone to commit it for you?
| lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp | ||
|---|---|---|
| 64 ↗ | (On Diff #124288) | It is a good point, but I think fixing it should wait until a later patch. |
Comment Actions
Awesome, thank you!
Do you have commit access or do you need someone to commit it for you?
I do have commit access.