[PATCH] D33825: [clang-tidy] signal handler must be plain old function check
JF Bastien via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sat Aug 29 15:47:49 PDT 2020
jfb requested changes to this revision.
jfb added a comment.
This revision now requires changes to proceed.
MSC54-CPP refers to POF, which as I pointed out above isn't relevant anymore. I'd much rather have a diagnostic which honors the state of things after http://wg21.link/p0270.
Additionally, lots of what MSC54-CPP intends is implementation-defined. We shouldn't diagnose for things that clang has defined as signal safe, at least not by default.
>From the paper, I'd like to see tests for these:
- a call to any standard library function, except for plain lock-free atomic atomic operations and functions explicitly identified as signal-safe. [Note: This implicitly excludes the use of new and delete expressions that rely on a library-provided memory allocator. – end note]; -- here I'd like to explicitly see the list of signal-safe functions, and examples of ones that are not
- an access to an object with thread storage duration;
- a dynamic_cast expression;
- throwing of an exception;
- control entering a try-block or function-try-block; or
- initialization of a variable with static storage duration requiring dynamic initialization (3.6.3 [basic.start.dynamic], 6.7 [stmt.decl])). [ Note: Such initialization might occur because it is the first odr-use (3.2 [basic.def.odr]) of that variable. -- end note ]
- waiting for the completion of the initialization of a variable with static storage duration (6.7 [stmt.decl]).
================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:84
+ else if (llvm::isa<CXXUnresolvedConstructExpr>(stmt))
+ return "Construction of an unresolved type here";
+ else
----------------
Most of the above are fine per p0270, and most don't have a test at the moment.
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-msc54-cpp.rst:15
+ static void sig_handler(int sig) {}
+ // warning: use 'external C' prefix for signal handlers
+
----------------
'extern', not 'external'
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-msc54-cpp.rst:23
+ extern "C" void cpp_signal_handler(int sig) {
+ // warning: do not use C++ constructs in signal handlers
+ throw "error message";
----------------
Update the example too.
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:74
+ // CHECK-MESSAGES: :[[@LINE+2]]:3: note: function called here
+ // CHECK-MESSAGES: TestClass::static_function();
+ TestClass::static_function();
----------------
I don't think this should diagnose, it is signal safe for clang's implementation, and allowed by p0270.
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:91
+ auto char c = '1';
+ extern _Thread_local double _Complex d;
+ static const unsigned long int i = sizeof(float);
----------------
_Thread_local isn't signal safe per p0270.
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:114
+ do {
+ _Atomic int v = _Alignof(char);
+ _Static_assert(42 == 42, "True");
----------------
The atomic isn't used here, and atomic initialization isn't atomic, so the test isn't sufficient.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D33825/new/
https://reviews.llvm.org/D33825
More information about the cfe-commits
mailing list