[analyzer] Accept C library functions from the std namespace
#84469
Conversation
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`.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: None (NagyDonat) ChangesPreviously, the function After this commit it will be possible to match functions like Full diff: https://github.com/llvm/llvm-project/pull/84469.diff 2 Files Affected:
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.
|
You can test this locally with the following command:git-clang-format --diff 6a0618a0289cb0c23ef3e5c820418650cc1d0fdc 4e91ad4be0a9055e0b86e27dbe22a18b8a08a8c5 -- clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h clang/lib/StaticAnalyzer/Core/CheckerContext.cppView the diff from clang-format here.diff --git a/clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp b/clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp
index 19c66cc6be..88f056ebe0 100644
--- a/clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp
+++ b/clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp
@@ -48,7 +48,8 @@ TEST(IsCLibraryFunctionTest, AcceptsExternCGlobal) {
}
TEST(IsCLibraryFunctionTest, RejectsNoInlineNoExternalLinkage) {
- // Functions that are neither inlined nor externally visible cannot be C library functions.
+ // 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));
|
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.
| /// 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`.) |
There was a problem hiding this comment.
Might as well go the extra mile and mention an example, like std::malloc.
There was a problem hiding this comment.
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.
| 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>). |
|
Currently MallocChecker doesn't use this This means that I cannot create a simple testcase that demonstrates the advantage of this commit: the limitations of However, my followup commits will soon add testcases that demonstrate the effects of this change, so I think it's a forgivable sin to merge this without a TC. |
|
I feel that I'm getting bogged down in this area with changes that are more general than what's strictly necessary and only tangentially related to my real goals... I'm not familiar with the unittest framework and if unittests were necessary for this change, then I'd sooner abandon it, because it's not worth the effort. (Disclaimer: this comment reflects my current annoyance, and is not necessarily aligned with my long-term opinion.) |
No worries! Here is what I had in mind: |
|
Once the other reviewers are happy, this looks good to me! |
This reverts commit df6a67f, which was accidentally pushed here instead of the branch corresponding to llvm#84469
|
@steakhal Thanks for contributing the unit tests, I applied your commit. |
| 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? | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The method isCLibraryFunction() applies a heuristic that C library functions must be externally visible (unless they're inlined):
// 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.
There was a problem hiding this comment.
Yes, stdlib functions should be "extern".
|
@steakhal It seems that the unit tests that you suggested fail on many different buildbots:
Most of these reports are either crashes or memory sanitizer reports with messages like
but there are also some "managed to execute it, but the test failed" situations. As similar issues did not appear when the method |
|
Thanks! I'll look into this. |
|
I proposed the PR #84963 for relanding this, including the fix for the use-after-free of the AST of the unittest. |
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 namespacestd, so this commit ensures that functions within the namespacestdare also accepted.After this commit it will be possible to match functions like
mallocorfreewithCallDescription::Mode::CLibrary.