=?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com>, =?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com>,Balazs Benics <benicsbal...@gmail.com> Message-ID: In-Reply-To: <llvm.org/llvm/llvm-project/pull/84...@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.n...@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.n...@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.n...@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 <benicsbal...@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", _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
