[analyzer] Relax assertion in BugReporterVisitors.cpp isInitializationOfVar #125044
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nOfVar If we see a variable declaration (aka. DeclStmt), and the VarRegion it declared doesn't have Stack memspace, we assumed that it must be a local static variable. However, the declared variable may be an extern declaration of a global. In this patch, let's admit that local extern declarations are a thing. For the sake of completeness, I also added one more test for thread_locals - which are implicitly considered statics btw. (the isStaticLocal correctly also considers thread locals as local statics).
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Balazs Benics (steakhal) ChangesIf we see a variable declaration (aka. DeclStmt), and the VarRegion it declared doesn't have Stack memspace, we assumed that it must be a local static variable. In this patch, let's admit that local extern declarations are a thing. For the sake of completeness, I also added one more test for thread_locals - which are implicitly considered statics btw. (the Fixes #124975 Full diff: https://github.com/llvm/llvm-project/pull/125044.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index a9b4dbb39b5bd6..a6142063895dbd 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1198,7 +1198,10 @@ static bool isInitializationOfVar(const ExplodedNode *N, const VarRegion *VR) {
// If we ever directly evaluate global DeclStmts, this assertion will be
// invalid, but this still seems preferable to silently accepting an
// initialization that may be for a path-sensitive variable.
- assert(VR->getDecl()->isStaticLocal() && "non-static stackless VarRegion");
+ [[maybe_unused]] bool IsLocalStaticOrLocalExtern =
+ VR->getDecl()->isStaticLocal() || VR->getDecl()->isLocalExternDecl();
+ assert(IsLocalStaticOrLocalExtern &&
+ "Declared a variable on the stack without Stack memspace?");
return true;
}
diff --git a/clang/test/Analysis/null-deref-path-notes.cpp b/clang/test/Analysis/null-deref-path-notes.cpp
index c7b0619e297b3c..a37bbfe41a2c70 100644
--- a/clang/test/Analysis/null-deref-path-notes.cpp
+++ b/clang/test/Analysis/null-deref-path-notes.cpp
@@ -23,3 +23,38 @@ void c::f(B &g, int &i) {
f(h, b); // expected-note{{Calling 'c::f'}}
}
}
+
+namespace GH124975 {
+void no_crash_in_br_visitors(int *p) {
+ if (p) {}
+ // expected-note@-1 {{Assuming 'p' is null}}
+ // expected-note@-2 {{Taking false branch}}
+
+ extern bool ExternLocalCoin;
+ // expected-note@+2 {{Assuming 'ExternLocalCoin' is false}}
+ // expected-note@+1 {{Taking false branch}}
+ if (ExternLocalCoin)
+ return;
+
+ *p = 4;
+ // expected-warning@-1 {{Dereference of null pointer (loaded from variable 'p')}}
+ // expected-note@-2 {{Dereference of null pointer (loaded from variable 'p')}}
+}
+
+// Thread local variables are implicitly static, so let's test them too.
+void thread_local_alternative(int *p) {
+ if (p) {}
+ // expected-note@-1 {{Assuming 'p' is null}}
+ // expected-note@-2 {{Taking false branch}}
+
+ thread_local bool ThreadLocalCoin;
+ // expected-note@+2 {{'ThreadLocalCoin' is false}}
+ // expected-note@+1 {{Taking false branch}}
+ if (ThreadLocalCoin)
+ return;
+
+ *p = 4;
+ // expected-warning@-1 {{Dereference of null pointer (loaded from variable 'p')}}
+ // expected-note@-2 {{Dereference of null pointer (loaded from variable 'p')}}
+}
+} // namespace GH124975
|
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/50/builds/9625 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If we see a variable declaration (aka. DeclStmt), and the VarRegion it declared doesn't have Stack memspace, we assumed that it must be a local static variable.
However, the declared variable may be an extern declaration of a global.
In this patch, let's admit that local extern declarations are a thing.
For the sake of completeness, I also added one more test for thread_locals - which are implicitly considered statics btw. (the
isStaticLocal()correctly also considers thread locals as local statics).Fixes #124975