Revert "[analyzer] Accept C library functions from the std namespace"
#84926
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (NagyDonat) ChangesReverts llvm/llvm-project#84469 because it causes buildbot failures. I'll examine them and re-submit the change. Full diff: https://github.com/llvm/llvm-project/pull/84926.diff 5 Files Affected:
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
index b4e1636130ca7c..3432d2648633c2 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
@@ -41,8 +41,12 @@ 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().
- /// (This mode only matches functions that are declared either directly
- /// within a TU or in the namespace `std`.)
+ /// 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>.
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 1a9bff529e9bb1..d6d4cec9dd3d4d 100644
--- a/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp
@@ -87,11 +87,9 @@ bool CheckerContext::isCLibraryFunction(const FunctionDecl *FD,
if (!II)
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>).
- const DeclContext *DC = FD->getDeclContext()->getRedeclContext();
- if (!(DC->isTranslationUnit() || DC->isStdNamespace()))
+ // 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())
return false;
// If this function is not externally visible, it is not a C library function.
diff --git a/clang/unittests/StaticAnalyzer/CMakeLists.txt b/clang/unittests/StaticAnalyzer/CMakeLists.txt
index db56e77331b821..775f0f8486b8f9 100644
--- a/clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ b/clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -11,7 +11,6 @@ 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
deleted file mode 100644
index 19c66cc6bee1eb..00000000000000
--- a/clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp
+++ /dev/null
@@ -1,89 +0,0 @@
-#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));
-}
diff --git a/llvm/utils/gn/secondary/clang/unittests/StaticAnalyzer/BUILD.gn b/llvm/utils/gn/secondary/clang/unittests/StaticAnalyzer/BUILD.gn
index 9c240cff181634..01c2b6ced3366f 100644
--- a/llvm/utils/gn/secondary/clang/unittests/StaticAnalyzer/BUILD.gn
+++ b/llvm/utils/gn/secondary/clang/unittests/StaticAnalyzer/BUILD.gn
@@ -19,7 +19,6 @@ unittest("StaticAnalysisTests") {
"CallEventTest.cpp",
"ConflictingEvalCallsTest.cpp",
"FalsePositiveRefutationBRVisitorTest.cpp",
- "IsCLibraryFunctionTest.cpp",
"NoStateChangeFuncVisitorTest.cpp",
"ParamRegionTest.cpp",
"RangeSetTest.cpp",
|
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: None (NagyDonat) ChangesReverts llvm/llvm-project#84469 because it causes buildbot failures. I'll examine them and re-submit the change. Full diff: https://github.com/llvm/llvm-project/pull/84926.diff 5 Files Affected:
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
index b4e1636130ca7c..3432d2648633c2 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
@@ -41,8 +41,12 @@ 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().
- /// (This mode only matches functions that are declared either directly
- /// within a TU or in the namespace `std`.)
+ /// 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>.
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 1a9bff529e9bb1..d6d4cec9dd3d4d 100644
--- a/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp
@@ -87,11 +87,9 @@ bool CheckerContext::isCLibraryFunction(const FunctionDecl *FD,
if (!II)
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>).
- const DeclContext *DC = FD->getDeclContext()->getRedeclContext();
- if (!(DC->isTranslationUnit() || DC->isStdNamespace()))
+ // 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())
return false;
// If this function is not externally visible, it is not a C library function.
diff --git a/clang/unittests/StaticAnalyzer/CMakeLists.txt b/clang/unittests/StaticAnalyzer/CMakeLists.txt
index db56e77331b821..775f0f8486b8f9 100644
--- a/clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ b/clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -11,7 +11,6 @@ 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
deleted file mode 100644
index 19c66cc6bee1eb..00000000000000
--- a/clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp
+++ /dev/null
@@ -1,89 +0,0 @@
-#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));
-}
diff --git a/llvm/utils/gn/secondary/clang/unittests/StaticAnalyzer/BUILD.gn b/llvm/utils/gn/secondary/clang/unittests/StaticAnalyzer/BUILD.gn
index 9c240cff181634..01c2b6ced3366f 100644
--- a/llvm/utils/gn/secondary/clang/unittests/StaticAnalyzer/BUILD.gn
+++ b/llvm/utils/gn/secondary/clang/unittests/StaticAnalyzer/BUILD.gn
@@ -19,7 +19,6 @@ unittest("StaticAnalysisTests") {
"CallEventTest.cpp",
"ConflictingEvalCallsTest.cpp",
"FalsePositiveRefutationBRVisitorTest.cpp",
- "IsCLibraryFunctionTest.cpp",
"NoStateChangeFuncVisitorTest.cpp",
"ParamRegionTest.cpp",
"RangeSetTest.cpp",
|
steakhal
added
the
skip-precommit-approval
steakhal
added a commit
to steakhal/llvm-project
that referenced
this pull request
Mar 12, 2024
…ce" (llvm#84926) This reverts commit f32b04d.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reverts #84469 because it causes buildbot failures. I'll examine them and re-submit the change.