[clang] [llvm] [analyzer] Accept C library functions from the `std` namespace (PR #84469)
=?utf-8?q?Donát?= Nagy , =?utf-8?q?Donát?= Nagy ,Balazs Benics ,NagyDonat Message-ID: In-Reply-To: steakhal wrote: I proposed the PR #84963 for relanding this, including the fix for the use-after-free of the AST of the unittest. https://github.com/llvm/llvm-project/pull/84469 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer] Accept C library functions from the `std` namespace (PR #84469)
=?utf-8?q?Donát?= Nagy , =?utf-8?q?Donát?= Nagy ,Balazs Benics ,NagyDonat Message-ID: In-Reply-To: steakhal wrote: Thanks! Indeed, it looks like valgrind also complains: ``` [ RUN ] IsCLibraryFunctionTest.AcceptsGlobal ==999215== Invalid read of size 1 ==999215==at 0x15420D0: hasAttrs (DeclBase.h:523) ==999215==by 0x15420D0: getAttr (DeclBase.h:579) ==999215==by 0x15420D0: clang::FunctionDecl::getBuiltinID(bool) const (???:3597) ==999215==by 0x1D20BE0: clang::ento::CheckerContext::isCLibraryFunction(clang::FunctionDecl const*, llvm::StringRef) (../../clang/lib/StaticAnalyzer/Core/CheckerContext.cpp:54) ==999215==by 0x117DACC: IsCLibraryFunctionTest_AcceptsGlobal_Test::TestBody() (../../clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp:40) ==999215==by 0x12D9778: HandleExceptionsInMethodIfSupported (gtest.cc:0) ==999215==by 0x12D9778: testing::Test::Run() (gtest.cc:2687) ==999215==by 0x12DAB6D: testing::TestInfo::Run() (gtest.cc:2836) ==999215==by 0x12DBE84: testing::TestSuite::Run() (gtest.cc:3015) ==999215==by 0x12EAA3D: testing::internal::UnitTestImpl::RunAllTests() (gtest.cc:5920) ==999215==by 0x12E9E6E: HandleExceptionsInMethodIfSupported (gtest.cc:0) ==999215==by 0x12E9E6E: testing::UnitTest::Run() (gtest.cc:5484) ==999215==by 0x12C71EC: RUN_ALL_TESTS (gtest.h:2317) ==999215==by 0x12C71EC: main (???:55) ``` I'll look into this. https://github.com/llvm/llvm-project/pull/84469 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer] Accept C library functions from the `std` namespace (PR #84469)
=?utf-8?q?Donát?= Nagy , =?utf-8?q?Donát?= Nagy ,Balazs Benics ,NagyDonat Message-ID: In-Reply-To: NagyDonat wrote: @steakhal It seems that the unit tests that you suggested fail on many different buildbots: - https://lab.llvm.org/buildbot/#/builders/86/builds/76040 - https://lab.llvm.org/buildbot/#/builders/265/builds/3177 - https://lab.llvm.org/buildbot/#/builders/231/builds/21515 - https://lab.llvm.org/buildbot/#/builders/123/builds/25486 - https://lab.llvm.org/buildbot/#/builders/60/builds/16359 - https://lab.llvm.org/buildbot/#/builders/236/builds/9980 - https://lab.llvm.org/buildbot/#/builders/93/builds/19173 - https://lab.llvm.org/buildbot/#/builders/5/builds/41701 - https://lab.llvm.org/buildbot/#/builders/168/builds/19170 - https://lab.llvm.org/buildbot/#/builders/72/builds/3087 - https://lab.llvm.org/buildbot/#/builders/74/builds/26589 Most of these reports are either crashes or memory sanitizer reports with messages like > [--] 1 test from IsCLibraryFunctionTest [ RUN ] IsCLibraryFunctionTest.AcceptsGlobal ==3810031==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x5599a8127045 in getAttr /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/clang/include/clang/AST/DeclBase.h:579:12 #1 0x5599a8127045 in clang::FunctionDecl::getBuiltinID(bool) const /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/clang/lib/AST/Decl.cpp:3597:26 #2 0x5599a97495d6 in clang::ento::CheckerContext::isCLibraryFunction(clang::FunctionDecl const*, llvm::StringRef) /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp:54:22 #3 0x5599a776b8a1 in IsCLibraryFunctionTest_AcceptsGlobal_Test::TestBody() /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp:40:3 #4 0x5599a7ae1a3a in HandleExceptionsInMethodIfSupported /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/third-party/unittest/googletest/src/gtest.cc #5 0x5599a7ae1a3a in testing::Test::Run() /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/third-party/unittest/googletest/src/gtest.cc:2687:5 [rest of stacktrace omitted] SUMMARY: MemorySanitizer: use-of-uninitialized-value /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/clang/include/clang/AST/DeclBase.h:579:12 in getAttr but there are also some "managed to execute it, but the test failed" situations. As similar issues did not appear when the method `isCLibraryFunction` was used in "real" invocations, I suspect that these issues might be caused by imperfections of the unit test environment (e.g. perhaps it doesn't initialize the builtin ID table, but the tests still happen to pass on linux). https://github.com/llvm/llvm-project/pull/84469 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer] Accept C library functions from the `std` namespace (PR #84469)
=?utf-8?q?Donát?= Nagy , =?utf-8?q?Donát?= Nagy ,Balazs Benics ,NagyDonat Message-ID: In-Reply-To: https://github.com/NagyDonat closed https://github.com/llvm/llvm-project/pull/84469 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer] Accept C library functions from the `std` namespace (PR #84469)
=?utf-8?q?Donát?= Nagy ,
=?utf-8?q?Donát?= Nagy ,Balazs Benics
,NagyDonat
Message-ID:
In-Reply-To:
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?=
Date: Fri, 8 Mar 2024 13:06:01 +0100
Subject: [PATCH 1/5] [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 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 .
+/// (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 ).
+ 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?=
Date: Fri, 8 Mar 2024 13:22:27 +0100
Subject: [PATCH 2/5] 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
[clang] [llvm] [analyzer] Accept C library functions from the `std` namespace (PR #84469)
=?utf-8?q?Don=C3=A1t?= Nagy , =?utf-8?q?Don=C3=A1t?= Nagy ,Balazs Benics Message-ID: In-Reply-To: https://github.com/steakhal approved this pull request. https://github.com/llvm/llvm-project/pull/84469 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer] Accept C library functions from the `std` namespace (PR #84469)
=?utf-8?q?Donát?= Nagy ,
=?utf-8?q?Donát?= Nagy ,Balazs Benics
Message-ID:
In-Reply-To:
@@ -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
+
+using namespace clang;
+using namespace ento;
+using namespace ast_matchers;
+
+testing::AssertionResult extractFunctionDecl(StringRef Code,
+ const FunctionDecl *) {
+ auto ASTUnit = tooling::buildASTFromCode(Code);
+ if (!ASTUnit)
+return testing::AssertionFailure() << "AST construction failed";
+
+ ASTContext = 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("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?
+}
NagyDonat wrote:
> Hm, can't stdlib functions have inline, is it disallowed?
I presume that there are _some_ inline functions in _some_ stdlib under
_certain_ platforms, because otherwise the "Note that we make an exception for
inline functions" part would not be present in the code.
https://github.com/llvm/llvm-project/pull/84469
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer] Accept C library functions from the `std` namespace (PR #84469)
=?utf-8?q?Don=C3=A1t?= Nagy ,
=?utf-8?q?Don=C3=A1t?= Nagy ,Balazs Benics
Message-ID:
In-Reply-To:
@@ -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
+
+using namespace clang;
+using namespace ento;
+using namespace ast_matchers;
+
+testing::AssertionResult extractFunctionDecl(StringRef Code,
+ const FunctionDecl *) {
+ auto ASTUnit = tooling::buildASTFromCode(Code);
+ if (!ASTUnit)
+return testing::AssertionFailure() << "AST construction failed";
+
+ ASTContext = 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("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?
+}
steakhal wrote:
Yes, stdlib functions should be "extern".
https://github.com/llvm/llvm-project/pull/84469
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer] Accept C library functions from the `std` namespace (PR #84469)
=?utf-8?q?Donát?= Nagy ,
=?utf-8?q?Donát?= Nagy ,Balazs Benics
Message-ID:
In-Reply-To:
@@ -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
+
+using namespace clang;
+using namespace ento;
+using namespace ast_matchers;
+
+testing::AssertionResult extractFunctionDecl(StringRef Code,
+ const FunctionDecl *) {
+ auto ASTUnit = tooling::buildASTFromCode(Code);
+ if (!ASTUnit)
+return testing::AssertionFailure() << "AST construction failed";
+
+ ASTContext = 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("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?
+}
steakhal wrote:
AH, sure. It makes complete sense! Hm, can't stdlib functions have `inline`, is
it disallowed?
https://github.com/llvm/llvm-project/pull/84469
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer] Accept C library functions from the `std` namespace (PR #84469)
=?utf-8?q?Donát?= Nagy , =?utf-8?q?Donát?= Nagy ,Balazs Benics Message-ID: In-Reply-To: https://github.com/NagyDonat edited https://github.com/llvm/llvm-project/pull/84469 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer] Accept C library functions from the `std` namespace (PR #84469)
=?utf-8?q?Donát?= Nagy ,
=?utf-8?q?Donát?= Nagy ,Balazs Benics
Message-ID:
In-Reply-To:
@@ -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
+
+using namespace clang;
+using namespace ento;
+using namespace ast_matchers;
+
+testing::AssertionResult extractFunctionDecl(StringRef Code,
+ const FunctionDecl *) {
+ auto ASTUnit = tooling::buildASTFromCode(Code);
+ if (!ASTUnit)
+return testing::AssertionFailure() << "AST construction failed";
+
+ ASTContext = 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("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?
+}
NagyDonat wrote:
```suggestion
TEST(IsCLibraryFunctionTest, RejectsNoInlineNoExternalLinkage) {
// Functions that are neither inlined nor externally visible cannot be C
library functions.
const FunctionDecl *Result;
ASSERT_TRUE(extractFunctionDecl(R"cpp(static void fun();)cpp", Result));
EXPECT_FALSE(CheckerContext::isCLibraryFunction(Result));
}
```
https://github.com/llvm/llvm-project/pull/84469
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer] Accept C library functions from the `std` namespace (PR #84469)
=?utf-8?q?Donát?= Nagy ,
=?utf-8?q?Donát?= Nagy ,Balazs Benics
Message-ID:
In-Reply-To:
@@ -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
+
+using namespace clang;
+using namespace ento;
+using namespace ast_matchers;
+
+testing::AssertionResult extractFunctionDecl(StringRef Code,
+ const FunctionDecl *) {
+ auto ASTUnit = tooling::buildASTFromCode(Code);
+ if (!ASTUnit)
+return testing::AssertionFailure() << "AST construction failed";
+
+ ASTContext = 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("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?
+}
NagyDonat wrote:
The method `isCLibraryFunction()` applies a heuristic that C library functions
must be externally visible (unless they're inlined):
```cpp
// If this function is not externally visible, it is not a C library function.
// Note that we make an exception for inline functions, which may be
// declared in header files without external linkage.
if (!FD->isInlined() && !FD->isExternallyVisible())
return false;
```
I think this is a reasonable heuristic that should not cause false negatives (I
don't think that we can have a library function that isn't inlined and isn't
externally visible) and might prevent a few false positives.
https://github.com/llvm/llvm-project/pull/84469
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer] Accept C library functions from the `std` namespace (PR #84469)
=?utf-8?q?Donát?= Nagy ,
=?utf-8?q?Donát?= Nagy ,Balazs Benics
Message-ID:
In-Reply-To:
@@ -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
+
+using namespace clang;
+using namespace ento;
+using namespace ast_matchers;
+
+testing::AssertionResult extractFunctionDecl(StringRef Code,
+ const FunctionDecl *) {
+ auto ASTUnit = tooling::buildASTFromCode(Code);
+ if (!ASTUnit)
+return testing::AssertionFailure() << "AST construction failed";
+
+ ASTContext = 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("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?
+}
steakhal wrote:
What do you all think about this case?
I thought `static` means the same for `C` in this context, and as such it
should be accepted by `isCLibraryFunction` - at least that's what I'd expect.
This behavior was likely broken before, thus I don't expect direct actions on
this.
https://github.com/llvm/llvm-project/pull/84469
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer] Accept C library functions from the `std` namespace (PR #84469)
=?utf-8?q?Don=C3=A1t?= Nagy , =?utf-8?q?Don=C3=A1t?= Nagy ,Balazs Benics Message-ID: In-Reply-To: https://github.com/steakhal approved this pull request. I'm good now. https://github.com/llvm/llvm-project/pull/84469 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer] Accept C library functions from the `std` namespace (PR #84469)
=?utf-8?q?Donát?= Nagy , =?utf-8?q?Donát?= Nagy ,Balazs Benics Message-ID: In-Reply-To: https://github.com/steakhal edited https://github.com/llvm/llvm-project/pull/84469 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer] Accept C library functions from the `std` namespace (PR #84469)
=?utf-8?q?Don=C3=A1t?= Nagy , =?utf-8?q?Don=C3=A1t?= Nagy ,Balazs Benics Message-ID: In-Reply-To: NagyDonat wrote: @steakhal Thanks for contributing the unit tests, I applied your commit. https://github.com/llvm/llvm-project/pull/84469 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [analyzer] Accept C library functions from the `std` namespace (PR #84469)
=?utf-8?q?Donát?= Nagy ,
=?utf-8?q?Donát?= Nagy ,Balazs Benics
Message-ID:
In-Reply-To:
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?=
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 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 .
+/// (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 ).
+ 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?=
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
