Skip over std namespace in WebKit checkers.

[rniwa]

No description provided.

[llvmbot]

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

@llvm/pr-subscribers-clang

Author: Ryosuke Niwa (rniwa)

Changes

---

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

4 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp (+6)
• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp (+6)
• (modified) clang/test/Analysis/Checkers/WebKit/call-args.cpp (+27)
• (modified) clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp (+26)

diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
index ae494de58da3da..0ff1a27ff412b9 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
@@ -53,6 +53,12 @@ class UncountedCallArgsChecker
       bool shouldVisitTemplateInstantiations() const { return true; }
       bool shouldVisitImplicitCode() const { return false; }
 
+      bool TraverseNamespaceDecl(NamespaceDecl *Decl) {
+        if (safeGetName(Decl) == "std")
+          return true;
+        return RecursiveASTVisitor<LocalVisitor>::TraverseNamespaceDecl(Decl);
+      }
+
       bool TraverseClassTemplateDecl(ClassTemplateDecl *Decl) {
         if (isRefType(safeGetName(Decl)))
           return true;
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
index 6036ad58cf253c..b27400678a5790 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
@@ -134,6 +134,12 @@ class UncountedLocalVarsChecker
       bool shouldVisitTemplateInstantiations() const { return true; }
       bool shouldVisitImplicitCode() const { return false; }
 
+      bool TraverseNamespaceDecl(NamespaceDecl *Decl) {
+        if (safeGetName(Decl) == "std")
+          return true;
+        return RecursiveASTVisitor<LocalVisitor>::TraverseNamespaceDecl(Decl);
+      }
+
       bool VisitVarDecl(VarDecl *V) {
         Checker->visitVarDecl(V);
         return true;
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args.cpp b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
index 2a4b6bb1f1063a..3a1a2f9e92289e 100644
--- a/clang/test/Analysis/Checkers/WebKit/call-args.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
@@ -333,3 +333,30 @@ namespace cxx_member_operator_call {
     // expected-warning@-1{{Call argument for parameter 'bad' is uncounted and unsafe}}
   }
 }
+
+namespace std {
+
+template <typename T>
+T* other_function();
+
+template <typename T>
+void another_function(T*, T*);
+
+template <typename T>
+void some_function(T* a)
+{
+  another_function(other_function<T>(), a);
+}
+
+} // std
+
+namespace ignore_std_namespace {
+
+RefCountable *ref_counted();
+
+void foo() {
+  std::some_function(ref_counted());
+  // expected-warning@-1{{Call argument for parameter 'a' is uncounted and unsafe}}
+}
+
+} // ignore_std_namespace
\ No newline at end of file
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
index 00673e91f471ea..c8550b7e2e77f1 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
@@ -187,3 +187,29 @@ void bar() {
 }
 
 } // namespace ignore_for_if
+
+namespace std {
+
+void memcpy(void*, void*, unsigned long);
+
+template <typename T>
+void some_function(T* a, T* b)
+{
+  T* temp = new T;
+  memcpy(temp, a, sizeof(T));
+  memcpy(a, b, sizeof(T));
+  memcpy(b, temp, sizeof(T));
+  delete temp;
+}
+
+} // std
+
+namespace ignore_std_namespace {
+
+RefCountable *ref_counted();
+
+void foo() {
+  std::some_function(ref_counted(), ref_counted());
+}
+
+} // ignore_std_namespace

[haoNoQ]

Ok am I reading this right: this is about testing code that lives directly in namespace std:: but somehow still lives in your project right? Such as your custom modifications to standard classes, various overloads and specializations of standard things.

This patch does not affect the behavior of checkers when you're just passing raw pointers from user code to standard functions such as std::memcpy(). You still want to have a warning about those.

And generally you already don't have any warnings about things that's going on in system headers themselves. There's a separate suppression about that.

[haoNoQ]

(If yes, LGTM!)

[rniwa]

Ok am I reading this right: this is about testing code that lives directly in namespace std:: but somehow still lives in your project right? Such as your custom modifications to standard classes, various overloads and specializations of standard things.

No, this is about standard library code getting warnings.

This patch does not affect the behavior of checkers when you're just passing raw pointers from user code to standard functions such as std::memcpy(). You still want to have a warning about those.

Right. But we don't want code within standard library to give warnings.

And generally you already don't have any warnings about things that's going on in system headers themselves. There's a separate suppression about that.

Hm... it's odd that we're getting warnings there then.

[haoNoQ]

Hmm it doesn't look like we actually have that other suppression. It looks like the checker simply never warned in the standard library because there aren't WebKit classes in the standard library. But it looks like you've found a few cases where it actually happens? How does this happen? Is this because templates get instantiated with WTF classes as type arguments? Maybe a something completely different solution should be considered?

If we think it's actually a good idea to suppress warnings about standard functions, typically these suppressions are implemented with getSourceManager().isInSystemHeader(SLoc) checks, such as the one we actually have. This covers not only the standard library, but also other code outside of your project (basically everything that goes with -isystem as opposed to -I). Which may or may not be what we want.

I think it's likely that your solution is optimal but I still want to see some examples to understand what exactly we're dealing with.

[rniwa]

Closing this in favor of #91103.