[clang] [alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. (PR #92639)

2024-05-24 Thread Ryosuke Niwa via cfe-commits 

https://github.com/rniwa closed https://github.com/llvm/llvm-project/pull/92639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. (PR #92639)

2024-05-23 Thread Ryosuke Niwa via cfe-commits 

https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/92639

>From 5ae3b193a6ec3617c99297da5b8d276b0e5bbc01 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa 
Date: Sat, 18 May 2024 02:17:30 -0700
Subject: [PATCH 1/2] [alpha.webkit.UncountedLocalVarsChecker] Detect
 assignments to uncounted local variable and parameters.

This PR updates alpha.webkit.UncountedLocalVarsChecker to emit warnings for 
assignments to uncounted
local variable and parameters instead of just the initialization during the 
declaration.
---
 .../WebKit/UncountedLocalVarsChecker.cpp  | 61 +++
 .../Checkers/WebKit/uncounted-local-vars.cpp  | 42 +
 2 files changed, 79 insertions(+), 24 deletions(-)

diff --git 
a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
index 0d9710a5e2d83..6c0d56303d5ad 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
@@ -135,7 +135,19 @@ class UncountedLocalVarsChecker
   bool shouldVisitImplicitCode() const { return false; }
 
   bool VisitVarDecl(VarDecl *V) {
-Checker->visitVarDecl(V);
+auto *Init = V->getInit();
+if (Init && V->isLocalVarDecl())
+  Checker->visitVarDecl(V, Init);
+return true;
+  }
+
+  bool VisitBinaryOperator(const BinaryOperator *BO) {
+if (BO->isAssignmentOp()) {
+  if (auto *VarRef = dyn_cast(BO->getLHS())) {
+if (auto *V = dyn_cast(VarRef->getDecl()))
+  Checker->visitVarDecl(V, BO->getRHS());
+  }
+}
 return true;
   }
 
@@ -174,7 +186,7 @@ class UncountedLocalVarsChecker
 visitor.TraverseDecl(const_cast(TUD));
   }
 
-  void visitVarDecl(const VarDecl *V) const {
+  void visitVarDecl(const VarDecl *V, const Expr *Value) const {
 if (shouldSkipVarDecl(V))
   return;
 
@@ -184,12 +196,8 @@ class UncountedLocalVarsChecker
 
 std::optional IsUncountedPtr = isUncountedPtr(ArgType);
 if (IsUncountedPtr && *IsUncountedPtr) {
-  const Expr *const InitExpr = V->getInit();
-  if (!InitExpr)
-return; // FIXME: later on we might warn on uninitialized vars too
-
   if (tryToFindPtrOrigin(
-  InitExpr, /*StopAtFirstRefCountedObj=*/false,
+  Value, /*StopAtFirstRefCountedObj=*/false,
   [&](const clang::Expr *InitArgOrigin, bool IsSafe) {
 if (!InitArgOrigin)
   return true;
@@ -232,34 +240,39 @@ class UncountedLocalVarsChecker
   }))
 return;
 
-  reportBug(V);
+  reportBug(V, Value);
 }
   }
 
   bool shouldSkipVarDecl(const VarDecl *V) const {
 assert(V);
-if (!V->isLocalVarDecl())
-  return true;
-
-if (BR->getSourceManager().isInSystemHeader(V->getLocation()))
-  return true;
-
-return false;
+return BR->getSourceManager().isInSystemHeader(V->getLocation());
   }
 
-  void reportBug(const VarDecl *V) const {
+  void reportBug(const VarDecl *V, const Expr *Value) const {
 assert(V);
 SmallString<100> Buf;
 llvm::raw_svector_ostream Os(Buf);
 
-Os << "Local variable ";
-printQuotedQualifiedName(Os, V);
-Os << " is uncounted and unsafe.";
-
-PathDiagnosticLocation BSLoc(V->getLocation(), BR->getSourceManager());
-auto Report = std::make_unique(Bug, Os.str(), BSLoc);
-Report->addRange(V->getSourceRange());
-BR->emitReport(std::move(Report));
+if (dyn_cast(V)) {
+  Os << "Assignment to an uncounted parameter ";
+  printQuotedQualifiedName(Os, V);
+  Os << " is unsafe.";
+
+  PathDiagnosticLocation BSLoc(Value->getExprLoc(), 
BR->getSourceManager());
+  auto Report = std::make_unique(Bug, Os.str(), BSLoc);
+  Report->addRange(Value->getSourceRange());
+  BR->emitReport(std::move(Report));
+} else {
+  Os << "Local variable ";
+  printQuotedQualifiedName(Os, V);
+  Os << " is uncounted and unsafe.";
+
+  PathDiagnosticLocation BSLoc(V->getLocation(), BR->getSourceManager());
+  auto Report = std::make_unique(Bug, Os.str(), BSLoc);
+  Report->addRange(V->getSourceRange());
+  BR->emitReport(std::move(Report));
+}
   }
 };
 } // namespace
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp 
b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
index 632a82eb0d8d1..abcb9c5122f70 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
@@ -216,3 +216,45 @@ void foo() {
 }
 
 } // namespace conditional_op
