[clang-tools-extra] 9d8c3ad - [clang-tidy] Change code of SignalHandlerCheck (NFC).
Balázs Kéri via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 25 06:54:31 PST 2022
Author: Balázs Kéri
Date: 2022-01-25T15:52:38+01:00
New Revision: 9d8c3ad94fad5dd5fb511af89c9e7c3679922ce0
URL: https://github.com/llvm/llvm-project/commit/9d8c3ad94fad5dd5fb511af89c9e7c3679922ce0
DIFF: https://github.com/llvm/llvm-project/commit/9d8c3ad94fad5dd5fb511af89c9e7c3679922ce0.diff
LOG: [clang-tidy] Change code of SignalHandlerCheck (NFC).
Using clang::CallGraph to get the called functions.
This makes a better foundation to improve support for
C++ and print the call chain.
Reviewed By: aaron.ballman
Differential Revision: https://reviews.llvm.org/D118016
Added:
Modified:
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
index 63478a954058..1fc19a8652e3 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
@@ -7,15 +7,9 @@
//===----------------------------------------------------------------------===//
#include "SignalHandlerCheck.h"
-#include "clang/AST/ASTContext.h"
-#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/Analysis/CallGraph.h"
-#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/DepthFirstIterator.h"
#include "llvm/ADT/STLExtras.h"
-#include "llvm/ADT/SmallVector.h"
-#include <iterator>
-#include <queue>
using namespace clang::ast_matchers;
@@ -42,7 +36,9 @@ struct OptionEnumMapping<
namespace bugprone {
-static bool isSystemCall(const FunctionDecl *FD) {
+namespace {
+
+bool isSystemCall(const FunctionDecl *FD) {
// Find a possible redeclaration in system header.
// FIXME: Looking at the canonical declaration is not the most exact way
// to do this.
@@ -73,6 +69,22 @@ static bool isSystemCall(const FunctionDecl *FD) {
FD->getCanonicalDecl()->getLocation());
}
+/// Given a call graph node of a function and another one that is called from
+/// this function, get a CallExpr of the corresponding function call.
+/// It is unspecified which call is found if multiple calls exist, but the order
+/// should be deterministic (depend only on the AST).
+Expr *findCallExpr(const CallGraphNode *Caller, const CallGraphNode *Callee) {
+ auto FoundCallee = llvm::find_if(
+ Caller->callees(), [Callee](const CallGraphNode::CallRecord &Call) {
+ return Call.Callee == Callee;
+ });
+ assert(FoundCallee != Caller->end() &&
+ "Callee should be called from the caller function here.");
+ return FoundCallee->CallExpr;
+}
+
+} // namespace
+
AST_MATCHER(FunctionDecl, isSystemCall) { return isSystemCall(&Node); }
SignalHandlerCheck::SignalHandlerCheck(StringRef Name,
@@ -117,68 +129,50 @@ void SignalHandlerCheck::check(const MatchFinder::MatchResult &Result) {
const auto *HandlerDecl =
Result.Nodes.getNodeAs<FunctionDecl>("handler_decl");
const auto *HandlerExpr = Result.Nodes.getNodeAs<DeclRefExpr>("handler_expr");
+ assert(SignalCall && HandlerDecl && HandlerExpr &&
+ "All of these should exist in a match here.");
+
+ if (CG.size() <= 1) {
+ // Call graph must be populated with the entire TU at the beginning.
+ // (It is possible to add a single function but the functions called from it
+ // are not analysed in this case.)
+ CG.addToCallGraph(const_cast<TranslationUnitDecl *>(
+ HandlerDecl->getTranslationUnitDecl()));
+ assert(CG.size() > 1 &&
+ "There should be at least one function added to call graph.");
+ }
- // Visit each function encountered in the callgraph only once.
- llvm::DenseSet<const FunctionDecl *> SeenFunctions;
-
- // The worklist of the callgraph visitation algorithm.
- std::deque<const CallExpr *> CalledFunctions;
-
- auto ProcessFunction = [&](const FunctionDecl *F, const Expr *CallOrRef) {
- // Ensure that canonical declaration is used.
- F = F->getCanonicalDecl();
-
- // Do not visit function if already encountered.
- if (!SeenFunctions.insert(F).second)
- return true;
-
- // Check if the call is allowed.
- // Non-system calls are not considered.
- if (isSystemCall(F)) {
- if (isSystemCallAllowed(F))
- return true;
-
- reportBug(F, CallOrRef, SignalCall, HandlerDecl);
-
- return false;
- }
-
- // Get the body of the encountered non-system call function.
- const FunctionDecl *FBody;
- if (!F->hasBody(FBody)) {
- reportBug(F, CallOrRef, SignalCall, HandlerDecl);
- return false;
- }
-
- // Collect all called functions.
- auto Matches = match(decl(forEachDescendant(callExpr().bind("call"))),
- *FBody, FBody->getASTContext());
- for (const auto &Match : Matches) {
- const auto *CE = Match.getNodeAs<CallExpr>("call");
- if (isa<FunctionDecl>(CE->getCalleeDecl()))
- CalledFunctions.push_back(CE);
- }
-
- return true;
- };
-
- if (!ProcessFunction(HandlerDecl, HandlerExpr))
+ // Check for special case when the signal handler itself is an unsafe external
+ // function.
+ if (!isFunctionAsyncSafe(HandlerDecl)) {
+ reportBug(HandlerDecl, HandlerExpr, SignalCall, HandlerDecl);
return;
+ }
- // Visit the definition of every function referenced by the handler function.
- // Check for allowed function calls.
- while (!CalledFunctions.empty()) {
- const CallExpr *FunctionCall = CalledFunctions.front();
- CalledFunctions.pop_front();
- // At insertion we have already ensured that only function calls are there.
- const auto *F = cast<FunctionDecl>(FunctionCall->getCalleeDecl());
-
- if (!ProcessFunction(F, FunctionCall))
- break;
+ CallGraphNode *HandlerNode = CG.getNode(HandlerDecl);
+ // Signal handler can be external but not unsafe, no call graph in this case.
+ if (!HandlerNode)
+ return;
+ // Start from signal handler and visit every function call.
+ for (auto Itr = llvm::df_begin(HandlerNode), ItrE = llvm::df_end(HandlerNode);
+ Itr != ItrE; ++Itr) {
+ const auto *CallF = dyn_cast<FunctionDecl>((*Itr)->getDecl());
+ if (CallF && !isFunctionAsyncSafe(CallF)) {
+ assert(Itr.getPathLength() >= 2);
+ reportBug(CallF, findCallExpr(Itr.getPath(Itr.getPathLength() - 2), *Itr),
+ SignalCall, HandlerDecl);
+ }
}
}
-bool SignalHandlerCheck::isSystemCallAllowed(const FunctionDecl *FD) const {
+bool SignalHandlerCheck::isFunctionAsyncSafe(const FunctionDecl *FD) const {
+ if (isSystemCall(FD))
+ return isSystemCallAsyncSafe(FD);
+ // For external (not checkable) functions assume that these are unsafe.
+ return FD->hasBody();
+}
+
+bool SignalHandlerCheck::isSystemCallAsyncSafe(const FunctionDecl *FD) const {
const IdentifierInfo *II = FD->getIdentifier();
// Unnamed functions are not explicitly allowed.
if (!II)
diff --git a/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h b/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
index 51f273ffa51e..9ac77a5cb323 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
@@ -10,6 +10,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SIGNALHANDLERCHECK_H
#include "../ClangTidyCheck.h"
+#include "clang/Analysis/CallGraph.h"
#include "llvm/ADT/StringSet.h"
namespace clang {
@@ -31,9 +32,12 @@ class SignalHandlerCheck : public ClangTidyCheck {
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
private:
+ bool isFunctionAsyncSafe(const FunctionDecl *FD) const;
+ bool isSystemCallAsyncSafe(const FunctionDecl *FD) const;
void reportBug(const FunctionDecl *CalledFunction, const Expr *CallOrRef,
const CallExpr *SignalCall, const FunctionDecl *HandlerDecl);
- bool isSystemCallAllowed(const FunctionDecl *FD) const;
+
+ CallGraph CG;
AsyncSafeFunctionSetType AsyncSafeFunctionSet;
llvm::StringSet<> &ConformingFunctions;
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
index e0b2c24f44ed..ac9b731cdc7a 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
@@ -51,6 +51,37 @@ void handler_extern(int) {
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
}
+void test_false_condition(int) {
+ if (0)
+ printf("1234");
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+}
+
+void test_multiple_calls(int) {
+ f_extern();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+ printf("1234");
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+ f_extern();
+ // first 'f_extern' call found only
+}
+
+void f_recursive();
+
+void test_recursive(int) {
+ f_recursive();
+ printf("");
+ // first 'printf' call (in other function) found only
+}
+
+void f_recursive() {
+ f_extern();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+ printf("");
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+ f_recursive(2);
+}
+
void test() {
signal(SIGINT, handler_abort);
signal(SIGINT, handler_signal);
@@ -66,4 +97,8 @@ void test() {
signal(SIGINT, SIG_IGN);
signal(SIGINT, SIG_DFL);
+
+ signal(SIGINT, test_false_condition);
+ signal(SIGINT, test_multiple_calls);
+ signal(SIGINT, test_recursive);
}
More information about the cfe-commits
mailing list