[clang] [llvm] [analyzer] Accept C library functions from the `std` namespace (PR #84469)

via llvm-commits llvm-commits at lists.llvm.org
Mon Mar 11 06:14:44 PDT 2024


=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>,Balazs Benics
 <benicsbalazs at gmail.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/84469 at github.com>


https://github.com/NagyDonat updated https://github.com/llvm/llvm-project/pull/84469

>From c357aa998204e6693430c801f5b7d3a9e5e09e37 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Fri, 8 Mar 2024 13:06:01 +0100
Subject: [PATCH 1/4] [analyzer] Accept C library functions from the `std`
 namespace

Previously, the function `isCLibraryFunction()` and logic relying on it
only accepted functions that are declared directly within a TU (i.e. not
in a namespace or a class). However C++ headers like <cstdlib> declare
many C standard library functions within the namespace `std`, so this
commit ensures that functions within the namespace `std` are also
accepted.

After this commit it will be possible to match functions like `malloc` or
`free` with `CallDescription::Mode::CLibrary`.
---
 .../StaticAnalyzer/Core/PathSensitive/CallDescription.h  | 8 ++------
 clang/lib/StaticAnalyzer/Core/CheckerContext.cpp         | 9 ++++++---
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
index 3432d2648633c2..7da65734a44cf3 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
@@ -41,12 +41,8 @@ class CallDescription {
     ///  - We also accept calls where the number of arguments or parameters is
     ///    greater than the specified value.
     /// For the exact heuristics, see CheckerContext::isCLibraryFunction().
-    /// Note that functions whose declaration context is not a TU (e.g.
-    /// methods, functions in namespaces) are not accepted as C library
-    /// functions.
-    /// FIXME: If I understand it correctly, this discards calls where C++ code
-    /// refers a C library function through the namespace `std::` via headers
-    /// like <cstdlib>.
+    /// (This mode only matches functions that are declared either directly
+    /// within a TU or in the `std::` namespace.)
     CLibrary,
 
     /// Matches "simple" functions that are not methods. (Static methods are
diff --git a/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp b/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp
index d6d4cec9dd3d4d..c1ae9b441d98d1 100644
--- a/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp
@@ -12,6 +12,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/Analysis/AnalysisDeclContext.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/StringExtras.h"
@@ -87,9 +88,11 @@ bool CheckerContext::isCLibraryFunction(const FunctionDecl *FD,
   if (!II)
     return false;
 
-  // Look through 'extern "C"' and anything similar invented in the future.
-  // If this function is not in TU directly, it is not a C library function.
-  if (!FD->getDeclContext()->getRedeclContext()->isTranslationUnit())
+  // C library functions are either declared directly within a TU (the common
+  // case) or they are accessed through the namespace `std::` (when they are
+  // used in C++ via headers like <cstdlib>).
+  if (!FD->getDeclContext()->getRedeclContext()->isTranslationUnit() &&
+          !AnalysisDeclContext::isInStdNamespace(FD))
     return false;
 
   // If this function is not externally visible, it is not a C library function.

>From 6afadd86a6ee790f58c7339dc019d8c8eac8a6b9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Fri, 8 Mar 2024 13:22:27 +0100
Subject: [PATCH 2/4] Don't match methods from the namespace `std`

Only accept those functions whose declaration is _directly_ within the
namespace `std` (that is, not within a class or a sub-namespace).
Transparent declaration contexts (e.g. `extern "C"`) are still allowed,
but this prevents matching methods.
---
 clang/lib/StaticAnalyzer/Core/CheckerContext.cpp | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp b/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp
index c1ae9b441d98d1..5e706d17eeae14 100644
--- a/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp
@@ -12,7 +12,6 @@
 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
-#include "clang/Analysis/AnalysisDeclContext.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/StringExtras.h"
@@ -91,8 +90,8 @@ bool CheckerContext::isCLibraryFunction(const FunctionDecl *FD,
   // C library functions are either declared directly within a TU (the common
   // case) or they are accessed through the namespace `std::` (when they are
   // used in C++ via headers like <cstdlib>).
-  if (!FD->getDeclContext()->getRedeclContext()->isTranslationUnit() &&
-          !AnalysisDeclContext::isInStdNamespace(FD))
+  const DeclContext *DC = FD->getDeclContext()->getRedeclContext();
+  if (!(DC->isTranslationUnit() || DC->isStdNamespace()))
     return false;
 
   // If this function is not externally visible, it is not a C library function.

>From 2cc0c75aeaa68a5b2d981b5d96b0ee109c7b410f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Fri, 8 Mar 2024 13:26:35 +0100
Subject: [PATCH 3/4] Tweak comments, fix word order

---
 .../clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h | 2 +-
 clang/lib/StaticAnalyzer/Core/CheckerContext.cpp              | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
index 7da65734a44cf3..b4e1636130ca7c 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
@@ -42,7 +42,7 @@ class CallDescription {
     ///    greater than the specified value.
     /// For the exact heuristics, see CheckerContext::isCLibraryFunction().
     /// (This mode only matches functions that are declared either directly
-    /// within a TU or in the `std::` namespace.)
+    /// within a TU or in the namespace `std`.)
     CLibrary,
 
     /// Matches "simple" functions that are not methods. (Static methods are
diff --git a/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp b/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp
index 5e706d17eeae14..1a9bff529e9bb1 100644
--- a/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp
@@ -88,8 +88,8 @@ bool CheckerContext::isCLibraryFunction(const FunctionDecl *FD,
     return false;
 
   // C library functions are either declared directly within a TU (the common
-  // case) or they are accessed through the namespace `std::` (when they are
-  // used in C++ via headers like <cstdlib>).
+  // case) or they are accessed through the namespace `std` (when they are used
+  // in C++ via headers like <cstdlib>).
   const DeclContext *DC = FD->getDeclContext()->getRedeclContext();
   if (!(DC->isTranslationUnit() || DC->isStdNamespace()))
     return false;

>From a15d6bc2b33628e29bcdaf11851af0f524c35d2b Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Fri, 8 Mar 2024 17:26:49 +0100
Subject: [PATCH 4/4] Add unittests

---
 clang/unittests/StaticAnalyzer/CMakeLists.txt |  1 +
 .../StaticAnalyzer/IsCLibraryFunctionTest.cpp | 89 +++++++++++++++++++
 .../clang/unittests/StaticAnalyzer/BUILD.gn   |  1 +
 3 files changed, 91 insertions(+)
 create mode 100644 clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp

diff --git a/clang/unittests/StaticAnalyzer/CMakeLists.txt b/clang/unittests/StaticAnalyzer/CMakeLists.txt
index 775f0f8486b8f9..db56e77331b821 100644
--- a/clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ b/clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -11,6 +11,7 @@ add_clang_unittest(StaticAnalysisTests
   CallEventTest.cpp
   ConflictingEvalCallsTest.cpp
   FalsePositiveRefutationBRVisitorTest.cpp
+  IsCLibraryFunctionTest.cpp
   NoStateChangeFuncVisitorTest.cpp
   ParamRegionTest.cpp
   RangeSetTest.cpp
diff --git a/clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp b/clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp
new file mode 100644
index 00000000000000..e1dea3458bc128
--- /dev/null
+++ b/clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp
@@ -0,0 +1,89 @@
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/AnalysisDeclContext.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+#include <memory>
+
+using namespace clang;
+using namespace ento;
+using namespace ast_matchers;
+
+testing::AssertionResult extractFunctionDecl(StringRef Code,
+                                             const FunctionDecl *&Result) {
+  auto ASTUnit = tooling::buildASTFromCode(Code);
+  if (!ASTUnit)
+    return testing::AssertionFailure() << "AST construction failed";
+
+  ASTContext &Context = ASTUnit->getASTContext();
+  if (Context.getDiagnostics().hasErrorOccurred())
+    return testing::AssertionFailure() << "Compilation error";
+
+  auto Matches = ast_matchers::match(functionDecl().bind("fn"), Context);
+  if (Matches.empty())
+    return testing::AssertionFailure() << "No function declaration found";
+
+  if (Matches.size() > 1)
+    return testing::AssertionFailure()
+           << "Multiple function declarations found";
+
+  Result = Matches[0].getNodeAs<FunctionDecl>("fn");
+  return testing::AssertionSuccess();
+}
+
+TEST(IsCLibraryFunctionTest, AcceptsGlobal) {
+  const FunctionDecl *Result;
+  ASSERT_TRUE(extractFunctionDecl(R"cpp(void fun();)cpp", Result));
+  EXPECT_TRUE(CheckerContext::isCLibraryFunction(Result));
+}
+
+TEST(IsCLibraryFunctionTest, AcceptsExternCGlobal) {
+  const FunctionDecl *Result;
+  ASSERT_TRUE(
+      extractFunctionDecl(R"cpp(extern "C" { void fun(); })cpp", Result));
+  EXPECT_TRUE(CheckerContext::isCLibraryFunction(Result));
+}
+
+TEST(IsCLibraryFunctionTest, AcceptsStaticGlobal) {
+  const FunctionDecl *Result;
+  ASSERT_TRUE(extractFunctionDecl(R"cpp(static void fun();)cpp", Result));
+  EXPECT_FALSE(CheckerContext::isCLibraryFunction(Result));
+  // FIXME: Shouldn't this be TRUE?
+}
+
+TEST(IsCLibraryFunctionTest, RejectsAnonymousNamespace) {
+  const FunctionDecl *Result;
+  ASSERT_TRUE(
+      extractFunctionDecl(R"cpp(namespace { void fun(); })cpp", Result));
+  EXPECT_FALSE(CheckerContext::isCLibraryFunction(Result));
+}
+
+TEST(IsCLibraryFunctionTest, AcceptsStdNamespace) {
+  const FunctionDecl *Result;
+  ASSERT_TRUE(
+      extractFunctionDecl(R"cpp(namespace std { void fun(); })cpp", Result));
+  EXPECT_TRUE(CheckerContext::isCLibraryFunction(Result));
+}
+
+TEST(IsCLibraryFunctionTest, RejectsOtherNamespaces) {
+  const FunctionDecl *Result;
+  ASSERT_TRUE(
+      extractFunctionDecl(R"cpp(namespace stdx { void fun(); })cpp", Result));
+  EXPECT_FALSE(CheckerContext::isCLibraryFunction(Result));
+}
+
+TEST(IsCLibraryFunctionTest, RejectsClassStatic) {
+  const FunctionDecl *Result;
+  ASSERT_TRUE(
+      extractFunctionDecl(R"cpp(class A { static void fun(); };)cpp", Result));
+  EXPECT_FALSE(CheckerContext::isCLibraryFunction(Result));
+}
+
+TEST(IsCLibraryFunctionTest, RejectsClassMember) {
+  const FunctionDecl *Result;
+  ASSERT_TRUE(extractFunctionDecl(R"cpp(class A { void fun(); };)cpp", Result));
+  EXPECT_FALSE(CheckerContext::isCLibraryFunction(Result));
+}
diff --git a/llvm/utils/gn/secondary/clang/unittests/StaticAnalyzer/BUILD.gn b/llvm/utils/gn/secondary/clang/unittests/StaticAnalyzer/BUILD.gn
index 01c2b6ced3366f..9c240cff181634 100644
--- a/llvm/utils/gn/secondary/clang/unittests/StaticAnalyzer/BUILD.gn
+++ b/llvm/utils/gn/secondary/clang/unittests/StaticAnalyzer/BUILD.gn
@@ -19,6 +19,7 @@ unittest("StaticAnalysisTests") {
     "CallEventTest.cpp",
     "ConflictingEvalCallsTest.cpp",
     "FalsePositiveRefutationBRVisitorTest.cpp",
+    "IsCLibraryFunctionTest.cpp",
     "NoStateChangeFuncVisitorTest.cpp",
     "ParamRegionTest.cpp",
     "RangeSetTest.cpp",



More information about the llvm-commits mailing list