[PATCH] D33825: [clang-tidy] signal handler must be plain old function check

[JF Bastien via Phabricator via cfe-commits]

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