[PATCH] D118996: [clang-tidy] Support C++14 in bugprone-signal-handler.
Whisperity via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 1 03:53:40 PST 2022
whisperity added a reviewer: whisperity.
whisperity added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:142-149
Finder->addMatcher(
callExpr(callee(SignalFunction), hasArgument(1, HandlerExpr))
.bind("register_call"),
this);
+ Finder->addMatcher(
+ callExpr(callee(SignalFunction), hasArgument(1, HandlerLambda))
+ .bind("register_call"),
----------------
LegalizeAdulthood wrote:
> Is there any utility/performance to be gained by combining these into a single matcher?
> e.g.
> ```
> callExpr(callee(SignalFunction), anyOf(hasArgument(1, HandlerExpr), hasArgument(1, HandlerLambda)))
> ```
> (not sure if that is syntactically valid, but you get my point)
Or perhaps
```
callExpr(callee(SignalFunction), hasArgument(1, expr(anyOf(HandlerExpr, HandlerLambda))));
```
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:247
+ if (!getLangOpts().CPlusPlus17 && getLangOpts().CPlusPlus)
+ return checkFunctionCPP14(FD, CallOrRef, ChainReporter);
----------------
balazske wrote:
> LegalizeAdulthood wrote:
> > How can the first expression here ever be false when
> > we rejected C++17 in the `isLanguageVersionSupported`
> > override?
> I was thinking for the case when this check supports C++17 too. The change can be added later, this makes current code more readable.
(True. I think @LegalizeAdulthood's comment can be marked as Done.)
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:41
+bool isInGlobalNamespace(const FunctionDecl *FD) {
+ const DeclContext *DC = FD->getDeclContext();
----------------
Is this feature not available in (some parent class of) `FunctionDecl`?
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:46
+ assert(DC && "Function should have a declaration context.");
+ return DC->isTranslationUnit();
+}
----------------
(Perhaps a >= C++20 fixme that this query here //might// not handle modules well?)
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:94
+SourceRange getSourceRangeOfStmt(const Stmt *S, ASTContext &Ctx) {
+ ParentMapContext &PM = Ctx.getParentMapContext();
----------------
Okay, this is interesting... Why is `Ctx` not `const`? Unless **`get`**`Something()` is changing things (which is non-intuitive then...), as far as I can tell, you're only reading from `Ctx`.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:96
+ ParentMapContext &PM = Ctx.getParentMapContext();
+ DynTypedNode P = DynTypedNode::create(*S);
+ while (P.getSourceRange().getBegin().isInvalid()) {
----------------
Are these objects cleaned up properly in their destructor (same question for `DynTypeNodeList`)?
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:97
+ DynTypedNode P = DynTypedNode::create(*S);
+ while (P.getSourceRange().getBegin().isInvalid()) {
+ DynTypedNodeList PL = PM.getParents(P);
----------------
`SourceRange` itself should have a sentinel query. Why is only the beginning of it interesting?
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:99-101
+ if (PL.size() != 1)
+ return {};
+ P = PL[0];
----------------
`size != 1` could still mean `size == 0` in which case taking an element seems dangerous. Is triggering such UB possible here?
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:153
+ diag(HandlerLambda->getBeginLoc(),
+ "lambda function is not allowed as signal handler (until C++17)")
+ << HandlerLambda->getSourceRange();
----------------
I am trying to find some source for this claim but so far hit a blank. 😦 Could you please elaborate where this information comes from? Most notably, [[ https://en.cppreference.com/w/cpp/utility/program/signal | std::signal on CppReference ]] makes no mention of this, which would be the first place I would expect most people to look at.
Maybe this claim is derived from the rule that signal handlers **MUST** have C linkage? (If so, is there a warning against people setting C++-linkage functions as signal handlers in this check in general?)
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:162
const auto *HandlerExpr = Result.Nodes.getNodeAs<DeclRefExpr>("handler_expr");
+
assert(SignalCall && HandlerDecl && HandlerExpr &&
----------------
(Nit: maybe it is better without this newline, to visibly demarcate the "local state init "block"" of the function.)
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:178-179
+ // Check the handler function.
+ // The warning is placed to the signal handler registration.
+ // No need to display a call chain and no need for more checks.
+ (void)checkFunction(HandlerDecl, HandlerExpr, [](bool) {});
----------------
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:180
+ // No need to display a call chain and no need for more checks.
+ (void)checkFunction(HandlerDecl, HandlerExpr, [](bool) {});
return;
----------------
What is happening here? Why is the last parameter a callback(?) taking a `bool`? Why is the lambda empty? I see it is a `std::function`, can't we instead pass an empty function object there, and guard against calling it in `checkFunction`, making things a bit more explicit?
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:194-195
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);
+ // The problem was found in a function that was called from a signal
+ // handler, or in the direct signal handler function.
+ // Generate notes for the whole call chain (including the signal handler
----------------
`problem`?
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:223-227
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;
----------------
🔮 Ever since D98635, `diag()` of Clang-Tidy //should// support full undersquigglying of entities in diagnostics. Perhaps if you're working on this check already, it would be a nice addition to this check's output! All you need to do is `<< /* something that is a SourceRange */;` into the result of a `diag()` call. 😃
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:228
<< FD << FunctionIsCalled;
+ ChainReporter(/*SkipPathEnd=*/true);
return true;
----------------
Either this... 🗡️
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:228
<< FD << FunctionIsCalled;
+ ChainReporter(/*SkipPathEnd=*/true);
return true;
----------------
whisperity wrote:
> Either this... 🗡️
🗡️ ... or this.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:235-239
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;
----------------
(🔮 Ditto.)
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:240
<< FD << FunctionIsCalled;
+ ChainReporter(/*SkipPathEnd=*/true);
return true;
----------------
(🗡️ Ditto.)
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:254-256
+ diag(CallOrRef->getBeginLoc(),
+ "functions with other than C linkage are not allowed as signal "
+ "handler (until C++17)");
----------------
(🔮 Ditto.)
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:257
+ "handler (until C++17)");
+ ChainReporter(/*SkipPathEnd=*/true);
+ return true;
----------------
(🗡️ Ditto.)
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:277-279
+ diag(R.getBegin(),
+ "C++ only construct is not allowed in signal handler (until C++17)")
+ << R;
----------------
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:280-281
+ << R;
+ diag(R.getBegin(), "statement class is: '%0'", DiagnosticIDs::Note)
+ << Name;
+ ChainReporter(/*SkipPathEnd=*/false);
----------------
I think this is a fine moment to use the `Remark` diagnostic category! ✨ It is very likely that the average client will not look into Clang internals here... (It is dubious whether or not emitting this diagnostic is meaningful in the first place, but at least it helps with //some// debugging/understanding for those who wish to dig deep.)
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h:27
public:
enum class AsyncSafeFunctionSetType { Minimal, POSIX };
----------------
Reading "set type" below where this is used, I would've thought this will be a specific instantiation of `SparseSet` at first.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h:39-40
/// Should not check function calls in the code (this part is done by the call
- /// graph scan).
+ /// graph scan). If a problem is found in the "function properties" (other
+ /// than the code body) no more problems are to be reported. Otherwise every
+ /// found problem in the code body should be reported (not only the first
----------------
What is a "function property"? (Or is that an implementation detail of the check?)
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h:50
+ /// function. This should be called at every generated bug report.
+ /// The bool parameter is used like `SkipPathEnd` in `reportHandlerChain`.
/// @return true only if a problem was found in the function.
----------------
(Format ditto.)
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h:54
+ std::function<void(bool)> ChainReporter);
+ /// Similar as `checkFunction` but only check for C++14 rules.
+ bool checkFunctionCPP14(const FunctionDecl *FD, const Expr *CallOrRef,
----------------
(Nit: to keep consistency with others and to ensure documentation renders properly, use `@p checkFunction` instead of backtick.)
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h:57
+ std::function<void(bool)> ChainReporter);
/// Return if a system call function is considered as asynchronous-safe.
bool isSystemCallAsyncSafe(const FunctionDecl *FD) const;
----------------
================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:118-119
+- :doc:`bugprone-signal-handler
+ <clang-tidy/checks/bugprone-signal-handler>` check now supports
+ (partially) signal handler rules for C++14. Bug report generation is
+ improved.
----------------
swap `supports` and `partially`
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst:8
+constructs that can cause undefined behavior. The rules for what is
+allowed are different for various C++ language versions.
+
----------------
(Is it the same for C all across the board?)
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst:10-19
+For C it is checked if non-asynchronous-safe functions are called.
+
+For C++ until (and including) C++14 such code constructs are found
+that are C++-specific and therefore not recommended in signal handlers.
+Additionally the calls to non-asynchronous-safe functions are found
+too and a signal handler or any function called from it is allowed
+to have only extern C linkage. Restrictions related to atomic
----------------
I would turn these into bullet points:
* For C, it is checked [...]
* Up to and including C++14, [...]
* Since C++17, [...]
* C++17 and newer standards are not implemented in this check.
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst:12-16
+For C++ until (and including) C++14 such code constructs are found
+that are C++-specific and therefore not recommended in signal handlers.
+Additionally the calls to non-asynchronous-safe functions are found
+too and a signal handler or any function called from it is allowed
+to have only extern C linkage. Restrictions related to atomic
----------------
Turning these into bullet points will make it easier for the clients to read and you can optimise away this troublesome sentence.
* Up to and including C++14, there are several language constructs which are unsafe to call from a signal handler.
** The signal handler, and every function it calls, **MUST** have C linkage.
** Called functions must be asynchronous-safe.
** etc.
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst:16
+too and a signal handler or any function called from it is allowed
+to have only extern C linkage. Restrictions related to atomic
+lock-free operations are not checked.
----------------
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst:22
+Any function that cannot be determined to be an asynchronous-safe
function call is assumed to be non-asynchronous-safe by the checker,
including user functions for which only the declaration is visible.
----------------
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst:24
including user functions for which only the declaration is visible.
User function calls with visible definition are checked recursively.
+Only the function names are considered and the fact that the function
----------------
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst:25-28
+Only the function names are considered and the fact that the function
+is a system-call, but no other restrictions on the arguments passed
+to the functions (the ``signal`` call is allowed without
restrictions).
----------------
I would rephrase this paragraph. You say that //only the names are considered// but the "against **what**" is not clear at this moment. Start with the expected outcome, the "result" function of this consideration:
> Asnychronous-safety is determined by comparing the function's name against a set of known functions. In addition, the function must be a system call. The (possible) arguments passed to the function are not checked.
Just as in D118370, I'm not exactly sure what is a //system call// here, as `printf` and `memcpy` are (standard) library functions. Either this //system call// terminology should be dropped in favour of //standard function// perhaps, or the details of this be made explicit. Looking at the implementation of the check, the predicate for //system call// is: `comes from a system header include` + `is in a global namespace` + ???.
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst:36
+<https://wiki.sei.cmu.edu/confluence/display/cplusplus/MSC54-CPP.+A+signal+handler+must+be+a+plain+old+function>`_.
+It has alias names ``cert-sig30-c`` and ``cert-msc54-cpp``.
----------------
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst:40-53
Selects which set of functions is considered as asynchronous-safe
(and therefore allowed in signal handlers). Value ``minimal`` selects
a minimal set that is defined in the CERT SIG30-C rule and includes functions
``abort()``, ``_Exit()``, ``quick_exit()`` and ``signal()``. Value ``POSIX``
selects a larger set of functions that is listed in POSIX.1-2017 (see `this
link
<https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03>`_
----------------
Again, this could use a re-working as a list of bullet-points.
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst:42
(and therefore allowed in signal handlers). Value ``minimal`` selects
a minimal set that is defined in the CERT SIG30-C rule and includes functions
``abort()``, ``_Exit()``, ``quick_exit()`` and ``signal()``. Value ``POSIX``
----------------
(I'd repeat the external URL and make `CERT SIG30-C rule` clickable.)
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst:44-47
selects a larger set of functions that is listed in POSIX.1-2017 (see `this
link
<https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03>`_
for more information).
----------------
Nit: Perhaps the link could be worked into the `POSIX.1-2017` somehow?
> that is listed in [[ http://example.com | POSIX.1-2017 (§ 2.4.3. Signal actions) ]].
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c:21
// 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
----------------
I really like that this trivial bit stops being repeated! Is this what the `SkipPathEnd` does in practice?
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c:35
void test_extern() {
- signal(SIGINT, handler_extern);
+ if (SIG_ERR == signal(SIGINT, handler_extern))
+ return;
----------------
I went back to the implementation but I am still unsure what is being exercised by this change. Is it that we are not sensitive to conditionals and still think that the `signal` call will set `handler_extern` to be a handler no matter what, and thus we produce diagnostics? I think this is worthy of a comment in the file itself, because as far as I can tell there are no matchers that try to catch `if`s in the check's implementation.
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c:168-172
void test_function_pointer() {
signal(SIGINT, handler_function_pointer);
+ sighandler_t handler_ptr = f_extern;
+ signal(SIGINT, handler_ptr);
}
----------------
What is the expected behavioural change by this? I don't see lines referring these `signal()` calls in other test cases. Should there be diagnostics but not yet implemented? Or should there just be no crashes? Both cases I believe should be mentioned in the comments.
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.cpp:5
+#include "stdio.h"
+//#include "signal.h"
+
----------------
Stale comment. Or perhaps this is important?
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.cpp:59
+void test() {
+ // Do not find problems here.
+ signal(SIGINT, handler_unsafe, 1);
----------------
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118996/new/
https://reviews.llvm.org/D118996
More information about the cfe-commits
mailing list