+
+namespace local_assignment_basic {
+
+RefCountable *provide_ref_ctnbl();
+
+void foo(RefCountable* a) {
+  RefCountable* b = a;
+  // expected-warning@-1{{Local variable 'b' is uncounted and unsafe 
[alpha.webkit.UncountedLocalVarsChecker]}}

[clang] [alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. (PR #92639)

2024-05-23 Thread Ryosuke Niwa via cfe-commits 

rniwa wrote:

> (It might be a good idea to add comments to those parts of the code to make 
> sure the reader knows that it was intentional.)

Ok, I was gonna say I'd add tests and then realized that the error message says 
"local variable" to both static local as well as global variables. That's 
probably misleading so will correct that.

https://github.com/llvm/llvm-project/pull/92639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. (PR #92639)

2024-05-23 Thread Artem Dergachev via cfe-commits 

haoNoQ wrote:

(It might be a good idea to add comments to those parts of the code to make 
sure the reader knows that it was intentional.)

https://github.com/llvm/llvm-project/pull/92639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. (PR #92639)

2024-05-23 Thread Artem Dergachev via cfe-commits 

https://github.com/haoNoQ approved this pull request.

Aha in this case LGTM!

https://github.com/llvm/llvm-project/pull/92639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. (PR #92639)

2024-05-23 Thread Ryosuke Niwa via cfe-commits 


@@ -135,7 +135,19 @@ class UncountedLocalVarsChecker
   bool shouldVisitImplicitCode() const { return false; }
 
   bool VisitVarDecl(VarDecl *V) {
-Checker->visitVarDecl(V);
+auto *Init = V->getInit();
+if (Init && V->isLocalVarDecl())
+  Checker->visitVarDecl(V, Init);
+return true;
+  }
+
+  bool VisitBinaryOperator(const BinaryOperator *BO) {
+if (BO->isAssignmentOp()) {
+  if (auto *VarRef = dyn_cast(BO->getLHS())) {
+if (auto *V = dyn_cast(VarRef->getDecl()))

rniwa wrote:

Hm... I don't think so. I think any assignment to a global variable is also bad 
if it's a raw pointer to a ref counted type. It's generally an anti-pattern to 
avoid.

This makes me think that the checker should probably be renamed to 
"UncountedVarDeclChecker" or something.

https://github.com/llvm/llvm-project/pull/92639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. (PR #92639)

2024-05-23 Thread Ryosuke Niwa via cfe-commits 


@@ -135,7 +135,19 @@ class UncountedLocalVarsChecker
   bool shouldVisitImplicitCode() const { return false; }
 
   bool VisitVarDecl(VarDecl *V) {
-Checker->visitVarDecl(V);
+auto *Init = V->getInit();
+if (Init && V->isLocalVarDecl())

rniwa wrote:

Oh, interesting. I *think* we do want to check static local variables as well 
since there is nothing preventing static local variable to be a raw pointer to 
a ref counted type, which is also bad.

https://github.com/llvm/llvm-project/pull/92639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. (PR #92639)

2024-05-23 Thread Artem Dergachev via cfe-commits 


@@ -135,7 +135,19 @@ class UncountedLocalVarsChecker
   bool shouldVisitImplicitCode() const { return false; }
 
   bool VisitVarDecl(VarDecl *V) {
-Checker->visitVarDecl(V);
+auto *Init = V->getInit();
+if (Init && V->isLocalVarDecl())

haoNoQ wrote:

This method says "yes" to static locals. Are we ok with that?

(These methods are super confusing so I'd rather double-check.)

https://github.com/llvm/llvm-project/pull/92639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. (PR #92639)

2024-05-23 Thread Artem Dergachev via cfe-commits 


@@ -135,7 +135,19 @@ class UncountedLocalVarsChecker
   bool shouldVisitImplicitCode() const { return false; }
 
   bool VisitVarDecl(VarDecl *V) {
-Checker->visitVarDecl(V);
+auto *Init = V->getInit();
+if (Init && V->isLocalVarDecl())
+  Checker->visitVarDecl(V, Init);
+return true;
+  }
+
+  bool VisitBinaryOperator(const BinaryOperator *BO) {
+if (BO->isAssignmentOp()) {
+  if (auto *VarRef = dyn_cast(BO->getLHS())) {
+if (auto *V = dyn_cast(VarRef->getDecl()))

haoNoQ wrote:

Would you also like to skip globals here? (But you probably don't want to skip 
parameters here – only in the initializer case.)

https://github.com/llvm/llvm-project/pull/92639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. (PR #92639)

2024-05-18 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang-static-analyzer-1

Author: Ryosuke Niwa (rniwa)


Changes

This PR updates alpha.webkit.UncountedLocalVarsChecker to emit warnings for 
assignments to uncounted local variable and parameters instead of just the 
initialization during the declaration.

---
Full diff: https://github.com/llvm/llvm-project/pull/92639.diff


2 Files Affected:

- (modified) 
clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp (+37-24) 
- (modified) clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp (+42) 


``diff
diff --git 
a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
index 0d9710a5e2d83..6c0d56303d5ad 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
@@ -135,7 +135,19 @@ class UncountedLocalVarsChecker
   bool shouldVisitImplicitCode() const { return false; }
 
   bool VisitVarDecl(VarDecl *V) {
-Checker->visitVarDecl(V);
+auto *Init = V->getInit();
+if (Init && V->isLocalVarDecl())
+  Checker->visitVarDecl(V, Init);
+return true;
+  }
+
+  bool VisitBinaryOperator(const BinaryOperator *BO) {
+if (BO->isAssignmentOp()) {
+  if (auto *VarRef = dyn_cast(BO->getLHS())) {
+if (auto *V = dyn_cast(VarRef->getDecl()))
+  Checker->visitVarDecl(V, BO->getRHS());
+  }
+}
 return true;
   }
 
@@ -174,7 +186,7 @@ class UncountedLocalVarsChecker
 visitor.TraverseDecl(const_cast(TUD));
   }
 
-  void visitVarDecl(const VarDecl *V) const {
+  void visitVarDecl(const VarDecl *V, const Expr *Value) const {
 if (shouldSkipVarDecl(V))
   return;
 
@@ -184,12 +196,8 @@ class UncountedLocalVarsChecker
 
 std::optional IsUncountedPtr = isUncountedPtr(ArgType);
 if (IsUncountedPtr && *IsUncountedPtr) {
-  const Expr *const InitExpr = V->getInit();
-  if (!InitExpr)
-return; // FIXME: later on we might warn on uninitialized vars too
-
   if (tryToFindPtrOrigin(
-  InitExpr, /*StopAtFirstRefCountedObj=*/false,
+  Value, /*StopAtFirstRefCountedObj=*/false,
   [&](const clang::Expr *InitArgOrigin, bool IsSafe) {
 if (!InitArgOrigin)
   return true;
@@ -232,34 +240,39 @@ class UncountedLocalVarsChecker
   }))
 return;
 
-  reportBug(V);
+  reportBug(V, Value);
 }
   }
 
   bool shouldSkipVarDecl(const VarDecl *V) const {
 assert(V);
-if (!V->isLocalVarDecl())
-  return true;
-
-if (BR->getSourceManager().isInSystemHeader(V->getLocation()))
-  return true;
-
-return false;
+return BR->getSourceManager().isInSystemHeader(V->getLocation());
   }
 
-  void reportBug(const VarDecl *V) const {
+  void reportBug(const VarDecl *V, const Expr *Value) const {
 assert(V);
 SmallString<100> Buf;
 llvm::raw_svector_ostream Os(Buf);
 
-Os << "Local variable ";
-printQuotedQualifiedName(Os, V);
-Os << " is uncounted and unsafe.";
-
-PathDiagnosticLocation BSLoc(V->getLocation(), BR->getSourceManager());
-auto Report = std::make_unique(Bug, Os.str(), BSLoc);
-Report->addRange(V->getSourceRange());
-BR->emitReport(std::move(Report));
+if (dyn_cast(V)) {
+  Os << "Assignment to an uncounted parameter ";
+  printQuotedQualifiedName(Os, V);
+  Os << " is unsafe.";
+
+  PathDiagnosticLocation BSLoc(Value->getExprLoc(), 
BR->getSourceManager());
+  auto Report = std::make_unique(Bug, Os.str(), BSLoc);
+  Report->addRange(Value->getSourceRange());
+  BR->emitReport(std::move(Report));
+} else {
+  Os << "Local variable ";
+  printQuotedQualifiedName(Os, V);
+  Os << " is uncounted and unsafe.";
+
+  PathDiagnosticLocation BSLoc(V->getLocation(), BR->getSourceManager());
+  auto Report = std::make_unique(Bug, Os.str(), BSLoc);
+  Report->addRange(V->getSourceRange());
+  BR->emitReport(std::move(Report));
+}
   }
 };
 } // namespace
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp 
b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
index 632a82eb0d8d1..abcb9c5122f70 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
@@ -216,3 +216,45 @@ void foo() {
 }
 
 } // namespace conditional_op
+
+namespace local_assignment_basic {
+
+RefCountable *provide_ref_ctnbl();
+
+void foo(RefCountable* a) {
+  RefCountable* b = a;
+  // expected-warning@-1{{Local variable 'b' is uncounted and unsafe 
[alpha.webkit.UncountedLocalVarsChecker]}}
+  if (b->trivial())
+b = provide_ref_ctnbl();
+}
+
+void bar(RefCountable* a) {
+  RefCountable* b;
+  // expected-warning@-1{{Local

[clang] [alpha.webkit.UncountedLocalVarsChecker] Detect assignments to uncounted local variable and parameters. (PR #92639)

2024-05-18 Thread Ryosuke Niwa via cfe-commits 

https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/92639

This PR updates alpha.webkit.UncountedLocalVarsChecker to emit warnings for 
assignments to uncounted local variable and parameters instead of just the 
initialization during the declaration.

>From 5ae3b193a6ec3617c99297da5b8d276b0e5bbc01 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa 
Date: Sat, 18 May 2024 02:17:30 -0700
Subject: [PATCH] [alpha.webkit.UncountedLocalVarsChecker] Detect assignments
 to uncounted local variable and parameters.

This PR updates alpha.webkit.UncountedLocalVarsChecker to emit warnings for 
assignments to uncounted
local variable and parameters instead of just the initialization during the 
declaration.
---
 .../WebKit/UncountedLocalVarsChecker.cpp  | 61 +++
 .../Checkers/WebKit/uncounted-local-vars.cpp  | 42 +
 2 files changed, 79 insertions(+), 24 deletions(-)

diff --git 
a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
index 0d9710a5e2d83..6c0d56303d5ad 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
@@ -135,7 +135,19 @@ class UncountedLocalVarsChecker
   bool shouldVisitImplicitCode() const { return false; }
 
   bool VisitVarDecl(VarDecl *V) {
-Checker->visitVarDecl(V);
+auto *Init = V->getInit();
+if (Init && V->isLocalVarDecl())
+  Checker->visitVarDecl(V, Init);
+return true;
+  }
+
+  bool VisitBinaryOperator(const BinaryOperator *BO) {
+if (BO->isAssignmentOp()) {
+  if (auto *VarRef = dyn_cast(BO->getLHS())) {
+if (auto *V = dyn_cast(VarRef->getDecl()))
+  Checker->visitVarDecl(V, BO->getRHS());
+  }
+}
 return true;
   }
 
@@ -174,7 +186,7 @@ class UncountedLocalVarsChecker
 visitor.TraverseDecl(const_cast(TUD));
   }
 
-  void visitVarDecl(const VarDecl *V) const {
+  void visitVarDecl(const VarDecl *V, const Expr *Value) const {
 if (shouldSkipVarDecl(V))
   return;
 
@@ -184,12 +196,8 @@ class UncountedLocalVarsChecker
 
 std::optional IsUncountedPtr = isUncountedPtr(ArgType);
 if (IsUncountedPtr && *IsUncountedPtr) {
-  const Expr *const InitExpr = V->getInit();
-  if (!InitExpr)
-return; // FIXME: later on we might warn on uninitialized vars too
-
   if (tryToFindPtrOrigin(
-  InitExpr, /*StopAtFirstRefCountedObj=*/false,
+  Value, /*StopAtFirstRefCountedObj=*/false,
   [&](const clang::Expr *InitArgOrigin, bool IsSafe) {
 if (!InitArgOrigin)
   return true;
@@ -232,34 +240,39 @@ class UncountedLocalVarsChecker
   }))
 return;
 
-  reportBug(V);
+  reportBug(V, Value);
 }
   }
 
   bool shouldSkipVarDecl(const VarDecl *V) const {
 assert(V);
-if (!V->isLocalVarDecl())
-  return true;
-
-if (BR->getSourceManager().isInSystemHeader(V->getLocation()))
-  return true;
-
-return false;
+return BR->getSourceManager().isInSystemHeader(V->getLocation());
   }
 
-  void reportBug(const VarDecl *V) const {
+  void reportBug(const VarDecl *V, const Expr *Value) const {
 assert(V);
 SmallString<100> Buf;
 llvm::raw_svector_ostream Os(Buf);
 
-Os << "Local variable ";
-printQuotedQualifiedName(Os, V);
-Os << " is uncounted and unsafe.";
-
-PathDiagnosticLocation BSLoc(V->getLocation(), BR->getSourceManager());
-auto Report = std::make_unique(Bug, Os.str(), BSLoc);
-Report->addRange(V->getSourceRange());
-BR->emitReport(std::move(Report));
+if (dyn_cast(V)) {
+  Os << "Assignment to an uncounted parameter ";
+  printQuotedQualifiedName(Os, V);
+  Os << " is unsafe.";
+
+  PathDiagnosticLocation BSLoc(Value->getExprLoc(), 
BR->getSourceManager());
+  auto Report = std::make_unique(Bug, Os.str(), BSLoc);
+  Report->addRange(Value->getSourceRange());
+  BR->emitReport(std::move(Report));
+} else {
+  Os << "Local variable ";
+  printQuotedQualifiedName(Os, V);
+  Os << " is uncounted and unsafe.";
+
+  PathDiagnosticLocation BSLoc(V->getLocation(), BR->getSourceManager());
+  auto Report = std::make_unique(Bug, Os.str(), BSLoc);
+  Report->addRange(V->getSourceRange());
+  BR->emitReport(std::move(Report));
+}
   }
 };
 } // namespace
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp 
b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
index 632a82eb0d8d1..abcb9c5122f70 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
@@ -216,3 +216,45 @@ void foo() {
 }
 
 } // namespace conditional_op
+
+namespace local_assignment_basic {
+
+RefCountable