[PATCH] D118370: [clang-tidy] bugprone-signal-handler: Code refactor (NFC)
balazske marked 2 inline comments as done.
balazske added inline comments.
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:151
CallGraphNode *HandlerNode = CG.getNode(HandlerDecl);
- // Signal handler can be external but not unsafe, no call graph in this case.
- if (!HandlerNode)
-return;
+ assert(HandlerNode &&
+ "Handler has body, should be present in the call graph.");
balazske wrote:
> There are problems with this approach in C++ code.
The call graph adds nodes for the canonical declaration only. When getting a
node it works only with the CanonicalDecl unless the `CallGraph` code is
changed (it seems to be worth to do this change).
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:167
+ const Expr *CallOrRef) {
+ const bool FunctionIsCalled = isa(CallOrRef);
+
njames93 wrote:
> This can probably just be inlined into its only use.
The value is used now multiple times. (But maybe the `diag` call can be put
into a lambda later.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118370/new/
https://reviews.llvm.org/D118370
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D118370: [clang-tidy] bugprone-signal-handler: Code refactor (NFC)
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'
[PATCH] D118370: [clang-tidy] bugprone-signal-handler: Code refactor (NFC)
njames93 added inline comments.
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:167
+ const Expr *CallOrRef) {
+ const bool FunctionIsCalled = isa(CallOrRef);
+
This can probably just be inlined into its only use.
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h:36
private:
+ bool checkFunction(const FunctionDecl *FD, const Expr *CallOrRef);
bool isFunctionAsyncSafe(const FunctionDecl *FD) const;
This isn't a very descriptive name, and the bool return value conveys no
meaning. Probably could do with some documentation.
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h:46-49
llvm::StringSet<>
static llvm::StringSet<> MinimalConformingFunctions;
static llvm::StringSet<> POSIXConformingFunctions;
While you're refactoring, those static StringSets really belong in the
implementation file, and the ConformingFunctions should be a const ref.
However global destructores are kind of a taboo thing, Maybe there is case to
change them into a sorted array and binary search them to see if a function is
conforming.
Probably wouldn't advise using TableGens StringMatcher to build this
functionality as that maybe a little too far.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118370/new/
https://reviews.llvm.org/D118370
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D118370: [clang-tidy] bugprone-signal-handler: Code refactor (NFC)
balazske added inline comments.
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:146
+ if (!HandlerDecl->hasBody()) {
+checkFunction(HandlerDecl, HandlerExpr);
return;
LegalizeAdulthood wrote:
> Why do we ignore the return value of `checkFunction` here?
>
> Also, the name doesn't reveal to me why the result is true
> or false, e.g. it acts like a predicate but its name doesn't ask
> a question.
The function returns if a problem (warning) was found. At the next call site it
is used to display additional notes, but here it is not needed. A better name
can be `isFunctionValidSignalHandler` but the function checks not only a fact,
it generates the warnings too.
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:151
CallGraphNode *HandlerNode = CG.getNode(HandlerDecl);
- // Signal handler can be external but not unsafe, no call graph in this case.
- if (!HandlerNode)
-return;
+ assert(HandlerNode &&
+ "Handler has body, should be present in the call graph.");
There are problems with this approach in C++ code.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118370/new/
https://reviews.llvm.org/D118370
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D118370: [clang-tidy] bugprone-signal-handler: Code refactor (NFC)
LegalizeAdulthood added inline comments.
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:146
+ if (!HandlerDecl->hasBody()) {
+checkFunction(HandlerDecl, HandlerExpr);
return;
Why do we ignore the return value of `checkFunction` here?
Also, the name doesn't reveal to me why the result is true
or false, e.g. it acts like a predicate but its name doesn't ask
a question.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118370/new/
https://reviews.llvm.org/D118370
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D118370: [clang-tidy] bugprone-signal-handler: Code refactor (NFC)
balazske created this revision.
Herald added subscribers: carlosgalvezp, steakhal, martong, gamesh411,
Szelethus, dkrupp, xazax.hun.
balazske requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
Another change of the code design.
Code simplified again, now there is a single place to check
a handler function and less functions for bug report emitting.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D118370
Files:
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
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,14 +33,12 @@
void check(const ast_matchers::MatchFinder::MatchResult ) override;
private:
+ bool checkFunction(const FunctionDecl *FD, const Expr *CallOrRef);
bool isFunctionAsyncSafe(const FunctionDecl *FD) const;
bool isSystemCallAsyncSafe(const FunctionDecl *FD) const;
- void reportBug(const FunctionDecl *CalledFunction, const Expr *CallOrRef,
- bool DirectHandler);
- void reportHandlerCommon(llvm::df_iterator Itr,
- const CallExpr *SignalCall,
- const FunctionDecl *HandlerDecl,
- const Expr *HandlerRef);
+ void reportHandlerChain(const llvm::df_iterator ,
+ const FunctionDecl *HandlerDecl,
+ const Expr *HandlerRef);
clang::CallGraph CG;
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,30 +142,41 @@
"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;
+ assert(HandlerNode &&
+ "Handler has 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((*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 && CallF != HandlerDecl) {
+ if (checkFunction(
+ CallF, findCallExpr(Itr.getPath(Itr.getPathLength() - 2), *Itr)))
+reportHandlerChain(Itr, HandlerDecl, HandlerExpr);
}
}
}
+bool SignalHandlerCheck::checkFunction(const FunctionDecl *FD,
+ const Expr *CallOrRef) {
+ const bool FunctionIsCalled = isa(CallOrRef);
+
+ if (!isFunctionAsyncSafe(FD)) {
+diag(CallOrRef->getBeginLoc(), "%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;
+}
+
bool SignalHandlerCheck::isFunctionAsyncSafe(const FunctionDecl *FD) const {
if (isSystemCall(FD))
return isSystemCallAsyncSafe(FD);
@@ -186,16 +197,8 @@
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 Itr, const CallExpr *SignalCall,
+void SignalHandlerCheck::reportHandlerChain(
+const llvm::df_iterator ,
const FunctionDecl *HandlerDecl, const Expr *HandlerRef) {
int CallLevel = Itr.getPathLength() - 2;
assert(CallLevel >= -1 && "Empty iterator?");
Index: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
===
---
