balazske updated this revision to Diff 404040.
balazske added a comment.
Even more simplification, added documentation.
Text of bug reports is more detailed, no NFC now.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118370/new/
https://reviews.llvm.org/D118370
Files:
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
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
@@ -17,7 +17,7 @@
void handler_printf(int) {
printf("1234");
- // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+ // CHECK-NOTES: :[[@LINE-1]]:3: warning: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
// CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'handler_printf'
// CHECK-NOTES: :[[@LINE+4]]:18: note: function 'handler_printf' registered here as signal handler
}
@@ -28,7 +28,7 @@
void handler_extern(int) {
f_extern();
- // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+ // CHECK-NOTES: :[[@LINE-1]]:3: warning: can not verify if external function 'f_extern' is asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
// CHECK-NOTES: :[[@LINE-2]]:3: note: function 'f_extern' called here from 'handler_extern'
// CHECK-NOTES: :[[@LINE+4]]:18: note: function 'handler_extern' registered here as signal handler
}
@@ -51,7 +51,7 @@
void f_bad() {
printf("1234");
- // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+ // CHECK-NOTES: :[[@LINE-1]]:3: warning: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
// CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'f_bad'
// CHECK-NOTES: :[[@LINE+5]]:3: note: function 'f_bad' called here from 'handler_bad'
// CHECK-NOTES: :[[@LINE+8]]:18: note: function 'handler_bad' registered here as signal handler
@@ -67,7 +67,7 @@
void f_bad1() {
printf("1234");
- // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+ // CHECK-NOTES: :[[@LINE-1]]:3: warning: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
// CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'f_bad1'
// CHECK-NOTES: :[[@LINE+6]]:3: note: function 'f_bad1' called here from 'f_bad2'
// CHECK-NOTES: :[[@LINE+9]]:3: note: function 'f_bad2' called here from 'handler_bad1'
@@ -99,7 +99,7 @@
void handler_false_condition(int) {
if (0)
printf("1234");
- // CHECK-NOTES: :[[@LINE-1]]:5: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+ // CHECK-NOTES: :[[@LINE-1]]:5: warning: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
// CHECK-NOTES: :[[@LINE-2]]:5: note: function 'printf' called here from 'handler_false_condition'
// CHECK-NOTES: :[[@LINE+4]]:18: note: function 'handler_false_condition' registered here as signal handler
}
@@ -110,11 +110,11 @@
void handler_multiple_calls(int) {
f_extern();
- // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+ // CHECK-NOTES: :[[@LINE-1]]:3: warning: can not verify if external function 'f_extern' is asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
// CHECK-NOTES: :[[@LINE-2]]:3: note: function 'f_extern' called here from 'handler_multiple_calls'
// CHECK-NOTES: :[[@LINE+10]]:18: note: function 'handler_multiple_calls' registered here as signal handler
printf("1234");
- // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+ // CHECK-NOTES: :[[@LINE-1]]:3: warning: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
// CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'handler_multiple_calls'
// CHECK-NOTES: :[[@LINE+6]]:18: note: function 'handler_multiple_calls' registered here as signal handler
f_extern();
@@ -135,12 +135,12 @@
void f_recursive() {
f_extern();
- // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+ // CHECK-NOTES: :[[@LINE-1]]:3: warning: can not verify if external function 'f_extern' is asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
// CHECK-NOTES: :[[@LINE-2]]:3: note: function 'f_extern' called here from 'f_recursive'
// CHECK-NOTES: :[[@LINE-9]]:3: note: function 'f_recursive' called here from 'handler_recursive'
// CHECK-NOTES: :[[@LINE+10]]:18: note: function 'handler_recursive' registered here as signal handler
printf("");
- // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+ // CHECK-NOTES: :[[@LINE-1]]:3: warning: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
// CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'f_recursive'
// CHECK-NOTES: :[[@LINE-14]]:3: note: function 'f_recursive' called here from 'handler_recursive'
// CHECK-NOTES: :[[@LINE+5]]:18: note: function 'handler_recursive' registered here as signal handler
@@ -153,7 +153,7 @@
void f_multiple_paths() {
printf("");
- // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+ // CHECK-NOTES: :[[@LINE-1]]:3: warning: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
// CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'f_multiple_paths'
// CHECK-NOTES: :[[@LINE+5]]:3: note: function 'f_multiple_paths' called here from 'handler_multiple_paths'
// CHECK-NOTES: :[[@LINE+9]]:18: note: function 'handler_multiple_paths' registered here as signal handler
@@ -184,9 +184,9 @@
signal(SIGINT, _Exit);
signal(SIGINT, other_call);
- // CHECK-NOTES: :[[@LINE-1]]:18: warning: 'other_call' may not be asynchronous-safe; using it as a signal handler may be dangerous [bugprone-signal-handler]
+ // CHECK-NOTES: :[[@LINE-1]]:18: warning: system call 'other_call' may not be asynchronous-safe; using it as a signal handler may be dangerous [bugprone-signal-handler]
signal(SIGINT, f_extern);
- // CHECK-NOTES: :[[@LINE-1]]:18: warning: 'f_extern' may not be asynchronous-safe; using it as a signal handler may be dangerous [bugprone-signal-handler]
+ // CHECK-NOTES: :[[@LINE-1]]:18: warning: can not verify if external function 'f_extern' is asynchronous-safe; using it as a signal handler may be dangerous [bugprone-signal-handler]
signal(SIGINT, SIG_IGN);
signal(SIGINT, SIG_DFL);
Index: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
+++ clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
@@ -33,22 +33,36 @@
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
private:
- bool isFunctionAsyncSafe(const FunctionDecl *FD) const;
+ /// Check if a function is allowed as signal handler.
+ /// Should test properties of the function, and check in the code body.
+ /// Should not check function calls in the code (this part is done by the call
+ /// graph scan).
+ /// @param FD The function to check. It may or may not have a definition.
+ /// @param CallOrRef Location of the call to this function (in another
+ /// function) or the reference to the function (if it is used as registered
+ /// signal handler). This is the location where diagnostics are to be placed.
+ /// @return true only if problem was found in the function.
+ bool checkFunction(const FunctionDecl *FD, const Expr *CallOrRef);
+ /// Return if a system call function is considered as asynchronous-safe.
bool isSystemCallAsyncSafe(const FunctionDecl *FD) const;
- void reportBug(const FunctionDecl *CalledFunction, const Expr *CallOrRef,
- bool DirectHandler);
- void reportHandlerCommon(llvm::df_iterator<clang::CallGraphNode *> Itr,
- const CallExpr *SignalCall,
- const FunctionDecl *HandlerDecl,
- const Expr *HandlerRef);
+ /// Add bug report notes to show the call chain of functions from signal
+ /// handler to an actual called function (from it).
+ /// @param Itr Position during a call graph depth-first iteration. It contains
+ /// the "path" (call chain) from signal handler to the actual found function
+ /// call.
+ /// @param HandlerRef Reference to the signal handler function where it is
+ /// registered (considered as first part of the call chain).
+ void reportHandlerChain(const llvm::df_iterator<clang::CallGraphNode *> &Itr,
+ const Expr *HandlerRef);
clang::CallGraph CG;
AsyncSafeFunctionSetType AsyncSafeFunctionSet;
- llvm::StringSet<> &ConformingFunctions;
+ const llvm::StringSet<> &ConformingFunctions;
- static llvm::StringSet<> MinimalConformingFunctions;
- static llvm::StringSet<> POSIXConformingFunctions;
+ // FIXME: Find a better place for these string sets (TableGen?).
+ static const llvm::StringSet<> MinimalConformingFunctions;
+ static const llvm::StringSet<> POSIXConformingFunctions;
};
} // namespace bugprone
Index: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
@@ -142,38 +142,60 @@
"There should be at least one function added to call graph.");
}
- // Check for special case when the signal handler itself is an unsafe external
- // function.
- if (!isFunctionAsyncSafe(HandlerDecl)) {
- reportBug(HandlerDecl, HandlerExpr, /*DirectHandler=*/true);
+ if (!HandlerDecl->hasBody()) {
+ checkFunction(HandlerDecl, HandlerExpr);
return;
}
- CallGraphNode *HandlerNode = CG.getNode(HandlerDecl);
- // Signal handler can be external but not unsafe, no call graph in this case.
- if (!HandlerNode)
- return;
+ CallGraphNode *HandlerNode = CG.getNode(HandlerDecl->getCanonicalDecl());
+ assert(HandlerNode &&
+ "Handler with body should be present in the call graph.");
// 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),
- /*DirectHandler=*/false);
- reportHandlerCommon(Itr, SignalCall, HandlerDecl, HandlerExpr);
+ if (CallF) {
+ unsigned int PathL = Itr.getPathLength();
+ const Expr *CallOrRef = (PathL == 1)
+ ? HandlerExpr
+ : findCallExpr(Itr.getPath(PathL - 2), *Itr);
+ if (checkFunction(CallF, CallOrRef))
+ reportHandlerChain(Itr, HandlerExpr);
}
}
}
-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::checkFunction(const FunctionDecl *FD,
+ const Expr *CallOrRef) {
+ const bool FunctionIsCalled = isa<CallExpr>(CallOrRef);
+
+ if (isSystemCall(FD)) {
+ if (!isSystemCallAsyncSafe(FD)) {
+ diag(CallOrRef->getBeginLoc(), "system call %0 may not be "
+ "asynchronous-safe; "
+ "%select{using it as|calling it from}1 "
+ "a signal handler may be dangerous")
+ << FD << FunctionIsCalled;
+ return true;
+ }
+ return false;
+ }
+
+ if (!FD->hasBody()) {
+ diag(CallOrRef->getBeginLoc(), "can not verify if external function %0 is "
+ "asynchronous-safe; "
+ "%select{using it as|calling it from}1 "
+ "a signal handler may be dangerous")
+ << FD << FunctionIsCalled;
+ return true;
+ }
+
+ return false;
}
bool SignalHandlerCheck::isSystemCallAsyncSafe(const FunctionDecl *FD) const {
+ assert(isSystemCall(FD));
+
const IdentifierInfo *II = FD->getIdentifier();
// Unnamed functions are not explicitly allowed.
if (!II)
@@ -186,17 +208,9 @@
return false;
}
-void SignalHandlerCheck::reportBug(const FunctionDecl *CalledFunction,
- const Expr *CallOrRef, bool DirectHandler) {
- diag(CallOrRef->getBeginLoc(),
- "%0 may not be asynchronous-safe; %select{calling it from|using it as}1 "
- "a signal handler may be dangerous")
- << CalledFunction << DirectHandler;
-}
-
-void SignalHandlerCheck::reportHandlerCommon(
- llvm::df_iterator<clang::CallGraphNode *> Itr, const CallExpr *SignalCall,
- const FunctionDecl *HandlerDecl, const Expr *HandlerRef) {
+void SignalHandlerCheck::reportHandlerChain(
+ const llvm::df_iterator<clang::CallGraphNode *> &Itr,
+ const Expr *HandlerRef) {
int CallLevel = Itr.getPathLength() - 2;
assert(CallLevel >= -1 && "Empty iterator?");
@@ -214,12 +228,12 @@
diag(HandlerRef->getBeginLoc(),
"function %0 registered here as signal handler", DiagnosticIDs::Note)
- << HandlerDecl;
+ << cast<FunctionDecl>(Caller->getDecl());
}
// This is the minimal set of safe functions.
// https://wiki.sei.cmu.edu/confluence/display/c/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers
-llvm::StringSet<> SignalHandlerCheck::MinimalConformingFunctions{
+const llvm::StringSet<> SignalHandlerCheck::MinimalConformingFunctions{
"signal", "abort", "_Exit", "quick_exit"};
// The POSIX-defined set of safe functions.
@@ -228,7 +242,7 @@
// mentioned POSIX specification was not updated after 'quick_exit' appeared
// in the C11 standard.
// Also, we want to keep the "minimal set" a subset of the "POSIX set".
-llvm::StringSet<> SignalHandlerCheck::POSIXConformingFunctions{
+const llvm::StringSet<> SignalHandlerCheck::POSIXConformingFunctions{
"_Exit",
"_exit",
"abort",
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
