Skip to content

[analyzer] Accept C library functions from the std namespace #84469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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 namespace `std`.)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well go the extra mile and mention an example, like std::malloc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think saying "functions from the C standard library" (at the beginning of this comment block) is clear enough, mentioning malloc is redundant after it. Also, technically, we are not using this mode for malloc yet.

CLibrary,

/// Matches "simple" functions that are not methods. (Static methods are
Expand Down
8 changes: 5 additions & 3 deletions clang/lib/StaticAnalyzer/Core/CheckerContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,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>).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

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.
Expand Down
1 change: 1 addition & 0 deletions clang/unittests/StaticAnalyzer/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ add_clang_unittest(StaticAnalysisTests
CallEventTest.cpp
ConflictingEvalCallsTest.cpp
FalsePositiveRefutationBRVisitorTest.cpp
IsCLibraryFunctionTest.cpp
NoStateChangeFuncVisitorTest.cpp
ParamRegionTest.cpp
RangeSetTest.cpp
Expand Down
89 changes: 89 additions & 0 deletions clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp
Original file line number Diff line number Diff line change
@@ -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, 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));
}

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));
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ unittest("StaticAnalysisTests") {
"CallEventTest.cpp",
"ConflictingEvalCallsTest.cpp",
"FalsePositiveRefutationBRVisitorTest.cpp",
"IsCLibraryFunctionTest.cpp",
"NoStateChangeFuncVisitorTest.cpp",
"ParamRegionTest.cpp",
"RangeSetTest.cpp",
Expand Down